-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: fix test when NODE_OPTIONS env var is set to --trace-warnings #20027
Conversation
@nodejs/testing @nodejs/build |
An argument could be made to also clear out the other environment variables. Or maybe allow them to be set but print out a warning that test results may be affected. |
@richardlau hm, how about we do both, warn them that test results may get affected if env variables are set. use |
Sounds quite complicated, I'd just unset this one by default for now, and do other things as we need to. If there are reasons to have NODE_OPTIONS set when running the test, we could just introduce an alternative variable like If you want to unset all of them I guess that's fine, but leaving it until someone runs into a problem seems reasonable too. |
@gibfahn yep, complicated. i guess we can land this pr as is then? and if we run into some problem in future we can come with some other solution |
Landed in 5af28c2 🎉 |
PR-URL: nodejs#20027 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #20027 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #20027 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #20027 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
it fixes the issue of tests failing if the shell has
export NODE_OPTIONS='--trace-warnings'
setChecklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes