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: t.after is never called #51997

Closed
KhafraDev opened this issue Mar 7, 2024 · 9 comments
Closed

test_runner: t.after is never called #51997

KhafraDev opened this issue Mar 7, 2024 · 9 comments
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@KhafraDev
Copy link
Member

Version

v21.7.0

Platform

n/a

Subsystem

test_runner

What steps will reproduce the bug?

import { once } from 'node:events'
import { createServer } from 'node:http'
import { test } from 'node:test'

test('after is called', async (t) => {
  const server = createServer((req, res) => res.end())
    .listen(0)

  t.after(() => {
    console.log('Called!')
    server.close()
  })
  await once(server, 'listening')

  await fetch(`http://localhost:${server.address().port}`)
})

node --test test.mjs

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

always

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

No response

What do you see instead?

the test hangs, "Called!" is never printed to the console

Additional information

It could be caused by #51389?

@KhafraDev KhafraDev added the test_runner Issues and PRs related to the test runner subsystem. label Mar 7, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2024

It could be caused by #51389?

Probably. It looks like #51389 was fixing #51371, which does not look like a bug according to the documentation.

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2024

cross referencing to nodejs/undici#2931

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2024

@cjihrig

In #51371 the example is

import { test } from 'node:test';
import { open } from 'node:fs/promises';

test('test', async (t) => {
  let filehandle;

  t.before(async () => {
    console.log('before');
    filehandle = await open('./index.mjs', 'r');
  });

  t.after(async () => {
    console.log('after');
    await filehandle.close();
  });

//   await t.test(() => {});
});

This can be simplified to

import { test } from 'node:test';

test('test', async (t) => {

  t.before(async () => {
    console.log('before');
  });

  t.after(async () => {
    console.log('after');
  });

  // await t.test(() => {});
});

So running this on node 21.6.2 results in printing only after but not before. In 21.7.0 it does not print anything.
commenting in the test call then in 21.6.2 it prints before and after, but in 21.7.0 only before.

cjihrig added a commit to cjihrig/node that referenced this issue Mar 7, 2024
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
cjihrig added a commit to cjihrig/node that referenced this issue Mar 7, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2024

I have opened #51998 with the revert + a regression test.

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2024

I am currently building node from main branch and want to understand what we should expect. But I wonder what the correct behavior is. Should it really not trigger before and after if there is not test, or should it anyway run before and after if there is no test?!

@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2024

The documented behavior for context.before() is:

This function is used to create a hook running before subtest of the current test.

The documented behavior for context.after() is:

This function is used to create a hook that runs after the current test finishes.

before(), beforeEach(), and afterEach() only run when there are subtests. after() should run all the time for cleanup purposes (this issue). The reason before() doesn't run all the time is because there are no subtests to run before and it can't run before the test itself that is already running.

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2024

Actually I have the feeling that before should also run always like after. But I thinks that your revert PR should land asap.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 7, 2024

Feel free to change whatever you want 😄

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 7, 2024

I would rather prefer continue working on undici ;).

nodejs-github-bot pushed a commit that referenced this issue Mar 7, 2024
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Mar 7, 2024
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: #51997
PR-URL: #51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue Mar 7, 2024
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Refs: #51997
PR-URL: #51998
Fixes: #51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
PR-URL: nodejs#51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Mar 26, 2024
Refs: nodejs#51997
PR-URL: nodejs#51998
Fixes: nodejs#51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jcbhmr pushed a commit to jcbhmr/node that referenced this issue May 15, 2024
This reverts commit a53fd95.

This caused a regression because the original issue this commit
was attempting to fix is not a bug. The after() hook should
always run.

Fixes: nodejs#51997
PR-URL: nodejs#51998
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
jcbhmr pushed a commit to jcbhmr/node that referenced this issue May 15, 2024
Refs: nodejs#51997
PR-URL: nodejs#51998
Fixes: nodejs#51997
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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