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: align behavior of it and test #46889

Closed
wants to merge 2 commits into from
Closed

test_runner: align behavior of it and test #46889

wants to merge 2 commits into from

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Feb 28, 2023 •
edited
Loading

following #46544 and #46888,
I think the differences between test and it lead to a lot of confusion and was a mistake.

once #46888 lands I propose we remove the differences.
feedback is welcomed

TODO:

  • update docs

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 28, 2023
aduh95
aduh95 reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

It would need a doc upgrade, and an entry in the history section of a YAML comment

benjamingr
benjamingr reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I agree I think the initial differences were a mistake in retrospect (as Antoine points out this needs a doc change)

benjamingr
benjamingr approved these changes Mar 1, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM once docs are green.

Note this is a breaking change so it may need to semver-major (or at least land before the runner is stable in 20.0

@MoLow MoLow force-pushed the align_it_and_test branch from 220c61d to 109cf16 Compare March 3, 2023 06:24
@MoLow MoLow requested review from aduh95 and cjihrig March 3, 2023 06:24
@MoLow MoLow force-pushed the align_it_and_test branch from 109cf16 to 9f185fa Compare March 3, 2023 06:30
aduh95
aduh95 reviewed Mar 3, 2023
View reviewed changes
doc/api/test.md Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Mar 3, 2023
View reviewed changes
doc/api/test.md Outdated Show resolved Hide resolved
@MoLow @aduh95
Apply suggestions from code review
e2eba87 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Mar 4, 2023
Open
17 tasks
@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 4, 2023
@github-actions github-actions bot mentioned this pull request Mar 5, 2023
Open
17 tasks
@MoLow
Copy link
Member Author

MoLow commented Mar 5, 2023

Landed in ca033c1

MoLow added a commit that referenced this pull request Mar 5, 2023
@MoLow
test_runner: align behavior of it and test
ca033c1 
PR-URL: #46889
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MoLow MoLow closed this Mar 5, 2023
@MoLow MoLow deleted the align_it_and_test branch March 5, 2023 07:42
This was referenced Mar 6, 2023
Open
Open
This was referenced Mar 8, 2023
Open
Open
targos pushed a commit that referenced this pull request Mar 13, 2023
@MoLow @targos
test_runner: align behavior of it and test
05ed78e 
PR-URL: #46889
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MoLow MoLow mentioned this pull request Mar 13, 2023
Closed
targos pushed a commit that referenced this pull request Mar 14, 2023
@MoLow @targos
test_runner: align behavior of it and test
762dc7c 
PR-URL: #46889
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request Mar 14, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@MoLow @danielleadams
test_runner: align behavior of it and test
e52b169 
PR-URL: #46889
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request May 16, 2023
@antongolub
fix(node): align behavior of it and test
feff7c4 
nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request May 16, 2023
@antongolub
fix(node): align behavior of it and test
99f286a 
nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
@antongolub antongolub mentioned this pull request May 16, 2023
Merged
8 tasks
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request May 16, 2023
@antongolub
fix(node): align behavior of it and test
8212abd 
nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
antongolub added a commit to antongolub-forks/DefinitelyTyped that referenced this pull request May 16, 2023
@antongolub
fix(node): align behavior of it and test
239da32 
nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request May 16, 2023
@antongolub
🤖 Merge PR #65511 fix(node): align behavior of it and test by @antong…
2679552 
…olub

nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
@atlowChemi atlowChemi mentioned this pull request Jun 28, 2023
Closed
Desplandis pushed a commit to Desplandis/DefinitelyTyped that referenced this pull request Jul 3, 2023
@antongolub @Desplandis
🤖 Merge PR DefinitelyTyped#65511 fix(node): align behavior of it and …
7f7ec8c 
…test by @antongolub

nodejs/node#46889
https://nodejs.org/en/blog/release/v18.16.0
https://nodejs.org/en/blog/release/v19.8.0
@panva panva removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
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.
6 participants
@MoLow @nodejs-github-bot @benjamingr @cjihrig @aduh95 @panva