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.

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

test_runner: fix ordering of test hooks #47931

Merged
merged 3 commits into from
May 11, 2023

Conversation

philnash
Copy link
Contributor

@philnash philnash commented May 9, 2023

For tests with subtests the before hook was being run after the beforeEach hook, which is the opposite to test suites and expectations.

Also, a function was being used to close over the after hooks, but at the point it was being run the after hooks were not yet set up.

Fixes #47915

@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 9, 2023
@MoLow
Copy link
Member

MoLow commented May 9, 2023

please amend the commit message from test: fix ordering of test hooks to test_runner: fix ordering of test hooks

For tests with subtests the before hook was being run after the
beforeEach hook, which is the opposite to test suites and expectations.

Also, a function was being used to close over the after hooks, but at
the point it was being run the after hooks were not yet set up.

Fixes nodejs#47915
@philnash philnash changed the title test: fix ordering of test hooks test_runner: fix ordering of test hooks May 9, 2023
@philnash
Copy link
Contributor Author

philnash commented May 9, 2023

I've updated the commit message. Thanks for the review.

@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. labels May 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@philnash
Copy link
Contributor Author

I can see that some of the Jenkins jobs are failing. Though I'm not sure why. Is there anything I can do to help this?

@MoLow
Copy link
Member

MoLow commented May 10, 2023

I can see that some of the Jenkins jobs are failing. Though I'm not sure why. Is there anything I can do to help this?

that is not related to this PR, unfortunately there are some flaky tests

@nodejs-github-bot
Copy link
Collaborator

@philnash
Copy link
Contributor Author

that is not related to this PR, unfortunately there are some flaky tests

Ah, fair enough. Thank you!

@MoLow MoLow requested a review from cjihrig May 10, 2023 14:25
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit 24615bd into nodejs:main May 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 24615bd

@MoLow
Copy link
Member

MoLow commented May 11, 2023

Thanks for the contribution @philnash 🎉

@philnash
Copy link
Contributor Author

Thanks for the help getting this in @MoLow!

targos pushed a commit that referenced this pull request May 12, 2023
For tests with subtests the before hook was being run after the
beforeEach hook, which is the opposite to test suites and expectations.

Also, a function was being used to close over the after hooks, but at
the point it was being run the after hooks were not yet set up.

Fixes #47915

PR-URL: #47931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
For tests with subtests the before hook was being run after the
beforeEach hook, which is the opposite to test suites and expectations.

Also, a function was being used to close over the after hooks, but at
the point it was being run the after hooks were not yet set up.

Fixes #47915

PR-URL: #47931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
For tests with subtests the before hook was being run after the
beforeEach hook, which is the opposite to test suites and expectations.

Also, a function was being used to close over the after hooks, but at
the point it was being run the after hooks were not yet set up.

Fixes nodejs#47915

PR-URL: nodejs#47931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Order of test hooks seems wrong, and is different to the order of hooks for a describe suite
4 participants