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

Order of nested vs outer afterEach hooks is unexpected and inconsistent with after #51671

Closed
bendemboski opened this issue Feb 5, 2024 · 2 comments · Fixed by #52239
Closed
Labels
confirmed-bug Issues with confirmed bugs. test_runner Issues and PRs related to the test runner subsystem.

Comments

@bendemboski
Copy link

bendemboski commented Feb 5, 2024

Version

21.6.1

Platform

darwin

Subsystem

test runner

What steps will reproduce the bug?

Create a file called hooks.mjs containing:

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

describe('parent', () => {
  afterEach(() => console.log('parent after each'));
  after(() => console.log('parent after'));

  describe('child', () => {
    afterEach(() => console.log('child after each'));
    after(() => console.log('child after'));

    it('works', () => {});
  });
});

and run node --test hooks.mjs.

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

child after each
parent after each
child after
parent after
<test results>

As with mocha, qunit, jest, vitest, and probably others, a child suite's afterEach hooks should be called before those of parent suites, not after them. Also, the execution order of parent/child afterEach hooks should match the order of parent/child after hooks.

Currently, after hooks correctly run child-first, but afterEach hooks run incorrectly parent-first.

What do you see instead?

parent after each
child after each
child after
parent after
<test results>

Additional information

This also reproduces with the test()/subtest API -- a test's afterEach hook will run before a subtest's afterEach hook.

@marco-ippolito marco-ippolito added the test_runner Issues and PRs related to the test runner subsystem. label Feb 7, 2024
@marco-ippolito
Copy link
Member

marco-ippolito commented Feb 7, 2024

I agree subtest after and afterEach should be called before parent @nodejs/test_runner.
I couldnt reproduce with testthough.

This works fine:

'use strict';
require('../common');
const assert = require('node:assert');
const { test, afterEach, after } = require('node:test');


let afterEachRacing = true;
let afterRacing = true;

test('parent', () => {
    afterEach(() => afterEachRacing = false);
    after(() => afterRacing = false);

    test('child', () => {
        afterEach(() => assert.ok(afterEachRacing));
        after(() => assert.ok(afterRacing));
        test(() => { });
    });
});

this fails:

'use strict';
require('../common');
const assert = require('node:assert');
const { describe, it,  afterEach, after } = require('node:test');


let afterEachRacing = true;
let afterRacing = true;

describe('parent', () => {
    afterEach(() => afterEachRacing = false);
    after(() => afterRacing = false);

    describe('child', () => {
        afterEach(() => assert.ok(afterEachRacing));
        after(() => assert.ok(afterRacing));
        it(() => { });
    });
});

@bendemboski
Copy link
Author

bendemboski commented Feb 7, 2024

I meant that

test('parent', async (t) => {
  t.afterEach((c) => console.log('parent after each', c.name));
  t.after((c) => console.log('parent after', c.name));

  await t.test('child', async (t) => {
    t.afterEach((c) => console.log('child after each', c.name));
    t.after((c) => console.log('child after', c.name));
  
    await t.test('works', () => {});
  });
});

produces

parent after each works
child after each works
parent after each child
child after child
parent after parent

instead of

child after each works
parent after each works
parent after each child
child after child
parent after parent

so the parent's afterEach for works runs before the child's.

@cjihrig cjihrig added the confirmed-bug Issues with confirmed bugs. label Mar 28, 2024
cjihrig added a commit to cjihrig/node that referenced this issue Mar 28, 2024
This commit updates the test runner afterEach hook so that the
current test's afterEach hooks run before any ancestor afterEach
hooks.

Fixes: nodejs#51671
nodejs-github-bot pushed a commit that referenced this issue Mar 30, 2024
This commit updates the test runner afterEach hook so that the
current test's afterEach hooks run before any ancestor afterEach
hooks.

Fixes: #51671
PR-URL: #52239
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
This commit updates the test runner afterEach hook so that the
current test's afterEach hooks run before any ancestor afterEach
hooks.

Fixes: #51671
PR-URL: #52239
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
This commit updates the test runner afterEach hook so that the
current test's afterEach hooks run before any ancestor afterEach
hooks.

Fixes: #51671
PR-URL: #52239
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants