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: print failed JS/parallel tests #45960

Merged
merged 5 commits into from
Dec 27, 2022

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Dec 24, 2022

This is a small productivity improvement. When running make test, or specifically make test-parallel or make jstest, if there any failed JavaScript (or parallel-run) tests, the error output is printed but the only summary is a line like this:

[02:24|% 100|+ 3110|-   2]: Done

With this PR, in addition to the previous output the test runner will also print a list of the failed tests, like so:

[02:32|% 100|+ 3110|-   2]: Done
Failed tests:
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-this-value-inside-hook-functions.mjs /node/test/parallel/test-loaders-this-value-inside-hook-functions.mjs
out/Release/node --experimental-loader ./test/fixtures/es-module-loaders/loader-unknown-builtin-module.mjs node/test/parallel/test-loaders-unknown-builtin-module.mjs

Again, this is in addition to the current output; the present full output for each test is preserved.

My reasons for adding this:

  • When there are many failing tests, it’s helpful to see a summary of all the failing tests to discern patterns of which groups of tests are failing. This cannot be done in the current output, where each failing test gets something like 10 to 100 lines of output.
  • The summary of commands makes it quick to re-run particular failed tests.

cc @nodejs/testing @nodejs/test_runner @nodejs/python

@GeoffreyBooth GeoffreyBooth added test Issues and PRs related to the tests. python PRs and issues that require attention from people who are familiar with Python. labels Dec 24, 2022
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 24, 2022
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Huzzah!

@aduh95
Copy link
Contributor

aduh95 commented Dec 24, 2022

/Cc @nodejs/testing

@aduh95
Copy link
Contributor

aduh95 commented Dec 24, 2022

Can you add a test failure in this PR, and run a light CI to see it impacts it negatively?

tools/test.py Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
tools/test.py Show resolved Hide resolved
@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Dec 24, 2022

Another tiny tweak that I think would make a huge improvement would be to add an empty line and/or horizontal line between test outputs (as it currently is, they are back to back, each block is nigh indistinguishable from the next).

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member Author

Another tiny tweak that I think would make a huge improvement would be to add an empty line and/or horizontal line between test outputs (as it currently is, they are back to back, each block is nigh indistinguishable from the next).

I just tried it and I think two blank lines are ideal, because many of the tests have single blank lines within them (like after a stack trace, say) but rarely if ever two blank lines, so visually scanning down a lot of results the two blank lines help make the breaks stand out.

@GeoffreyBooth
Copy link
Member Author

Can you add a test failure in this PR, and run a light CI to see it impacts it negatively?

This is a CI run with an intentionally failing test: https://ci.nodejs.org/job/node-test-pull-request/48725/

@nodejs nodejs deleted a comment from nodejs-github-bot Dec 25, 2022
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member Author

@cclauss I’ve added the type hint and responded to all of your suggestions. I believe this resolves your concerns; do you mind please removing your block?

tools/test.py Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth dismissed cclauss’s stale review December 26, 2022 18:37

Made requested changes

@GeoffreyBooth
Copy link
Member Author

@cclauss Please let me know if you have any other notes.

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 26, 2022
@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 Dec 26, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45960
✔  Done loading data for nodejs/node/pull/45960
----------------------------------- PR info ------------------------------------
Title      test: print failed JS/parallel tests (#45960)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GeoffreyBooth:print-failures -> nodejs:main
Labels     test, tools, python, author ready, commit-queue-squash
Commits    5
 - test: print failed JS/parallel tests
 - add type hint
 - Add newline between failed test outputs
 - Match style for variable names
 - Add import for backward compatibility
Committers 1
 - Geoffrey Booth 
PR-URL: https://github.com/nodejs/node/pull/45960
Reviewed-By: Jacob Smith 
Reviewed-By: Christian Clauss 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45960
Reviewed-By: Jacob Smith 
Reviewed-By: Christian Clauss 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 24 Dec 2022 01:50:48 GMT
   ✔  Approvals: 2
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/45960#pullrequestreview-1229549661
   ✔  - Christian Clauss (@cclauss): https://github.com/nodejs/node/pull/45960#pullrequestreview-1230212097
   ✖  Last GitHub CI failed
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3783385909

@GeoffreyBooth GeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit eb4c83f into nodejs:main Dec 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in eb4c83f

@GeoffreyBooth GeoffreyBooth deleted the print-failures branch December 27, 2022 04:03
targos pushed a commit that referenced this pull request Jan 1, 2023
PR-URL: #45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
PR-URL: #45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
PR-URL: #45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
PR-URL: #45960
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants