GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: fix global before not called when no global test exists by rluvaton · Pull Request #48877 · nodejs/node

Conversation

@rluvaton

@rluvaton rluvaton commented Jul 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

fix #48844

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and tonivj5 reacted with thumbs up emoji tonivj5 and bricss reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 2 reactions
@rluvaton
test_runner: fix global before not called when no global test exists
2969867
@nodejs-github-bot

nodejs-github-bot commented Jul 21, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@rluvaton
add test
99786ae
rluvaton
rluvaton commented Jul 22, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/output/hooks-with-no-global-test.js
'global after',
]);
} catch (e) {
// TODO(rluvaton): remove the try catch after #48867 is fixed

@rluvaton rluvaton Jul 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
rluvaton
rluvaton commented Jul 22, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/output/hooks-with-no-global-test.js
Comment on lines +30 to +31
'describe afterEach',
'describe nested afterEach',

@rluvaton rluvaton Jul 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rluvaton rluvaton Jul 22, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aduh95
aduh95 reviewed Jul 22, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/output/hooks-with-no-global-test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
atlowChemi
atlowChemi reviewed Jul 23, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Outdated
}

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

@atlowChemi atlowChemi Jul 23, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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());

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Jul 23, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rluvaton rluvaton Jul 23, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rluvaton rluvaton Jul 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Jul 24, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rluvaton rluvaton Jul 24, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Jul 24, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rluvaton rluvaton Jul 24, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
rluvaton and others added 2 commits July 23, 2023 10:09
@rluvaton
remove ?.
5b2c5d6
@rluvaton @aduh95
Update test/fixtures/test-runner/output/hooks-with-no-global-test.js
6c5a5af 
Co-authored-by: Antoine du Hamel <[email protected]>
atlowChemi
atlowChemi approved these changes Jul 23, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@rluvaton
remove ?.
f2c551c
atlowChemi
atlowChemi approved these changes Jul 24, 2023
View reviewed changes
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52912/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi atlowChemi added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jul 24, 2023
MoLow
MoLow approved these changes Jul 24, 2023
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rluvaton

rluvaton commented Jul 24, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi atlowChemi added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jul 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 24, 2023
@nodejs-github-bot
nodejs-github-bot merged commit a0f3ed8 into nodejs:main Jul 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 24, 2023

Copy link
Copy Markdown
Collaborator

Landed in a0f3ed8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@rluvaton @Ceres6
test_runner: fix global before not called when no global test exists
d27f556 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@ghost ghost mentioned this pull request Aug 1, 2023
Test runner executes after() in declared order #48736
Closed
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@rluvaton @Ceres6
test_runner: fix global before not called when no global test exists
5afbe2a 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@rluvaton @Ceres6
test_runner: fix global before not called when no global test exists
2d860df 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@rluvaton @UlisesGascon
test_runner: fix global before not called when no global test exists
79ccb66 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@rluvaton @RafaelGSS
test_runner: fix global before not called when no global test exists
3b5663c 
PR-URL: #48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged
@RafaelGSS

RafaelGSS commented Aug 17, 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
rluvaton reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS added the backport-requested-v20.x label Aug 17, 2023
This was referenced Aug 17, 2023
test_runner: cleanup test timeout abort listener #48915
Merged
test_runner: fix timeout in *Each hook failing further tests #48925
Merged
test_runner: fix global after not failing the tests #48913
Merged
test: remove --no-warnings flag in test_runner fixtures #48989
Merged
test_runner: expose location of tests #48975
Merged
test_runner: reland run global after() hook earlier #49116
Merged
rluvaton added a commit to rluvaton/node that referenced this pull request Aug 17, 2023
@rluvaton
test_runner: fix global before not called when no global test exists
b6ea2cd 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@rluvaton rluvaton added backport-open-v20.x and removed backport-requested-v20.x labels Aug 17, 2023
This was referenced Aug 17, 2023
[v20.x backport] test_runner: fix global before not called when no global test exists #49219
Closed
[v20.x backport] backport multiple test runner related prs #49225
Closed
@rluvaton

rluvaton commented Aug 18, 2023

Copy link
Copy Markdown
Member Author

Backport PR:

  • [v20.x backport] backport multiple test runner related prs #49225
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rluvaton added a commit to rluvaton/node that referenced this pull request Sep 4, 2023
@rluvaton
test_runner: fix global before not called when no global test exists
b84b2bf 
PR-URL: nodejs#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@rluvaton @UlisesGascon
test_runner: fix global before not called when no global test exists
75333f3 
PR-URL: #48877
Backport-PR-URL: #49225
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
@targos targos added backported-to-v20.x and removed backport-open-v20.x labels Nov 12, 2023
targos pushed a commit that referenced this pull request Nov 27, 2023
@rluvaton @targos
test_runner: fix global before not called when no global test exists
66ea9bd 
PR-URL: #48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: fix global before not called when no global test
b88b35e 
PR-URL: nodejs/node#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: fix global before not called when no global test
dfea090 
PR-URL: nodejs/node#48877
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@MoLow MoLow MoLow approved these changes
@atlowChemi atlowChemi atlowChemi approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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: global before doesn't run if there are no global test

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@rluvaton @nodejs-github-bot @RafaelGSS @MoLow @aduh95 @atlowChemi @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.