FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: fix #50665 `.skip`, `.todo` and `.only` missing in subtests by rluvaton · Pull Request #50673 · nodejs/node · GitHub

/ node Public

test_runner: fix #50665 .skip, .todo and .only missing in subtests - #50673

Closed
rluvaton wants to merge 1 commit into
nodejs:mainfrom
rluvaton:fix-50665-only-skip-todo-not-available-for-subtest
ConversationCommits (1)ChecksFiles changed
Closed

test_runner: fix #50665 .skip, .todo and .only missing in subtests#50673
rluvaton wants to merge 1 commit into
nodejs:mainfrom
rluvaton:fix-50665-only-skip-todo-not-available-for-subtest

Conversation

rluvaton commented Nov 11, 2023

Copy link
Copy Markdown
Member

Fix #50665

nodejs-github-bot commented Nov 11, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Nov 11, 2023
rluvaton force-pushed the fix-50665-only-skip-todo-not-available-for-subtest branch 2 times, most recently from 638b7b6 to 63fdacf Compare November 11, 2023 21:28
rluvaton marked this pull request as ready for review November 11, 2023 21:28
rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2023
rluvaton force-pushed the fix-50665-only-skip-todo-not-available-for-subtest branch from 63fdacf to 8834f4b Compare November 11, 2023 21:31
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2023

nodejs-github-bot commented Nov 11, 2023

Copy link
Copy Markdown
Collaborator

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

rluvaton added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 11, 2023
rluvaton requested review from MoLow, atlowChemi and cjihrig and removed request for cjihrig November 11, 2023 23:36
ljharb reviewed Nov 12, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated
Comment on lines 127 to 236

ljharb Nov 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

wouldn't it be more performant to leave this as a prototype method, and instead do a public class field test = FunctionPrototypeBind(this.test, this);, so that the "meat" of the method can be shared and optimized across all instances?

separately, couldn't the skip/todo/only methods be assigned to the prototype method once, in a static block, instead of a new function created on every instance?

rluvaton Nov 12, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you mean with Object.defineProperty, then without changing the #test to [kTest] it can't as it cannot access private method outside the class.
if you mean adding the skip/todo/any in the constructor but keeping the test function as is, than for some reason it did not work for me

The skip/todo/any methods are needed to assign on the test function, not on the TestContext

ljharb Nov 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, true, the 3 expandos need to be done in the constructor every time - altho the meat could be shared private methods and the per-instance thing could just be a bound function.

MoLow reviewed Nov 12, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated

MoLow Nov 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

why not keep as prototype methods?

rluvaton Nov 12, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

for some reason it did not work for me adding skip/todo/etc to the test function

bakkot commented Nov 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

The docs for only say

it is possible to skip all top level tests except for a selected subset by passing the only option to the tests that should be run. When a test with the only option set is run, all subtests are also run.

How is that supposed to map to use of only in subtests? Consider test('a', t => { t.test.only('b', () => {}); }) - i.e., a top-level test which is not marked as only with a subtest which is. I can imagine at least two possibilities:

  • Since the top-level test does not have only, running in --test-only skips the entire top-level test. You'd have to use test.only('a', t => { t.test.only('b', () => {}); }) for this to work.
  • It somehow figures out that the top-level test contains a t.only, and enables the top-level test, but skips subtests of that test.

Presumably this implements the first one (though I don't see any tests for that scenario). Either way the docs should be updated, because right now they imply that only is specific to top-level tests. Maybe something like:

When a test with the only option set is run, all subtests are also run, unless some of the subtests of that test also have the only option set, in which case only those subtests are run. Using only for a subtest has no effect unless its parent test also has only set.

rluvaton commented Nov 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

@bakkot this is a known issue and unrelated to this PR or the linked issue

If I'm correct #48932 is the one to fix that issue

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 29, 2023

aduh95 commented Nov 29, 2023

Copy link
Copy Markdown
Contributor

Removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. as this PR has no approvals

rluvaton force-pushed the fix-50665-only-skip-todo-not-available-for-subtest branch from 8834f4b to 8ba986d Compare January 20, 2024 19:55
rluvaton force-pushed the fix-50665-only-skip-todo-not-available-for-subtest branch from 8ba986d to 77128c7 Compare June 25, 2024 09:16
cjihrig added the stalled Issues and PRs that are stalled. label Sep 19, 2024

github-actions Bot commented Sep 19, 2024

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions Bot commented Oct 20, 2024

Copy link
Copy Markdown
Contributor

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

github-actions Bot closed this Oct 20, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments
MoLow MoLow left review comments
atlowChemi Awaiting requested review from atlowChemi
cjihrig Awaiting requested review from cjihrig

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. 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.

test runner (--test, node:test) subtest test context lack .skip, .only and .todo functions

7 participants

Back | FazBrowse Home | New Git URL