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

test_runner: hide failing tests title when all tests pass #47370

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Apr 2, 2023

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 2, 2023
@ljharb
Copy link
Member

ljharb commented Apr 2, 2023

Won’t this make it harder for tooling to parse out the lack of failures?

@aduh95
Copy link
Contributor

aduh95 commented Apr 2, 2023

Can you please rebase to remove the additional commit?

@MoLow MoLow force-pushed the spec-show-failing-only-when-failing branch from e93177c to 3638be8 Compare April 2, 2023 07:35
@MoLow
Copy link
Member Author

MoLow commented Apr 2, 2023

Won’t this make it harder for tooling to parse out the lack of failures?

they can do that either by counting test:fail events or by checking the exit code

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Don't we have tests for this?

lib/internal/test_runner/reporter/spec.js Outdated Show resolved Hide resolved
@MoLow MoLow force-pushed the spec-show-failing-only-when-failing branch 2 times, most recently from b5b2435 to 9fd2137 Compare April 2, 2023 11:54
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

@GeoffreyBooth
Copy link
Member

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

@MoLow
Copy link
Member Author

MoLow commented Apr 2, 2023

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

We would need better snapshot/comparison functionality (perhaps as part of node:assert?) in order to do that. I do desire to re-introduce assert.snapshot once I have time, but the lack of that ability should not block the introduction of other capabilities and features just because our testing tools are not cutting edge.

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

Sorry, I thought the code is self-explanatory. we added a list of failing tests at the end of the report, but if all tests pass there is still a "failing tests" title. that title is what the commit message is referring to. if you have a better suggestion for the commit message I am ok with changing it

@MoLow MoLow force-pushed the spec-show-failing-only-when-failing branch from 9fd2137 to 87a38b5 Compare April 2, 2023 16:44
@MoLow MoLow changed the title test_runner: hide failing tests when all tests pass test_runner: hide failing tests title when all tests pass Apr 2, 2023
@benjamingr
Copy link
Member

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

Open an issue to track that probably and suggest it as a bigger policy change?

@GeoffreyBooth
Copy link
Member

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

We would need better snapshot/comparison functionality (perhaps as part of node:assert?) in order to do that. I do desire to re-introduce assert.snapshot once I have time, but the lack of that ability should not block the introduction of other capabilities and features just because our testing tools are not cutting edge.

We can do it today via assert.match. See #41352, where I moved all the ESM tests out of test/message into test/es-module. You could follow that same pattern.

As for larger policy change, sure we can discuss it, but in the meantime why make the problem worse? As far as I know only the test runner team is adding new tests into test/message lately, so if those folks find a pattern that works for them that stops adding to test/message, that would be future-proof.

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

Sorry, I thought the code is self-explanatory. we added a list of failing tests at the end of the report, but if all tests pass there is still a “failing tests” title. that title is what the commit message is referring to. if you have a better suggestion for the commit message I am ok with changing it

Maybe just add a before/after of the output that’s changing in the PR description? I looked in the PR diff expecting to see a test assertion changing, now that the “Test failed” label was removed, and was surprised not to find one.

@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 2, 2023
@nodejs-github-bot
Copy link
Collaborator

@MoLow
Copy link
Member Author

MoLow commented Apr 2, 2023

We can do it today via assert.match. See #41352, where I moved all the ESM tests out of test/message into test/es-module. You could follow that same pattern.

this pattern is not feasible for the test runner tests, since the output of the test runner is an interface consumed by node users, we try to test all of it - which makes the amount of output we test significantly larger than other areas under tests/messsage.
testing all the possible combinations of output generated by the test runner using regex is not something maintainable AFAIK.
I am in favor of migrating away from tools/test.py but we need a better alternative than assert.match.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Apr 2, 2023

this pattern is not feasible for the test runner tests, since the output of the test runner is an interface consumed by node users, we try to test all of it - which makes the amount of output we test significantly larger than other areas under tests/messsage.

I think this is equivalent?

import { mustCall } from '../common/index.mjs';
import { match, strictEqual } from 'node:assert';
import { spawn } from 'node:child_process';
import { execPath } from 'node:process';

const source = `import { it } from 'node:test';
it('should pass', () => {});`;

const expectedOutput = `* should pass *(*ms)*
*ℹ tests 1*
*ℹ suites 0*
*ℹ pass 1*
*ℹ fail 0*
*ℹ cancelled 0*
*ℹ skipped 0*
*ℹ todo 0*
*ℹ duration_ms *
`;

const child = spawn(execPath, [
  '--no-warnings',
  '--test-reporter=spec',
  `--input-type=module`,
  '--eval',
  source,
]);

let stdout = '';
let stderr = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
  stdout += data;
});
child.stderr.setEncoding('utf8');
child.stderr.on('data', (data) => {
  stderr += data;
});
child.on('close', mustCall((code, _signal) => {
  strictEqual(code, 0);
  strictEqual(stderr, '');

  const expectedLines = expectedOutput.split('\n');
  stdout.split('\n').forEach((line, lineIndex) => {
    const expectedLine = expectedLines[lineIndex];
    match(line, new RegExp(expectedLine.replaceAll('*', '.*')));
  });
}));

There’s a lot more boilerplate here, but it could be simplified with some helpers added to test/common; and if you don’t like putting the source code in a string you could save it as a fixture. And of course it could be slightly faster if the expected output were written as a regex in the first place (or at least with .* instead of *) but I wanted to match the .out file in this PR.

@benjamingr
Copy link
Member

As for larger policy change, sure we can discuss it, but in the meantime why make the problem worse? As far as I know only the test runner team is adding new tests into test/message lately, so if those folks find a pattern that works for them that stops adding to test/message, that would be future-proof.

There is an existing process/prior art which this PR uses. The burden of doing refactors should not be on individual PR authors and if you want to drive a bigger policy change I think the better place to discuss it is not on an individual PR.

You have been on the other side of people leaving comments on PRs asking for people to do extra (good, important) work not necessarily related the PR before. Giving you your (good) advice back - doing the technical bits and driving the effort goes a long way and is generally a lot better received than "don't do X" comments when there is prior art of X. You are of course welcome to interact in any positive way you see fit - I'm just trying to explain my perspective on what works well with these sort of communications.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Apr 3, 2023

Il try putting some time into developing an internal tool we can use for this, and hopefully, one day expose it

Thanks. I guess for now my question is, what about the approach in this PR (the message test) do you prefer over the alternative I proposed above? As far as I can tell the alternative tests exactly the same output, as completely as the .out test does, and is just as maintainable because the expected output string is exactly the same. You could put the source input and the expected output in separate files and import them if that makes things more comprehensible, but I put it all together for the purposes of an example.

Update: on my machine, the alternative proposal ran 3x faster than the message version:

hyperfine --warmup 3 --runs 10 \
  'tools/test.py --mode=release message/test_runner_spec_reporter_successful.js' \
  './node ./test/parallel/test-runner-spec-reporter-successful.mjs'

Benchmark 1: tools/test.py --mode=release message/test_runner_spec_reporter_successful.js
  Time (mean ± σ):     973.9 ms ±  18.4 ms    [User: 560.7 ms, System: 152.3 ms]
  Range (min … max):   951.3 ms … 994.4 ms    10 runs

Benchmark 2: ./node ./test/parallel/test-runner-spec-reporter-successful.mjs
  Time (mean ± σ):     278.8 ms ±   4.1 ms    [User: 229.0 ms, System: 48.5 ms]
  Range (min … max):   272.3 ms … 288.6 ms    10 runs

Summary
  './node ./test/parallel/test-runner-spec-reporter-successful.mjs' ran
    3.49 ± 0.08 times faster than 'tools/test.py --mode=release message/test_runner_spec_reporter_successful.js'

And of course it should run faster still if it wasn’t generating the regexes dynamically, but that’s a performance/readability tradeoff question.

@MoLow
Copy link
Member Author

MoLow commented Apr 3, 2023

As far as I can tell the alternative tests exactly the same output, as completely as the .out test does, and is just as maintainable because the expected output string is exactly the same. You could put the source input and the expected output in separate files and import them if that makes things more comprehensible, but I put it all together for the purposes of an example.

just off the top of my head, the process of producing the .out file is currently straightforward (./node test.js > test.out), but in your suggestion, it is not.
I am not saying it is impossible or hard, it is just not at the level for use in this PR, and even if it was I would prefer it to be in its own PR

@GeoffreyBooth
Copy link
Member

just off the top of my head, the process of producing the .out file is currently straightforward (./node test.js > test.out)

Except for the fact that you need to go through the generated .out file and replace a bunch of things with asterisks?

And of course you could still use the .out file, generated however you’re doing it currently, and replace my const expectedOutput = line above with const expectedOutput = fs.readFileSync('./test_runner_spec_reporter_successful.out').

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47370
✔  Done loading data for nodejs/node/pull/47370
----------------------------------- PR info ------------------------------------
Title      test_runner: hide failing tests title when all tests pass (#47370)
Author     Moshe Atlow  (@MoLow)
Branch     MoLow:spec-show-failing-only-when-failing -> nodejs:main
Labels     author ready, needs-ci, dont-land-on-v14.x, test_runner
Commits    1
 - test_runner: hide failing tests title when all tests pass
Committers 1
 - Moshe Atlow 
PR-URL: https://github.com/nodejs/node/pull/47370
Reviewed-By: Colin Ihrig 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47370
Reviewed-By: Colin Ihrig 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test_runner: hide failing tests title when all tests pass
   ℹ  This PR was created on Sun, 02 Apr 2023 07:14:07 GMT
   ✔  Approvals: 3
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368114083
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368115172
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368130734
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-04-04T04:44:06Z: https://ci.nodejs.org/job/node-test-pull-request/50898/
- Querying data for job/node-test-pull-request/50898/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4605193841

PR-URL: nodejs#47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MoLow MoLow force-pushed the spec-show-failing-only-when-failing branch from ba7f953 to af8ed02 Compare April 4, 2023 07:40
@MoLow
Copy link
Member Author

MoLow commented Apr 4, 2023

Landed in af8ed02

@MoLow MoLow merged commit af8ed02 into nodejs:main Apr 4, 2023
@MoLow MoLow deleted the spec-show-failing-only-when-failing branch April 4, 2023 07:40
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47370
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants