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

Jump to bottom

Improve test result accessibility #3082

Merged
merged 14 commits into from Sep 4, 2022
Merged

Improve test result accessibility #3082

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6e1dbb9
Explain diff gutter symbols
gibson042 Aug 6, 2022
1c92630
Add test result labels
gibson042 Aug 6, 2022
93070fe
Bold the source line of errors in code excerpts
gibson042 Aug 6, 2022
ef19867
DRY out writeTestSummary
gibson042 Aug 6, 2022
017a45a
Better propagate color configuration
gibson042 Aug 7, 2022
49fbe24
TAP reporter: Strip ANSI control sequences from error labels
gibson042 Aug 7, 2022
f1beca2
Update tests
gibson042 Aug 7, 2022
1b64bff
Set chalk options automatically from options.cjs `set`
gibson042 Aug 7, 2022
36d764c
Merge remote-tracking branch 'origin/main' into gh-2919-test-result-a…
novemberborn Aug 26, 2022
9b45dc3
Undo changes to lib/chalk.js
novemberborn Aug 27, 2022
fcc1f07
Keep relying on Chalk to parse --color argv
novemberborn Aug 27, 2022
0c0fd21
Tweak result prefixes
novemberborn Sep 4, 2022
ee9f000
Print assertion error in italics
novemberborn Sep 4, 2022
2449d74
Update snapshot
novemberborn Sep 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/assert.js
View file
Open in desktop
Expand Up @@ -8,8 +8,10 @@ import {SnapshotError, VersionMismatchError} from './snapshot-manager.js';

function formatDescriptorDiff(actualDescriptor, expectedDescriptor, options) {
options = {...options, ...concordanceOptions};
const {diffGutters} = options.theme;
const {insertLine, deleteLine} = options.theme.string.diff;
return {
label: 'Difference:',
label: `Difference (${diffGutters.actual}${deleteLine.open}actual${deleteLine.close}, ${diffGutters.expected}${insertLine.open}expected${insertLine.close}):`,
formatted: concordance.diffDescriptors(actualDescriptor, expectedDescriptor, options),
};
}
Expand Down
8 changes: 8 additions & 0 deletions lib/chalk.js
View file
Open in desktop
@@ -1,15 +1,23 @@
import {EventEmitter} from 'node:events';

import {Chalk} from 'chalk'; // eslint-disable-line unicorn/import-style

let chalk = new Chalk(); // eslint-disable-line import/no-mutable-exports

export {chalk};

let configured = false;
const events = new EventEmitter();
const on = events.on.bind(events);
const emit = events.emit.bind(events);
export function set(options) {
if (configured) {
throw new Error('Chalk has already been configured');
}

configured = true;
chalk = new Chalk(options);
emit('set', chalk);
}

export {on};
2 changes: 1 addition & 1 deletion lib/cli.js
View file
Open in desktop
Expand Up @@ -223,7 +223,7 @@ export default async function loadCli() { // eslint-disable-line complexity

const combined = {...conf};

for (const flag of Object.keys(FLAGS)) {
for (const flag of ['color', ...Object.keys(FLAGS)]) {
novemberborn marked this conversation as resolved.
Show resolved Hide resolved
if (flag === 'no-worker-threads' && Reflect.has(argv, 'worker-threads')) {
combined.workerThreads = argv['worker-threads'];
continue;
Expand Down
2 changes: 1 addition & 1 deletion lib/code-excerpt.js
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ export default function exceptCode(source, options = {}) {
const coloredLineNumber = isErrorSource ? lineNumber : chalk.grey(lineNumber);
const result = ` ${coloredLineNumber} ${item.value.padEnd(extendedWidth)}`;

return isErrorSource ? chalk.bgRed(result) : result;
return isErrorSource ? chalk.bgRed.bold(result) : result;
})
.join('\n');
}
14 changes: 9 additions & 5 deletions lib/concordance-options.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import ansiStyles from 'ansi-styles';
import {Chalk} from 'chalk'; // eslint-disable-line unicorn/import-style
import stripAnsi from 'strip-ansi';

import {chalk} from './chalk.js';
import {chalk, on as onChalkEvent} from './chalk.js';

const forceColor = new Chalk({level: Math.max(chalk.level, 1)});

Expand Down Expand Up @@ -85,17 +85,21 @@ const colorTheme = {
undefined: ansiStyles.yellow,
};

const plainTheme = JSON.parse(JSON.stringify(colorTheme), value => typeof value === 'string' ? stripAnsi(value) : value);

const theme = chalk.level > 0 ? colorTheme : plainTheme;
const plainTheme = JSON.parse(JSON.stringify(colorTheme), (_name, value) => typeof value === 'string' ? stripAnsi(value) : value);
novemberborn marked this conversation as resolved.
Show resolved Hide resolved

const concordanceOptions = {
// Use Node's object inspection depth, clamped to a minimum of 3
get maxDepth() {
return Math.max(3, inspect.defaultOptions.depth);
},
theme,
theme: undefined,
};
function setTheme(chalk) {
concordanceOptions.theme = chalk.level > 0 ? colorTheme : plainTheme;
}

setTheme(chalk);
onChalkEvent('set', setTheme);

export default concordanceOptions;

Expand Down
27 changes: 21 additions & 6 deletions lib/reporters/default.js
View file
Open in desktop
Expand Up @@ -244,9 +244,9 @@ export default class Reporter {

case 'selected-test': {
if (event.skip) {
this.lineWriter.writeLine(colors.skip(`- ${this.prefixTitle(event.testFile, event.title)}`));
this.lineWriter.writeLine(colors.skip(`- SKIP: ${this.prefixTitle(event.testFile, event.title)}`));
} else if (event.todo) {
this.lineWriter.writeLine(colors.todo(`- ${this.prefixTitle(event.testFile, event.title)}`));
this.lineWriter.writeLine(colors.todo(`- TODO: ${this.prefixTitle(event.testFile, event.title)}`));
}

break;
Expand Down Expand Up @@ -504,15 +504,30 @@ export default class Reporter {
}

writeTestSummary(event) {
// Prefix icon indicates matched expectations vs. not.
// Prefix color indicates passed-as-expected vs. not (fail or unexpected pass).
// This yields four possibilities, which in the standard configuration render as:
// * normal test, pass: <green>✔ PASS:</green>
// * normal test, fail: <red>✘ FAIL:</red>
// * fail-expected test, fail: <red>✔ EXPECTED FAIL:</red>
// * fail-expected test, pass: <red>✘ UNEXPECTED PASS:</red>
let prefix;
let suffix;
if (event.type === 'hook-failed' || event.type === 'test-failed') {
this.write(`${colors.error(figures.cross)} ${this.prefixTitle(event.testFile, event.title)} ${colors.error(event.err.message)}`);
const type = event.knownFailing ? 'UNEXPECTED PASS' : 'FAIL';
prefix = colors.error(`${figures.cross} ${type}:`);
suffix = colors.error(event.err.message);
} else if (event.knownFailing) {
this.write(`${colors.error(figures.tick)} ${colors.error(this.prefixTitle(event.testFile, event.title))}`);
prefix = colors.error(figures.tick + ' EXPECTED FAIL:');
} else {
const duration = event.duration > this.durationThreshold ? colors.duration(' (' + prettyMs(event.duration) + ')') : '';
this.write(`${colors.pass(figures.tick)} ${this.prefixTitle(event.testFile, event.title)}${duration}`);
prefix = colors.pass(figures.tick + ' PASS:');
if (event.duration > this.durationThreshold) {
suffix = colors.duration(`(${prettyMs(event.duration)})`);
}
}

const label = this.prefixTitle(event.testFile, event.title);
this.write(`${prefix} ${label}${suffix ? ' ' + suffix : ''}`);
this.writeLogs(event);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/tap.js
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ function dumpError(error) {
}

if (error.values.length > 0) {
object.values = Object.fromEntries(error.values.map(({label, formatted}) => [label, stripAnsi(formatted)]));
object.values = Object.fromEntries(error.values.map(({label, formatted}) => [stripAnsi(label), stripAnsi(formatted)]));
}
}

Expand Down
2 changes: 0 additions & 2 deletions lib/worker/base.js
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@ import {workerData} from 'node:worker_threads';

import setUpCurrentlyUnhandled from 'currently-unhandled';

import {set as setChalk} from '../chalk.js';
import nowAndTimers from '../now-and-timers.cjs';
import providerManager from '../provider-manager.js';
import Runner from '../runner.js';
Expand All @@ -22,7 +21,6 @@ const currentlyUnhandled = setUpCurrentlyUnhandled();

const run = async options => {
setOptions(options);
setChalk(options.chalkOptions);

if (options.chalkOptions.level > 0) {
const {stdout, stderr} = process;
Expand Down
13 changes: 13 additions & 0 deletions lib/worker/options.cjs
View file
Open in desktop
@@ -1,4 +1,10 @@
'use strict';
const chalk = import('../chalk.js'); // eslint-disable-line node/no-unsupported-features/es-syntax
let setChalk;
chalk.then(chalk => {
setChalk = chalk.set;
});

let options = null;
exports.get = () => {
if (!options) {
Expand All @@ -14,4 +20,11 @@ exports.set = newOptions => {
}

options = newOptions;
if (options.chalkOptions) {
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.

}
};
26 changes: 13 additions & 13 deletions test-tap/assert.js
View file
Open in desktop
Expand Up @@ -5,10 +5,10 @@ import stripAnsi from 'strip-ansi';
import {test} from 'tap';

import * as assert from '../lib/assert.js';
import {set as setChalk} from '../lib/chalk.js';
import * as snapshotManager from '../lib/snapshot-manager.js';
import {set as setOptions} from '../lib/worker/options.cjs';

setOptions({chalkOptions: {level: 0}});
setChalk({level: 0});

let lastFailure = null;
let lastPassed = false;
Expand Down Expand Up @@ -279,7 +279,7 @@ test('.is()', t => {
message: '',
raw: {actual: 'foo', expected: 'bar'},
values: [
{label: 'Difference:', formatted: /- 'foo'\n\+ 'bar'/},
{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 'bar'/},
],
});

Expand All @@ -289,31 +289,31 @@ test('.is()', t => {
expected: 42,
message: '',
values: [
{label: 'Difference:', formatted: /- 'foo'\n\+ 42/},
{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 42/},
],
});

failsWith(t, () => assertions.is('foo', 42, 'my message'), {
assertion: 'is',
message: 'my message',
values: [
{label: 'Difference:', formatted: /- 'foo'\n\+ 42/},
{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 42/},
],
});

failsWith(t, () => assertions.is(0, -0, 'my message'), {
assertion: 'is',
message: 'my message',
values: [
{label: 'Difference:', formatted: /- 0\n\+ -0/},
{label: 'Difference (- actual, + expected):', formatted: /- 0\n\+ -0/},
],
});

failsWith(t, () => assertions.is(-0, 0, 'my message'), {
assertion: 'is',
message: 'my message',
values: [
{label: 'Difference:', formatted: /- -0\n\+ 0/},
{label: 'Difference (- actual, + expected):', formatted: /- -0\n\+ 0/},
],
});

Expand Down Expand Up @@ -535,20 +535,20 @@ test('.deepEqual()', t => {
assertion: 'deepEqual',
message: '',
raw: {actual: 'foo', expected: 'bar'},
values: [{label: 'Difference:', formatted: /- 'foo'\n\+ 'bar'/}],
values: [{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 'bar'/}],
});

failsWith(t, () => assertions.deepEqual('foo', 42), {
assertion: 'deepEqual',
message: '',
raw: {actual: 'foo', expected: 42},
values: [{label: 'Difference:', formatted: /- 'foo'\n\+ 42/}],
values: [{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 42/}],
});

failsWith(t, () => assertions.deepEqual('foo', 42, 'my message'), {
assertion: 'deepEqual',
message: 'my message',
values: [{label: 'Difference:', formatted: /- 'foo'\n\+ 42/}],
values: [{label: 'Difference (- actual, + expected):', formatted: /- 'foo'\n\+ 42/}],
});

failsWith(t, () => assertions.deepEqual({}, {}, null), {
Expand Down Expand Up @@ -758,7 +758,7 @@ test('.like()', t => {
failsWith(t, () => assertions.like({a: 'foo', b: 'irrelevant'}, {a: 'bar'}), {
assertion: 'like',
message: '',
values: [{label: 'Difference:', formatted: /{\n-\s*a: 'foo',\n\+\s*a: 'bar',\n\s*}/}],
values: [{label: 'Difference (- actual, + expected):', formatted: /{\n-\s*a: 'foo',\n\+\s*a: 'bar',\n\s*}/}],
});

t.end();
Expand Down Expand Up @@ -1429,7 +1429,7 @@ test('.snapshot()', async t => {
failsWith(t, () => assertions.snapshot({foo: 'not bar'}), {
assertion: 'snapshot',
message: 'Did not match snapshot',
values: [{label: 'Difference:', formatted: ' {\n- foo: \'not bar\',\n+ foo: \'bar\',\n }'}],
values: [{label: 'Difference (- actual, + expected):', formatted: ' {\n- foo: \'not bar\',\n+ foo: \'bar\',\n }'}],
});
}

Expand All @@ -1442,7 +1442,7 @@ test('.snapshot()', async t => {
failsWith(t, () => assertions.snapshot({foo: 'not bar'}, 'my message'), {
assertion: 'snapshot',
message: 'my message',
values: [{label: 'Difference:', formatted: ' {\n- foo: \'not bar\',\n+ foo: \'bar\',\n }'}],
values: [{label: 'Difference (- actual, + expected):', formatted: ' {\n- foo: \'not bar\',\n+ foo: \'bar\',\n }'}],
});
}

Expand Down
6 changes: 3 additions & 3 deletions test-tap/code-excerpt.js
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ test('read code excerpt', t => {
const excerpt = codeExcerpt({file, line: 2, isWithinProject: true, isDependency: false});
const expected = [
` ${chalk.grey('1:')} function a() {`,
chalk.bgRed(' 2: alert(); '),
chalk.bgRed.bold(' 2: alert(); '),
` ${chalk.grey('3:')} } `,
].join('\n');

Expand All @@ -40,7 +40,7 @@ test('truncate lines', t => {
const excerpt = codeExcerpt({file, line: 2, isWithinProject: true, isDependency: false}, {maxWidth: 14});
const expected = [
` ${chalk.grey('1:')} functio…`,
chalk.bgRed(' 2: alert…'),
chalk.bgRed.bold(' 2: alert…'),
` ${chalk.grey('3:')} } `,
].join('\n');

Expand All @@ -66,7 +66,7 @@ test('format line numbers', t => {
const excerpt = codeExcerpt({file, line: 10, isWithinProject: true, isDependency: false});
const expected = [
` ${chalk.grey(' 9:')} function a() {`,
chalk.bgRed(' 10: alert(); '),
chalk.bgRed.bold(' 10: alert(); '),
` ${chalk.grey('11:')} } `,
].join('\n');

Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/default.edgecases.v14.log
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@
import-and-use-test-member.cjs:3

2:
 3: test('pass', t => t.pass());
 3: test('pass', t => t.pass());
4:

TypeError: test is not a function
Expand All @@ -49,7 +49,7 @@

throws.cjs:1

 1: throw new Error('throws');
 1: throw new Error('throws');
2:

Error: throws
Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/default.edgecases.v16.log
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@
import-and-use-test-member.cjs:3

2:
 3: test('pass', t => t.pass());
 3: test('pass', t => t.pass());
4:

TypeError: test is not a function
Expand All @@ -49,7 +49,7 @@

throws.cjs:1

 1: throw new Error('throws');
 1: throw new Error('throws');
2:

Error: throws
Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/default.edgecases.v18.log
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@
import-and-use-test-member.cjs:3

2:
 3: test('pass', t => t.pass());
 3: test('pass', t => t.pass());
4:

TypeError: test is not a function
Expand All @@ -49,7 +49,7 @@

throws.cjs:1

 1: throw new Error('throws');
 1: throw new Error('throws');
2:

Error: throws
Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/default.failfast.v14.log
View file
Open in desktop
@@ -1,6 +1,6 @@

---tty-stream-chunk-separator
✖ a › fails Test failed via `t.fail()`
✖ FAIL: a › fails Test failed via `t.fail()`
---tty-stream-chunk-separator
─

Expand All @@ -9,7 +9,7 @@
a.cjs:3

2:
 3: test('fails', t => t.fail());
 3: test('fails', t => t.fail());
4:

Test failed via `t.fail()`
Expand Down