-
Notifications
You must be signed in to change notification settings - Fork 16
Add ability to designate tests as "untestable" #1405
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
Conversation
cdea8a8
to
b3e657c
Compare
> In reports for commands with unexpected behaviors, change the phrase > that appears above the table of unexpected behaviors from: > > > Other behaviors that create negative impact: > > to: > > > Negative side effects of COMMAND_NAME > > and mark it up as a level 4 heading (same level as the command results > heading) > > Change the caption on the table from: > > > Undesirable behaviors for test TEST_NAME > > to: > > > Negative side effects of COMMAND_NAME > > In the table, change the title of the "Behavior" column to "Side > Effect". > If there were no negative side effects, just show the text, no > heading, e.g., > "Negative side effects of b (virtual cursor active): > None" > > Even better would be a clear statement: "The command 'b (virtual > cursor active)' did not cause any negative side effects."
892c569
to
b7e338a
Compare
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.
Looks good. I've left some thoughts on minor inline changes but for the most part, this is great, given how much changes are happening here! Also appreciate the update to the tests to make the intention of things here even clearer.
One blocking item though is that the the following strings haven't been updated in <TestRenderer />
:
- Were there additional undesirable behaviors? -> Did negative side effects occur? (code)
- No, there were no additional undesirable behaviors. -> No, negative side effects did not occur. (code)
- Yes, there were additional undesirable behaviors. -> Yes, negative side effects occured. (code)
- Undesirable behaviors -> Negative side effects (this appears when the above radio option is selected, and it wasn't listed in the linked issue but seems reasonable for that copy to be used here) (code)
But that comes from the aria-at harness so I'll leave my comment on that dependent PR instead on what needs to be changed.
client/components/CandidateReview/CandidateTestPlanRun/index.jsx
Outdated
Show resolved
Hide resolved
client/components/CandidateReview/CandidateTestPlanRun/index.jsx
Outdated
Show resolved
Hide resolved
import { TestPlanReportMetricsPropType } from '../common/proptypes'; | ||
import PropTypes from 'prop-types'; | ||
|
||
const NegativeSideEffecsSummaryHeading = ({ metrics, as: Element = 'h2' }) => { |
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.
const NegativeSideEffecsSummaryHeading = ({ metrics, as: Element = 'h2' }) => { | |
const NegativeSideEffectsSummaryHeading = ({ metrics, as: Element = 'h2' }) => { |
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.
I guess I'll just conduct my spelling experiments somewhere else. Your loss!
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.
Will miss it! 😅
Thanks for the review, @howard-e, especially such detailed references! |
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 @jugglinmike, great stuff! As I noted before, this is well done considering so much file changes happening at once.
Resolves gh-1352
Depends on w3c/aria-at#1248