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 global before not called when no global test exists #48877

Merged

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Jul 21, 2023

fix #48844

@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 Jul 21, 2023
'global after',
]);
} catch (e) {
// TODO(rluvaton): remove the try catch after #48867 is fixed
Copy link
Member Author

Choose a reason for hiding this comment

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

because of #48867 we must use process.exit as otherwise, it won't fail the test

Comment on lines +30 to +31
'describe afterEach',
'describe nested afterEach',
Copy link
Member Author

@rluvaton rluvaton Jul 22, 2023

Choose a reason for hiding this comment

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

it seems really weird that we executing top describe afterEach before the nested one...

Copy link
Member Author

Choose a reason for hiding this comment

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

I see #48736 (comment) also talked about afterEach...

@@ -821,6 +821,10 @@ class Suite extends Test {
return;
}

if (this.parent?.hooks.before.length > 0) {
await this.parent.runHook('before', this.parent?.getRunArgs());
Copy link
Member

Choose a reason for hiding this comment

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

If we reach here this.parent will have a value (and we already call runHook without the optional chaining)

Suggested change
await this.parent.runHook('before', this.parent?.getRunArgs());
await this.parent.runHook('before', this.parent.getRunArgs());

Copy link
Member

Choose a reason for hiding this comment

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

I would expect this to replace the call to this.runHook('before', hookArgs);, and also for the same implementation for beforeEach, afterEach and after

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't for after as if there are multiple describe we only need to run for the last one after all test completes

Copy link
Member Author

@rluvaton rluvaton Jul 23, 2023

Choose a reason for hiding this comment

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

Why did you expect that? Current describe hooks have different hookArgs that the parent (which I think is why some tests are failing when I run with the current tests args)

Copy link
Member

Choose a reason for hiding this comment

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

I guess It is just surprising that the before and after hooks are not "symmetric"

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, the first describe should run the before and the last describe should run the after...

Copy link
Member

Choose a reason for hiding this comment

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

I would simply expect to also see this.parent.runHook('after') - but I guess I am missing something

Copy link
Member Author

Choose a reason for hiding this comment

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

let's take this test for example:

const {describe, it, before, after} = require("node:test");


describe('top desc', () => {
  before(() => {
    console.log('before top desc');
  });

  after(() => {
    console.log('after top desc');
  });

  describe('inner describe 1', () => {
    before(() => {
      console.log('before inner 1 desc');
    });

    after(() => {
      console.log('after inner 1 desc');
    });

    it('inner it 1', () => {
      console.log('inner it 1');
    });
  });

  describe('inner describe 2', () => {
    before(() => {
      console.log('before inner 2 desc');
    });

    after(() => {
      console.log('after inner 2 desc');
    });

    it('inner it 1', () => {
      console.log('inner it 2');
    });
  });
});

the expected log is:

before top desc
before inner 1 desc
inner it 1
after inner 1 desc
before inner 2 desc
inner it 2
after inner 2 desc
after top desc <-----

if I add this.parent.runHook('after') as well the output will be:

before top desc
before inner 1 desc
inner it 1
after top desc <-----
after inner 1 desc
before inner 2 desc
inner it 2
after inner 2 desc

rluvaton and others added 2 commits July 23, 2023 10:09
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2023
@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 Jul 24, 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. I think afterEach and beforeEach might need the same fix, but that can be handled by another PR

@rluvaton
Copy link
Member Author

There is no need as we concat the before and after each in the constructor

@atlowChemi atlowChemi 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 Jul 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit a0f3ed8 into nodejs:main Jul 24, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a0f3ed8

@rluvaton rluvaton deleted the global-before-not-run-if-no-global-test branch July 24, 2023 18:33
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
@RafaelGSS
Copy link
Member

This commit didn't land cleanly on v20.x-staging. Could you please open a manual backport? Reference: https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md

@RafaelGSS RafaelGSS added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Aug 17, 2023
rluvaton added a commit to rluvaton/node that referenced this pull request Aug 17, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@rluvaton rluvaton added backport-open-v20.x Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels Aug 17, 2023
rluvaton added a commit to rluvaton/node that referenced this pull request Aug 18, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@rluvaton
Copy link
Member Author

rluvaton added a commit to rluvaton/node that referenced this pull request Sep 4, 2023
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #48877
Backport-PR-URL: #49225
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
targos pushed a commit that referenced this pull request Oct 28, 2023
PR-URL: #48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@targos targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-open-v20.x Indicate that the PR has an open backport labels Nov 12, 2023
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48877
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48877
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. backported-to-v20.x PRs backported to the v20.x-staging branch. 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: global before doesn't run if there are no global test
7 participants