FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: fix global before not called when no global test exists by rluvaton · Pull Request #48877 · nodejs/node · GitHub

/ node Public

test_runner: fix global before not called when no global test exists - #48877

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
rluvaton:global-before-not-run-if-no-global-test
Jul 24, 2023
Merged

test_runner: fix global before not called when no global test exists#48877
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
rluvaton:global-before-not-run-if-no-global-test

Conversation

rluvaton commented Jul 21, 2023
edited
Loading

Copy link
Copy Markdown
Member

fix #48844

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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',

rluvaton Jul 22, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/internal/test_runner/test.js Outdated
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

rluvaton Jul 23, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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>
Comment thread lib/internal/test_runner/test.js Outdated
atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2023

Copy link
Copy Markdown
Collaborator

atlowChemi added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 24, 2023

MoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. I think afterEach and beforeEach might need the same fix, but that can be handled by another PR

Copy link
Copy Markdown
Member Author

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

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

Copy link
Copy Markdown
Collaborator

Landed in a0f3ed8

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>
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 mentioned this pull request Aug 15, 2023

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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 mentioned this pull request Sep 10, 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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 outstanding review comments, 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: global before doesn't run if there are no global test

7 participants


Back | FazBrowse Home | New Git URL