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

Fix tests (Node.js v18.17.0) #1950

Closed
wants to merge 1 commit into from
Closed

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 8, 2023

Tests of node child processes stopped working on Windows after upgrading to Node.js v18.17.0. The reason turned out to be the fact Jest allows an internally set value of the FORCE_COLOR environment variable to leak through to tests, see jestjs/jest#14391. This results in colored output of the child processes, and so comparisons of the output with expected values fail.

This PR is a quick fix unsetting the variable for the child processes.

@shadowspawn
Copy link
Collaborator

I do not see FORCE_COLOR being used in the Commander code base.

I do see some odd test failures using Node v18.17.0 on Mac, and in the Jest tests being run automatically in the VSCode editor. I'll add a comment in the linked issue.

I don't think is a problem that needs to be solved in Commander though.

@shadowspawn
Copy link
Collaborator

I don't think is a problem that needs to be solved in Commander though.

However, I am glad to know about the issue before stumbling across it myself! 😅

@aweebit
Copy link
Contributor Author

aweebit commented Aug 8, 2023

I don't think is a problem that needs to be solved in Commander though.

Was supposed to be a temporary fix, but I guess I will just use npx jest --worker-threads for now. It does seem to run a little faster on my machine, which is also pretty cool.

@shadowspawn shadowspawn changed the title Fix tests (Node.js v18.17.0, Windows) Fix tests (Node.js v18.17.0) Aug 9, 2023
@aweebit aweebit force-pushed the fix/jest-force-color branch 2 times, most recently from ab92568 to 95a5d50 Compare August 9, 2023 10:02
@aweebit
Copy link
Contributor Author

aweebit commented Aug 9, 2023

Force pushed to make merges less cluttered in case the PR does end up considered useful.

@shadowspawn
Copy link
Collaborator

If the PR GitHub Checks start failing or other developers trip over this then will do something, but currently passing and hopefully gets fixed in Jest.

For readers, temporary developer work-around to run tests is use npx jest --worker-threads or npx jest --runInBand

@shadowspawn
Copy link
Collaborator

I don't like patching the code with detailed work-around for FORCE_COLOR.

But perhaps modifying the Jest call in the run-script to use --worker-threads for now? Has it been well-behaved for you?

(I am currently running at the previous node to avoid the problem!)

@aweebit
Copy link
Contributor Author

aweebit commented Aug 20, 2023

But perhaps modifying the Jest call in the run-script to use --worker-threads for now? Has it been well-behaved for you?

For me, it has, but the feature is still experimental, so still not sure it should be relied on in general.

@shadowspawn
Copy link
Collaborator

Would be temporary, but good point, no need to use experimental. Tests with --runInBand take 10 s on my computer, which is fine.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 20, 2023

Would be temporary, but good point, no need to use experimental. Tests with --runInBand take 10 s on my computer, which is fine.

It's 50 vs 20 seconds on mine 🤨

@shadowspawn
Copy link
Collaborator

Closing this in favour of #2011 which has a simpler work-around (with @aweebit added as co-author)

Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants