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: fix global after hook #48231

Merged
merged 3 commits into from
May 31, 2023
Merged

test_runner: fix global after hook #48231

merged 3 commits into from
May 31, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented May 28, 2023 •
edited
Loading

Fixes: #48230

@MoLow MoLow requested a review from cjihrig May 28, 2023 14:52
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 May 28, 2023
cjihrig
cjihrig reviewed May 29, 2023
View reviewed changes
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Show resolved Hide resolved
@MoLow
Copy link
Member Author

MoLow commented May 30, 2023

@cjihrig I just pushed a fix making root test use run and this way it gets all the logic and hooks included inside run OOTB.
this meant I had to adjust run to be 100% synchronous in cases we use it for the root test

@MoLow MoLow requested a review from cjihrig May 30, 2023 20:02
cjihrig
cjihrig reviewed May 31, 2023
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.

this meant I had to adjust run to be 100% synchronous in cases we use it for the root test

run() can't be 100% synchronous - it's an async function.

I guess there are no implications since the tests seem to pass, but don't we call root.postRun() in runner.js? Should we be calling root.run() there instead now?

lib/internal/test_runner/harness.js Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member Author

MoLow commented May 31, 2023

but don't we call root.postRun() in runner.js? Should we be calling root.run() there instead now?

well in runner.js we run test files only, and we use a root that is not global (created using createTestTree), so there is no need to use run since it is impossible to set hooks or anything else that run handles

@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5e98a74 into nodejs:main May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5e98a74

@MoLow MoLow deleted the fix-global-after branch May 31, 2023 15:33
@github-actions github-actions bot mentioned this pull request Jun 2, 2023
Open
34 tasks
This was referenced Jun 3, 2023
Open
Open
targos pushed a commit that referenced this pull request Jun 4, 2023
@MoLow @targos
test_runner: fix global after hook
f3268d6 
PR-URL: #48231
Fixes: #48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Jun 4, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jun 5, 2023
Open
30 tasks
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
@MoLow @franciszek-koltuniuk-red
test_runner: fix global after hook
659d5e2 
PR-URL: nodejs#48231
Fixes: nodejs#48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow @danielleadams
test_runner: fix global after hook
44d17d6 
PR-URL: #48231
Fixes: #48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
test_runner: fix global after hook
6a82fbd 
PR-URL: nodejs#48231
Fixes: nodejs#48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: fix global after hook
f044963 
PR-URL: nodejs#48231
Fixes: nodejs#48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@MoLow @Ceres6
test_runner: fix global after hook
178db24 
PR-URL: nodejs#48231
Fixes: nodejs#48230
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlowChemi atlowChemi atlowChemi left review comments

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

test-runner: global after not working
5 participants
@MoLow @nodejs-github-bot @benjamingr @cjihrig @atlowChemi