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: fix test when NODE_OPTIONS env var is set to --trace-warnings #20027

Closed
wants to merge 1 commit into from

Conversation

aks-
Copy link
Member

@aks- aks- commented Apr 14, 2018

it fixes the issue of tests failing if the shell has export NODE_OPTIONS='--trace-warnings' set

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Apr 14, 2018
@Trott
Copy link
Member

Trott commented Apr 15, 2018

@nodejs/testing @nodejs/build

@richardlau
Copy link
Member

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.

@aks-
Copy link
Member Author

aks- commented Apr 16, 2018

@richardlau hm, how about we do both, warn them that test results may get affected if env variables are set. use --unset-env-variables flag to unset all the variables that might affect test o/p?

@gibfahn
Copy link
Member

gibfahn commented Apr 16, 2018

@richardlau hm, how about we do both, warn them that test results may get affected if env variables are set. use --unset-env-variables flag to unset all the variables that might affect test o/p?

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 NODE_TEST_OPTIONS, but again I wouldn't do that unless it's necessary.

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.

@aks-
Copy link
Member Author

aks- commented Apr 17, 2018

@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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 22, 2018
@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

Landed in 5af28c2 🎉

@BridgeAR BridgeAR closed this Apr 23, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 23, 2018
PR-URL: nodejs#20027
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
PR-URL: #20027
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 30, 2018
PR-URL: #20027
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
PR-URL: #20027
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@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. 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.

8 participants