Skip to content
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

Improve test result accessibility #3082

Merged
merged 14 commits into from Sep 4, 2022

Conversation

gibson042
Copy link
Contributor

  • Explain diff gutter symbols.
  • Add test result labels (color covers figure and label, and indicates passed-as-expected vs. not).
  • Bold the source line of errors in code excerpts.

Fixes #1558
Fixes #2919

example output

Color covers figure and label, and indicates passed-as-expected
vs. not (expected/unexpected fail or unexpected pass).

Fixes avajs#2919
@gibson042
Copy link
Contributor Author

This produces lots of failures like the following; how do I update the [TAP] test expectations in bulk?

        not ok 43 - should be equal
          ---
          compare: ===
          at:
            line: 59
            column: 6
            file: file:///home/runner/work/ava/ava/test-tap/assert.js
            function: assertFailure
          stack: |
            assertFailure (file://test-tap/assert.js:59:6)
            failsWith (file://test-tap/assert.js:98:2)
            Test.<anonymous> (file://test-tap/assert.js:277:2)
            file://test-tap/assert.js:202:1
          diff: |
            --- expected
            +++ actual
            @@ -1,1 +1,1 @@
            -Difference:
            +Difference (- actual, + expected):
          ...

@gibson042
Copy link
Contributor Author

This _seems_ to work, but the resulting interaction between CommonJS and ESM
feels a bit Zalgo-y. It may be better for modules to keep setting global
options and chalk options in parallel while the former remains CommonJS,
but I wanted to see what simplification would look like.
if (setChalk) {
setChalk(options.chalkOptions);
} else {
chalk.then(chalk => chalk.set(options.chalkOptions));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from 1b64bff, an independent experiment that can be reverted if deemed unsuccessful.

This seems to work, but the resulting interaction between CommonJS and ESM
feels a bit Zalgo-y. It may be better for modules to keep setting global
options and chalk options in parallel while the former remains CommonJS,
but I wanted to see what simplification would look like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for this PR? Can we discuss it in a separate PR? It's all a little complicated I know, do we need to change it? What do you think the benefits are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep it, because otherwise the new tests don't correspond with reality (because worker output does not consistently respect color settings) and will undergo a fair amount of churn. But if you feel strongly that it should be separated, I can do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a go at removing it, and other than test-tap/assert.js requiring an extra stripAnsi() there's no impact. I think I prefer the "set once" approach rather than having code react to changes.

lib/cli.js Outdated Show resolved Hide resolved
lib/concordance-options.js Show resolved Hide resolved
if (setChalk) {
setChalk(options.chalkOptions);
} else {
chalk.then(chalk => chalk.set(options.chalkOptions));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for this PR? Can we discuss it in a separate PR? It's all a little complicated I know, do we need to change it? What do you think the benefits are?

@novemberborn
Copy link
Member

@sindresorhus what do you think?

@novemberborn
Copy link
Member

Thank you @gibson042, very exciting!

Note that we're seeing similar CI failures on the main branch, so let's not worry about those here.

@sindresorhus
Copy link
Member

I agree with almost everything. However, I'm not a big fan of the PASS: and FAIL: noise. I think the icons are clear enough for the common case and we could instead only use the prefixes for the other cases like EXPECTED FAIL and SKIP.

@gibson042
Copy link
Contributor Author

I agree with almost everything. However, I'm not a big fan of the PASS: and FAIL: noise. I think the icons are clear enough for the common case and we could instead only use the prefixes for the other cases like EXPECTED FAIL and SKIP.

@sindresorhus Without "PASS" and "FAIL", vision-impaired consumers are left to infer semantics from information like "check mark" and "multiplication symbol" (the latter improved a bit after sindresorhus/figures#92 ). I'd prefer to keep the verbosity in support of accessibility and output consistency, but if you still disagree then let me know and I'll remove them.

@novemberborn
Copy link
Member

@gibson042 I should have some time this week to help get this over the line.

@novemberborn
Copy link
Member

I agree with almost everything. However, I'm not a big fan of the PASS: and FAIL: noise. I think the icons are clear enough for the common case and we could instead only use the prefixes for the other cases like EXPECTED FAIL and SKIP.

@sindresorhus Without "PASS" and "FAIL", vision-impaired consumers are left to infer semantics from information like "check mark" and "multiplication symbol" (the latter improved a bit after sindresorhus/figures#92 ). I'd prefer to keep the verbosity in support of accessibility and output consistency, but if you still disagree then let me know and I'll remove them.

@sindresorhus how strongly do you feel about this?

I do like the consistency.

@sindresorhus
Copy link
Member

@sindresorhus how strongly do you feel about this?

I don't feel strongly about it.

@sindresorhus
Copy link
Member

I do like the consistency.

Most assertion will be pass or fail, so there wouldn't be much inconsistency in practice. And it may even have been beneficial to make the uncommon cases more obvious. The result now is also more noisy.

Use lowercase, since AVA doesn't output anything in all caps. Wrap in square brackets to differentiate from the test title.

Remove prefix for passed tests. We assume that most tests pass most of the time.
To better differentiate from the test title.
@novemberborn
Copy link
Member

Project name aside, we don't really use all caps. I've pushed a commit to use lowercase and put it in square brackets.

Most of the time, most tests pass. So I've removed the prefix for passed tests.

I also noticed that the assertion message was only differentiated from the test title by color, so I've made that italic.

@novemberborn novemberborn merged commit 797287a into avajs:main Sep 4, 2022
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test results are inaccessible to red-green color-blind, blind and CI Explain diff gutter symbols
3 participants