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.

Sign up for GitHub

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

Jump to bottom

cli: allow --test-[name/skip]-pattern in NODE_OPTIONS #53001

Merged
merged 1 commit into from
Aug 16, 2024
Merged

cli: allow --test-[name/skip]-pattern in NODE_OPTIONS #53001

merged 1 commit into from
Aug 16, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented May 15, 2024 •
edited
Loading

workaround for #51384

NODE_OPTIONS="--test-name-pattern=XXX" node my_test.js

Allows --test-name-pattern and --test-skip-pattern to appear in the NODE_OPTIONS environment variable.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 15, 2024
@RedYetiDev RedYetiDev added cli Issues and PRs related to the Node.js command line interface. test_runner Issues and PRs related to the test runner subsystem. labels May 15, 2024
@cjihrig
Copy link
Contributor

cjihrig commented May 15, 2024

Just an FYI - this does not fix #51384.

@RedYetiDev
Copy link
Member Author

Just an FYI - this does not fix #51384.

Why? If these options can be passed as env variables, then they won't be hostile to NPM scripts, as they can be used in the variable? Right?

@cjihrig
Copy link
Contributor

cjihrig commented May 15, 2024

The ask in that issue is to be able to run something like npm test -- --test-name-pattern="my pattern". More generally (based on feedback I've received), people want to be able to mix test runner arguments in process.execArgv and process.argv.

I'm not saying this is a bad change, but this is not what is being requested and I don't think people will be satisfied with it for that use case.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 13, 2024

@RedYetiDev are you planning to pursue this?

@RedYetiDev
Copy link
Member Author

Sure, I didn't realize there were conflicts. I'll fix them now :-)

@RedYetiDev
Copy link
Member Author

Conflicts resolved

cjihrig
cjihrig approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the CI passes.

@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61089/

@RedYetiDev
Copy link
Member Author

CI is passing except for unrelated failures :-)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61150/

@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 16, 2024
@nodejs-github-bot nodejs-github-bot merged commit 59da1df into nodejs:main Aug 16, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 59da1df

RafaelGSS pushed a commit that referenced this pull request Aug 19, 2024
@RedYetiDev @RafaelGSS
cli: allow --test-[name/skip]-pattern in NODE_OPTIONS
c3b1a43 
PR-URL: #53001
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 21, 2024
@RedYetiDev @RafaelGSS
cli: allow --test-[name/skip]-pattern in NODE_OPTIONS
1ca598c 
PR-URL: #53001
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RedYetiDev RedYetiDev mentioned this pull request Aug 27, 2024
Open
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MoLow MoLow MoLow approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RedYetiDev @cjihrig @nodejs-github-bot @MoLow @targos