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: skip each hooks for skipped tests #52115

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 16, 2024

When a test is skipped, the corresponding beforeEach and afterEach hooks should also be skipped.

Fixes: #52112

When a test is skipped, the corresponding beforeEach and afterEach
hooks should also be skipped.

Fixes: nodejs#52112
@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 Mar 16, 2024
Copy link
Member

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

What do you think about adding a test that it's not skipped for regular tests when having both regular and skipped + adding test that not running before each when entire describe is skipped

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 16, 2024

Added more tests.

Co-authored-by: Chemi Atlow <chemi@atlow.co.il>
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 17, 2024
@nodejs-github-bot
Copy link
Collaborator

@atlowChemi atlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2024
@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 18, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 18, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/52115
✔  Done loading data for nodejs/node/pull/52115
----------------------------------- PR info ------------------------------------
Title      test_runner: skip each hooks for skipped tests (#52115)
Author     Colin Ihrig  (@cjihrig)
Branch     cjihrig:each-hooks -> nodejs:main
Labels     author ready, needs-ci, test_runner
Commits    3
 - test_runner: skip each hooks for skipped tests
 - additional tests
 - Update test/fixtures/test-runner/output/suite-skip-hooks.js
Committers 2
 - cjihrig 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/52115
Fixes: https://github.com/nodejs/node/issues/52112
Reviewed-By: Raz Luvaton 
Reviewed-By: Chemi Atlow 
Reviewed-By: Moshe Atlow 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52115
Fixes: https://github.com/nodejs/node/issues/52112
Reviewed-By: Raz Luvaton 
Reviewed-By: Chemi Atlow 
Reviewed-By: Moshe Atlow 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 16 Mar 2024 16:13:58 GMT
   ✔  Approvals: 3
   ✔  - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/52115#pullrequestreview-1941189165
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52115#pullrequestreview-1941388256
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52115#pullrequestreview-1941414647
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-03-17T07:13:58Z: https://ci.nodejs.org/job/node-test-pull-request/57786/
- Querying data for job/node-test-pull-request/57786/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 52115
From https://github.com/nodejs/node
 * branch                  refs/pull/52115/merge -> FETCH_HEAD
✔  Fetched commits as 8a191e4e6a39..3d5934220ed7
--------------------------------------------------------------------------------
Auto-merging lib/internal/test_runner/test.js
[main 3d705cbf9b] test_runner: skip each hooks for skipped tests
 Author: cjihrig 
 Date: Sat Mar 16 12:12:44 2024 -0400
 4 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 test/fixtures/test-runner/output/skip-each-hooks.js
 create mode 100644 test/fixtures/test-runner/output/skip-each-hooks.snapshot
[main a567bd199f] additional tests
 Author: cjihrig 
 Date: Sat Mar 16 13:32:22 2024 -0400
 3 files changed, 101 insertions(+)
 create mode 100644 test/fixtures/test-runner/output/suite-skip-hooks.js
 create mode 100644 test/fixtures/test-runner/output/suite-skip-hooks.snapshot
[main 8c4534e860] Update test/fixtures/test-runner/output/suite-skip-hooks.js
 Author: Colin Ihrig 
 Date: Sat Mar 16 19:13:57 2024 -0400
 1 file changed, 16 deletions(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes
⚠ Found Fixes: #52112, skipping..
--------------------------------- New Message ----------------------------------
test_runner: skip each hooks for skipped tests

When a test is skipped, the corresponding beforeEach and afterEach
hooks should also be skipped.

Fixes: #52112
PR-URL: #52115
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il
Reviewed-By: Moshe Atlow moshe@atlow.co.il

[detached HEAD 5e03fbb79a] test_runner: skip each hooks for skipped tests
Author: cjihrig cjihrig@gmail.com
Date: Sat Mar 16 12:12:44 2024 -0400
4 files changed, 35 insertions(+), 2 deletions(-)
create mode 100644 test/fixtures/test-runner/output/skip-each-hooks.js
create mode 100644 test/fixtures/test-runner/output/skip-each-hooks.snapshot
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
additional tests

PR-URL: #52115
Fixes: #52112
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il
Reviewed-By: Moshe Atlow moshe@atlow.co.il

[detached HEAD 098047ad02] additional tests
Author: cjihrig cjihrig@gmail.com
Date: Sat Mar 16 13:32:22 2024 -0400
3 files changed, 101 insertions(+)
create mode 100644 test/fixtures/test-runner/output/suite-skip-hooks.js
create mode 100644 test/fixtures/test-runner/output/suite-skip-hooks.snapshot
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test/fixtures/test-runner/output/suite-skip-hooks.js

Co-authored-by: Chemi Atlow chemi@atlow.co.il
PR-URL: #52115
Fixes: #52112
Reviewed-By: Raz Luvaton rluvaton@gmail.com
Reviewed-By: Chemi Atlow chemi@atlow.co.il
Reviewed-By: Moshe Atlow moshe@atlow.co.il

[detached HEAD 1440c50495] Update test/fixtures/test-runner/output/suite-skip-hooks.js
Author: Colin Ihrig cjihrig@gmail.com
Date: Sat Mar 16 19:13:57 2024 -0400
1 file changed, 16 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/8329948090

@MoLow MoLow added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 18, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6f4d601 into nodejs:main Mar 18, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6f4d601

@cjihrig cjihrig deleted the each-hooks branch March 18, 2024 16:34
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
When a test is skipped, the corresponding beforeEach and afterEach
hooks should also be skipped.

Fixes: nodejs#52112
PR-URL: nodejs#52115
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
When a test is skipped, the corresponding beforeEach and afterEach
hooks should also be skipped.

Fixes: #52112
PR-URL: #52115
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
When a test is skipped, the corresponding beforeEach and afterEach
hooks should also be skipped.

Fixes: #52112
PR-URL: #52115
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
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.

test_runner: skip beforeEach and afterEach hooks if test is skipped
5 participants