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

tools: fix test runner in presence of NODE_REPL_EXTERNAL_MODULE #29956

Merged
merged 1 commit into from
Oct 13, 2019

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Oct 13, 2019

running the repl tests will fail because node will use the system's NODE_REPL_EXTERNAL_MODULE variable. This ensures the built-in repl is used.

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

@devsnek devsnek added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. fast-track PRs that do not need to wait for 48 hours to land. labels Oct 13, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Oct 13, 2019

Collaborators, 👍 here to fast-track.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2019
@devsnek devsnek merged commit ea3d5ff into nodejs:master Oct 13, 2019
@devsnek devsnek deleted the fix/build branch October 13, 2019 21:24
@Trott
Copy link
Member

Trott commented Oct 13, 2019

Fast-tracking requires two approvals but this only received one.

@Trott
Copy link
Member

Trott commented Oct 13, 2019

Fast-tracking requires two approvals but this only received one.

(Also: Totally reasonable to not know all the rules, so I'm kinda "eh, whatever" about this sort of thing. IMO, if we want all our rules followed, we need to either simplify them or automate them--maybe both. I hope to try to get a commit queue initiative restarted at Interactive this year, so hopefully we'll get that automation....)

@devsnek
Copy link
Member Author

devsnek commented Oct 13, 2019

@Trott oh i thought richard counted, since he approved after i added the label. apologies.

@Trott
Copy link
Member

Trott commented Oct 13, 2019

@Trott oh i thought richard counted, since he approved after i added the label. apologies.

Yeah, that stands to reason. Our guide is explicit about a fast-track approval needing to be explicitly approved in addition to approving the change, but I wouldn't be surprised if that often doesn't happen.

targos pushed a commit that referenced this pull request Nov 8, 2019
targos pushed a commit that referenced this pull request Nov 10, 2019
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. fast-track PRs that do not need to wait for 48 hours to land. 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.

4 participants