Update undesirable behavior impact collection details #939
Conversation
There was a problem hiding this comment.
Hey this looks great! Having been reviewing previous PRs related to assertions, these UI changes actually helped me put it all together and understand the experience we were driving towards. I'm liking how this has evolved this year.
While testing I found a situation that could lead to the "behavior cell" being blank. As I described to you offline, you have to submit the test result with an empty details field. I'll come back later and verify the fix for that.
| highImpactPassedAssertionCount, | ||
| highImpactFailedAssertionCount, | ||
| severeImpactPassedAssertionCount, | ||
| severeImpactFailedAssertionCount, | ||
| moderateImpactPassedAssertionCount, | ||
| moderateImpactFailedAssertionCount, | ||
| commandsCount, |
There was a problem hiding this comment.
Since metrics are saved in the database does that mean we should have a migration to make sure the metrics are up-to-date?
There was a problem hiding this comment.
Since this hasn't yet gone to the production environment yet to affect that database, no need to write that migration. There wouldn't be any metrics that include those properties just yet.
| @@ -89,9 +89,9 @@ const TestPlanResultsTable = ({ | |||
| { | |||
| id: `UnexpectedBehavior_MUST_${nextId()}`, | |||
| assertion: { | |||
| text: 'Other behaviors that create high negative-impacts are not exhibited' | |||
| text: 'Other behaviors that create severe negative-impacts are not exhibited' | |||
There was a problem hiding this comment.
The hyphen here isn't grammatically correct, it should be 'Other behaviors that create severe negative impacts are not exhibited'.
There was a problem hiding this comment.
True, although unsure if it was intentional. This could also be shortened in the future soon to:
Severe negative side effects do not occur
If you'd like to weigh in on that discussion.
| responsive | ||
| aria-label={`Undesirable behaviors for test ${test.title}`} | ||
| className={`test-plan-undesirable-behaviors-table ${tableClassName}`} | ||
| > |
There was a problem hiding this comment.
For consistency, where possible can we call these unexpected behaviors?
| @@ -262,8 +262,8 @@ const getMetrics = ({ | |||
| testsCount, | |||
| testsFailedCount, | |||
| unexpectedBehaviorCount, | |||
| highImpactPassedAssertionCount, | |||
| highImpactFailedAssertionCount, | |||
There was a problem hiding this comment.
The name highImpactFailedAssertionCount is still present in server/util/getMetrics.js is there a plan to remove it from there?
There was a problem hiding this comment.
I think this comment may have been before the latest changes from main were merged in here, so this may be outdated? server/util/getMetrics.js isn't on this branch anymore.
… after submitting results that previously excluded required input
This addresses feedback in #738 (comment).
This PR does the following:
Other behaviors that create high negative-impacts are not exhibitedto sayOther behaviors that create severe negative-impacts are not exhibitedinsteadOther behaviors that create negative impact:and presents the content in a table with Behavior, Details and Impact as the columnsNote that the aria-at sourced related changes are included in an open PR on that repository: w3c/aria-at#1016