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

test_runner: warn if only is used without properly enabling only tests #46448

Closed
cjihrig opened this issue Jan 31, 2023 · 3 comments · Fixed by #46540
Closed

test_runner: warn if only is used without properly enabling only tests #46448

cjihrig opened this issue Jan 31, 2023 · 3 comments · Fixed by #46540
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Jan 31, 2023

What is the problem this feature will solve?

only tests require enabling only mode, otherwise only does nothing. This can lead people to think that only does not work.

What is the feature you are proposing to solve the problem?

The test runner should warn if someone tries to use only without properly enabling it.

What alternatives have you considered?

Doing nothing.
@cjihrig cjihrig added feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem. labels Jan 31, 2023
@richiemccoll
Copy link
Contributor

richiemccoll commented Feb 6, 2023 •
edited
Loading

@cjihrig I can take a look at this one, if nobody else has picked it up.

  • Should this warning be a test diagnostic? or use a different mechanism for warnings?
  • Would this be emitted once per parent test, once per test file or something different?

@MoLow
Copy link
Member

MoLow commented Feb 6, 2023

@cjihrig I can take a look at this one, if nobody else has picked it up.

  • Should this warning be a test diagnostic? or use a different mechanism for warnings?
  • Would this be emitted once per parent test, once per test file or something different?

I think either process.emitWarning, or simply test.diagnostics is ok

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 7, 2023

I think we should prefer a test diagnostic.

@richiemccoll richiemccoll mentioned this issue Feb 7, 2023
Merged
nodejs-github-bot pushed a commit that referenced this issue Feb 21, 2023
@richiemccoll
test_runner: emit test-only diagnostic warning
c90ea93 
PR-URL: #46540
Fixes: #46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
3983267 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
0c8f40d 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
@richiemccoll @MoLow
test_runner: emit test-only diagnostic warning
b50cfb3 
PR-URL: nodejs#46540
Fixes: nodejs#46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
@richiemccoll @juanarbol
test_runner: emit test-only diagnostic warning
94c870b 
PR-URL: #46540
Backport-PR-URL: #46839
Fixes: #46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
@richiemccoll @juanarbol
test_runner: emit test-only diagnostic warning
cd3aaa8 
PR-URL: #46540
Backport-PR-URL: #46839
Fixes: #46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 13, 2023
@richiemccoll @targos
test_runner: emit test-only diagnostic warning
62d0c0f 
PR-URL: #46540
Fixes: #46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 14, 2023
@richiemccoll @targos
test_runner: emit test-only diagnostic warning
ffa86f7 
PR-URL: #46540
Fixes: #46448
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test_runner: emit test-only diagnostic warning richiemccoll/node
4 participants
@cjihrig @MoLow @richiemccoll and others