Sitelet https://github.com/nodejs/node/issues/53202
Skip to content

test_runner: before hook runs even if it's not followed by any test #53202

Description

@urugator

Version

v20.13.1

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

test_runner

What steps will reproduce the bug?

import { before, describe, test } from 'node:test';

describe('suite1', (s) => {
  before(async () => {
    console.log(`${s.name} before`);
  });

  test('test1', async (t) => {
    console.log(`${s.name} ${t.name}`);
  });

  test('test2', async (t) => {
    console.log(`${s.name} ${t.name}`);
  });
});

Execute with:
node --test --test-name-pattern="nonsence"

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

Always. No.

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

before hook shouldn't run if it's not followed by any test. There is no practial or logical reason to run before hook alone, it wastes time and resources.

What do you see instead?

suite1 before
▶ suite1
  ﹣ test1 (2.2042ms) # test name does not match pattern
  ﹣ test2 (0.3612ms) # test name does not match pattern
▶ suite1 (8.3937ms)
ℹ tests 2
ℹ suites 1
ℹ pass 0
ℹ fail 0
ℹ cancelled 0
ℹ skipped 2
ℹ todo 0
ℹ duration_ms 141.0891

Additional information

No response

Activity

  1. marco-ippolito commented on May 29, 2024

    @marco-ippolito
    Member

    I've had the same thought but in reality just because you don't call any assertion it doesn't mean the test is not doing anything.
    Example: #51997
    The same it applies to t.before, changing it would break many things.

  2. urugator commented on May 29, 2024

    @urugator
    Author
    1. It's not a test, it's a suite. If test and suite are the technically the same thing, then the suite should respect --test-name-pattern and shouldn't run. Atm there is no way to run a single suite as at least before hook of all other suites are always executed. That makes zero sense. The setup logic in our tests takes minutes. So executing a single test in one suite can take hours, just because all these other suites have their own setup logic in before.
    2. Doc: This function is used to create a hook running before subtest of the current test.
      Firstly: Before subtest is quite different from before any code/assertions/whatever of the current test. If there are no subtests, then before doesn't run before anything - it just runs.
      Secondly: If test has no subtests, then before hook is completely useless, because it won't run before the code inside the test - that code is already running at that point. This is different from after, which you can actually register at any point during the execution and be sure it runs after your code.
  3. marco-ippolito commented on May 29, 2024

    @marco-ippolito
    Member

    I'm not sure I understand your point. I'll reopen to hear other collaborators opinion

  4. cjihrig commented on May 29, 2024

    @cjihrig
    Contributor

    I think the current behavior for subtests is correct - before() and after() should always run. For suites, I think we should do whatever other test runners do for before() and after().

  5. urugator commented on May 30, 2024

    @urugator
    Author

    My main problem is with suits, because unlike tests, they are not subject to --test-name-pattern filtering.
    So there is no way to prevent executing the code inside them, including before hook.

    I think the current behavior for subtests

    What do you mean by subtests? Don't you mean just tests?
    before should always run before any subtests, there is no question about that.
    The question is whether it should run for test, without any substests.
    Imo it should not. If there are no subtests, then you can't even sensibly define before what the hook is supposed to run:

    test('test without subtests', async () => {
      before(async () => {
        // runs after body1 and body2 (not before)
      })
    
      console.log('body1')
      
      before(() => {
        // runs after body1 (not before)
      });
    
      console.log('body2');
    })

    So you're saying it should always run, but I am asking before what it should always run?
    Now if a test does contain subtests, but none of them is executed, it's exactly equivalent to a test without subtests.

  6. urugator commented on May 30, 2024

    @urugator
    Author

    I think it basically boils down to whether before means before the current test or before subtests in the current test. I am arguing it can't refer to the current test, because current test is already running, so you can no longer schedule any job before it. So if before refers to subtests, then give me a practical example where you want to run before if none of the subtests are executed.
    The problem is asymmetry with after, which can actually refer to the currently running test. And indeed this is reflected in the documentation:

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

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

  7. cjihrig commented on May 31, 2024

    @cjihrig
    Contributor

    This appears to be fixed in v22. My guess is that #52221 being labeled semver-major is the reason it's not fixed in v20.

  8. cjihrig commented on Aug 13, 2024

    @cjihrig
    Contributor

    Closing per my last comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    test_runnerIssues and PRs related to the test runner subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions