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 after hook inside test suite is not executed if before() throws an error #51062

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

pulkit-30
Copy link
Contributor

fix #50842

fix code:

import { it, before, describe, after } from 'node:test';

describe('failing test suite', () => {
  before(async () => {
    console.log('before called');
    throw new Error('error');
  });

  after(async () => {
    console.log('after called');
  });

  it('should be ok', async () => {});
});
pulkitgupta@Pulkits-MacBook-Air node % ./node index.mjs
before called
+ after called
▶ failing test suite
  ✖ should be ok
    'test did not finish before its parent and was cancelled'

▶ failing test suite (3.217584ms)

  Error: error
      at SuiteContext.<anonymous> (file:///Users/pulkitgupta/Desktop/node/index.mjs:6:11)
      at TestHook.runInAsyncScope (node:async_hooks:206:9)
      at TestHook.run (node:internal/test_runner/test:631:25)
      at TestHook.run (node:internal/test_runner/test:856:18)
      at TestHook.run (node:internal/util:522:12)
      at node:internal/test_runner/test:565:20
      at async Suite.runHook (node:internal/test_runner/test:563:7)
      at async Suite.run (node:internal/test_runner/test:945:7)
      at async startSubtest (node:internal/test_runner/harness:216:3)

ℹ tests 1
ℹ suites 1
ℹ pass 0
ℹ fail 0
ℹ cancelled 1
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 15.439667

✖ failing tests:

test at file:/Users/pulkitgupta/Desktop/node/index.mjs:13:3
✖ should be ok
  'test did not finish before its parent and was cancelled'

test at file:/Users/pulkitgupta/Desktop/node/index.mjs:3:1
✖ failing test suite (3.217584ms)
  Error: error
      at SuiteContext.<anonymous> (file:///Users/pulkitgupta/Desktop/node/index.mjs:6:11)
      at TestHook.runInAsyncScope (node:async_hooks:206:9)
      at TestHook.run (node:internal/test_runner/test:631:25)
      at TestHook.run (node:internal/test_runner/test:856:18)
      at TestHook.run (node:internal/util:522:12)
      at node:internal/test_runner/test:565:20
      at async Suite.runHook (node:internal/test_runner/test:563:7)
      at async Suite.run (node:internal/test_runner/test:945:7)
      at async startSubtest (node:internal/test_runner/harness:216:3)
pulkitgupta@Pulkits-MacBook-Air node % 

ref: #50842 (comment)

@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 Dec 5, 2023
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

LGTM. can you please amend the commit message to adher guidlines?

@pulkit-30
Copy link
Contributor Author

@rluvaton PTAL

@rluvaton
Copy link
Member

@pulkit-30 I have a lot of GitHub notifications so I'm likely gonna miss something. if I do, you can ping me at https://twitter.com/rluvaton

@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@rluvaton rluvaton 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 Dec 17, 2023
@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 Dec 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51062
✔  Done loading data for nodejs/node/pull/51062
----------------------------------- PR info ------------------------------------
Title      test_runner: fix after hook inside test suite is not executed if before() throws an error (#51062)
Author     Pulkit Gupta  (@pulkit-30)
Branch     pulkit-30:fix-50842 -> nodejs:main
Labels     author ready, needs-ci, test_runner
Commits    2
 - test_runner: fixed to run after hook if before throws an error
 - fix tests
Committers 1
 - pulkit-30 
PR-URL: https://github.com/nodejs/node/pull/51062
Refs: https://github.com/nodejs/node/issues/50842
Reviewed-By: Moshe Atlow 
Reviewed-By: Raz Luvaton 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51062
Refs: https://github.com/nodejs/node/issues/50842
Reviewed-By: Moshe Atlow 
Reviewed-By: Raz Luvaton 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 05 Dec 2023 14:47:02 GMT
   ✔  Approvals: 2
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/51062#pullrequestreview-1766090728
   ✔  - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/51062#pullrequestreview-1785478063
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-17T17:25:50Z: https://ci.nodejs.org/job/node-test-pull-request/56352/
- Querying data for job/node-test-pull-request/56352/
   ✔  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 51062
From https://github.com/nodejs/node
 * branch                  refs/pull/51062/merge -> FETCH_HEAD
✔  Fetched commits as 0afe731d357d..7bef78c9d7d3
--------------------------------------------------------------------------------
[main d72d26f820] test_runner: fixed to run after hook if before throws an error
 Author: pulkit-30 
 Date: Wed Dec 6 07:04:01 2023 +0530
 4 files changed, 87 insertions(+), 8 deletions(-)
[main 15f53e6fb6] fix tests
 Author: pulkit-30 
 Date: Wed Dec 6 07:07:39 2023 +0530
 2 files changed, 3 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: fixed to run after hook if before throws an error

PR-URL: #51062
Refs: #50842
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Raz Luvaton rluvaton@gmail.com

[detached HEAD abcb705896] test_runner: fixed to run after hook if before throws an error
Author: pulkit-30 pulkit30.bsr@gmail.com
Date: Wed Dec 6 07:04:01 2023 +0530
4 files changed, 87 insertions(+), 8 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

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

PR-URL: #51062
Refs: #50842
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Raz Luvaton rluvaton@gmail.com

[detached HEAD 87d85068e6] fix tests
Author: pulkit-30 pulkit30.bsr@gmail.com
Date: Wed Dec 6 07:07:39 2023 +0530
2 files changed, 3 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/7240829485

@rluvaton rluvaton 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 Dec 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit 71e19e8 into nodejs:main Dec 17, 2023
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 71e19e8

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #51062
Refs: #50842
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51062
Refs: #50842
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
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.

After inside describe is not executed when the before has an error
4 participants