-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand sample test content to handle more scenarios #1224
Conversation
@stalgiag I certainly don't think the additional data support here is exhaustive. Please let me know if this aligns with your thoughts or if there are any additional pressing scenarios to cover. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great set of changes! I tested locally and really love the expanded cases covered by this PR. I left some inline comments but nothing blocking a merge.
} | ||
} | ||
} | ||
`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up the errant indents!
* 'incompleteAndFailingDueToUnexpectedBehaviors' | | ||
* 'incompleteAndFailingDueToMultiple']} fakeTestResultTypes | ||
* @param {import('sequelize').Transaction} transaction | ||
* @param {'SINGLE' | 'ALL' | number} numFakeTestResultConflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little interesting. Could you just do a number with Math.Infinity
for 'ALL'? I guess it is a little strange to read 'infinity' when we really mean all. No strong feelings here but just struck me that this could be a single
testResult.scenarioResults
.slice(0, numFakeTestResultConflicts)
.forEach(setResult);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Addressed in 4b70b0b
|
||
// Slightly different from older already recommended TestPlanRun 13 to get | ||
// multiple results shown for: | ||
// Recommended Report + Checkbox Example (Mixed-State) + NVDA + Chrome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Addresses #759
This PR makes changes to the current test sample data to:
checkbox-tri-state
to have multiple advanced NVDA + Chrome Test Plan Reports to display the AT Versions Summary table/scripts/populate-test-data
)