| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
| 'global after', | ||
| ]); | ||
| } catch (e) { | ||
| // TODO(rluvaton): remove the try catch after #48867 is fixed |
There was a problem hiding this comment.
because of #48867 we must use process.exit as otherwise, it won't fail the test
Sorry, something went wrong.
| 'describe afterEach', | ||
| 'describe nested afterEach', |
There was a problem hiding this comment.
it seems really weird that we executing top describe afterEach before the nested one...
Sorry, something went wrong.
There was a problem hiding this comment.
I see #48736 (comment) also talked about afterEach...
Sorry, something went wrong.
| } | ||
|
|
||
| if (this.parent?.hooks.before.length > 0) { | ||
| await this.parent.runHook('before', this.parent?.getRunArgs()); |
There was a problem hiding this comment.
If we reach here this.parent will have a value (and we already call runHook without the optional chaining)
| await this.parent.runHook('before', this.parent?.getRunArgs()); | |
| await this.parent.runHook('before', this.parent.getRunArgs()); |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess It is just surprising that the before and after hooks are not "symmetric"
Sorry, something went wrong.
There was a problem hiding this comment.
It is, the first describe should run the before and the last describe should run the after...
Sorry, something went wrong.
There was a problem hiding this comment.
I would simply expect to also see this.parent.runHook('after') - but I guess I am missing something
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. I think afterEach and beforeEach might need the same fix, but that can be handled by another PR
Sorry, something went wrong.
|
There is no need as we concat the before and after each in the constructor |
Sorry, something went wrong.
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: #48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
|
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 |
Sorry, something went wrong.
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sorry, something went wrong.
PR-URL: nodejs#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: #48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs/node#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs/node#48877 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
| Back | FazBrowse Home | New Git URL |
fix #48844