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

lib: adds colors to X's and .'s in test_runner dot reporter #52278

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Aidan7757
Copy link

Refs: #51770

First time contributing - if there are any procedural or code changes that need to be made let me know. Thanks!

Aidan Chadha

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner labels Mar 30, 2024
@MoLow
Copy link
Member

MoLow commented Mar 31, 2024

Please fix the commit message, and update snapshots (NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs)

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Good start, see Moshe's comments on how to proceed, thanks 🙏

@Aidan7757
Copy link
Author

Hi, I'm getting a zsh: Permission Denied when attempting to update the snapshots.

@MoLow
Copy link
Member

MoLow commented Mar 31, 2024

Hi, I'm getting a zsh: Permission Denied when attempting to update the snapshots.

perhaps you checked out the repo with another user/with sudo? you might need to chown/chmod

@Aidan7757
Copy link
Author

Just reworked the commit message and ran the command given after being able to get access. I'm unsure of the purpose of snapshots - it ended up returning errors that were contained in a file I had no touched and I'm unsure of what to do with the statements returned. Any advice would be appreciated, thanks for all the help!

Errors returned from snapshot:
node/test/parallel/test-runner-output.mjs: line 1: import: command not found
node/test/parallel/test-runner-output.mjs: line 2: import: command not found
node/test/parallel/test-runner-output.mjs: line 3: import: command not found
node/test/parallel/test-runner-output.mjs: line 4: import: command not found
node/test/parallel/test-runner-output.mjs: line 5: import: command not found
node/test/parallel/test-runner-output.mjs: line 7: const: command not found
node/test/parallel/test-runner-output.mjs: line 8: process.config.variables.icu_gyp_path: command not found
node/test/parallel/test-runner-output.mjs: line 9: process.config.variables.node_shared_openssl: command not found
node/test/parallel/test-runner-output.mjs: line 11: syntax error near unexpected token str' node/test/parallel/test-runner-output.mjs: line 11: function replaceTestDuration(str) {'

Thanks!

@MoLow
Copy link
Member

MoLow commented Apr 1, 2024

Snapshot tests are in place to validate that the output of test runner reporters stays the same across commits.
do you receive these errors when running the same command on the main branch?

@Aidan7757
Copy link
Author

Yes, the error persists on the main branch.

@atlowChemi
Copy link
Member

Just reworked the commit message and ran the command given after being able to get access

Awesome!
Please note the susystem should be test_runner, not lib 🙂

I'm unsure of the purpose of snapshots

As Moshe explained, snapshots are files containing the expected output for a test. We use these to ensure the reporters work as expected as well as other things, and as this PR proposes a change to the dot reporters output, the snapshots require an update.

I'm unsure of what to do with the statements returned

Does rebuilding the Nodejs executable work? (make -j4)
Does running the tests without the NODE_REGENERATE_SNAPSHOTS env var work?

@Aidan7757
Copy link
Author

Calling make -j4 returns an error of:
make: *** [config.gypi] Error 1
and calling the tests without the environment variable returns the same errors.

Sorry for all of the issues! I appreciate all of the aid given and hopefully theres an easy solution hidden in plain sight that I'm just missing, I'll keep digging for it.

@atlowChemi
Copy link
Member

Calling make -j4 returns an error of:
make: *** [config.gypi] Error 1

Could you try pasting the entire output, or the error throughout the process?
It is kind of impossible to know what did not work just based on the exit code 🙂

@Aidan7757
Copy link
Author

Fortunately the make -j4 appears to have fixed the issue and the
NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs now returns:
ℹ tests 42 ℹ suites 1 ℹ pass 42 ℹ fail 0 ℹ cancelled 0 ℹ skipped 0 ℹ todo 0 ℹ duration_ms 1411.179291

Does this mean that the code is able to be submitted? There are still a few checks that appear to be failing within the pull request.

@MoLow
Copy link
Member

MoLow commented Apr 2, 2024

you should commit all changed snapshot files that are changed by NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants