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

Revert "test_runner: do not invoke after hook when test is empty" #51998

Closed
wants to merge 2 commits into from
Closed

Revert "test_runner: do not invoke after hook when test is empty" #51998

wants to merge 2 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 7, 2024

Fixes: #51997

cjihrig added 2 commits March 6, 2024 21:13
@cjihrig
Revert "test_runner: do not invoke after hook when test is empty"
c39a545 
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
@cjihrig
test: add regression test for test_runner after hook
cfccb99 
Refs: nodejs#51997
@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 7, 2024
@cjihrig cjihrig mentioned this pull request Mar 7, 2024
Closed
@cjihrig cjihrig changed the title https://github.com/nodejs/node/commit/c39a545bd96873873f2f5f4aff299194438964b8 Revert "test_runner: do not invoke after hook when test is empty" Mar 7, 2024
@richardlau richardlau mentioned this pull request Mar 7, 2024
Merged
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 7, 2024
mcollina
mcollina approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Mar 7, 2024
@metcoder95 metcoder95 mentioned this pull request Mar 7, 2024
Closed
@targos
Copy link
Member

targos commented Mar 7, 2024

Should we make a quick release to fix this regression?

@mcollina
Copy link
Member

mcollina commented Mar 7, 2024

That'd be really useful, thanks.

@mcollina mcollina added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 7, 2024

Fast-track has been requested by @mcollina. Please 👍 to approve.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 7, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 4f3cf4e...3aa0658

nodejs-github-bot pushed a commit that referenced this pull request Mar 7, 2024
@cjihrig @nodejs-github-bot
Revert "test_runner: do not invoke after hook when test is empty"
0e8070d 
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: #51997
PR-URL: #51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Mar 7, 2024
@cjihrig @nodejs-github-bot
test: add regression test for test_runner after hook
3aa0658 
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Mar 7, 2024
@cjihrig @targos
Revert "test_runner: do not invoke after hook when test is empty"
85aa6ca 
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: #51997
PR-URL: #51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Mar 7, 2024
@cjihrig @targos
test: add regression test for test_runner after hook
bee3b36 
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This was referenced Mar 7, 2024
Closed
Merged
@marco-ippolito
Copy link
Member

Sorry for the regression 😢🙏🏻

@cjihrig cjihrig deleted the revert branch March 7, 2024 14:01
@climba03003 climba03003 mentioned this pull request Mar 8, 2024
Closed
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@cjihrig @richardlau
test: add regression test for test_runner after hook
5f340f2 
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@cjihrig @richardlau
test: add regression test for test_runner after hook
905c48f 
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@cjihrig @rdw-msft
Revert "test_runner: do not invoke after hook when test is empty"
fda645a 
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
PR-URL: nodejs#51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@cjihrig @rdw-msft
test: add regression test for test_runner after hook
9f17dc5 
Refs: nodejs#51997
PR-URL: nodejs#51998
Fixes: nodejs#51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
@cjihrig @jcbhmr
Revert "test_runner: do not invoke after hook when test is empty"
ecc9084 
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
PR-URL: nodejs#51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
@cjihrig @jcbhmr
test: add regression test for test_runner after hook
252f1e1 
Refs: nodejs#51997
PR-URL: nodejs#51998
Fixes: nodejs#51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KhafraDev KhafraDev KhafraDev approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@MoLow MoLow MoLow approved these changes

@richardlau richardlau richardlau approved these changes

@targos targos targos approved these changes

@mcollina mcollina mcollina 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-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fast-track PRs that do not need to wait for 48 hours to land. 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: t.after is never called
9 participants
@cjihrig @nodejs-github-bot @targos @mcollina @marco-ippolito @richardlau @MoLow @KhafraDev @aduh95