Skip to content

Mocha-like root hooks for test runner #54675

Description

@ericfortis

What is the problem this feature will solve?

Currently, before and after outside a describe run on every file, so projects that could have a global setup and teardown, such as server, database, or puppeteer browser, could run tests faster by executing a global before and after only once.

What is the feature you are proposing to solve the problem?

Ensuring before and after outside any test run only once, similar to Mocha Rook Hooks

What alternatives have you considered?

Alternatives:

  1. As is. It works, but it could be faster
  2. mocha

Activity

  1. avivkeller commented on Aug 31, 2024

    @avivkeller
    Member

    Could you write an example (demo code) of what you are looking for vs what is provided?

    Edit: Didn't mean to self assign, meant to apply test_runner label.

  2. added
    test_runnerIssues and PRs related to the test runner subsystem.
    on Aug 31, 2024
  3. ericfortis commented on Aug 31, 2024

    @ericfortis
    ContributorAuthor

    my-dir/setup.js

    import { before, after } from 'node:test'
    
    before(() => {
      console.log('LAUNCH my server only once')
    })
    after(() => {
      console.log('CLOSE my server only once')
    })

    my-dir/foo.test.js

    import { equal } from 'node:assert/strict'
    import { describe, it } from 'node:test'
    
    describe('foo', () => {
      it('1 is 1', () => equal(1, 1))
    })

    my-dir/bar.test.js

    import { equal } from 'node:assert/strict'
    import { describe, it } from 'node:test'
    
    describe('bar', () => {
      it('2 is 2', () => equal(2, 2))
    })
    node --test --import ./setup.js

    Its output shows that the before and after hooks get executed on every file.

    LAUNCH my server only once
    CLOSE my server only once
    ▶ bar
      ✔ 2 is 2 (0.349167ms)
    ▶ bar (0.671375ms)
    LAUNCH my server only once
    CLOSE my server only once
    ▶ foo
      ✔ 1 is 1 (0.367583ms)
    ▶ foo (0.696583ms)
    ℹ tests 2
    ℹ suites 2
    ℹ pass 2
    ℹ fail 0
    ℹ cancelled 0
    ℹ skipped 0
    ℹ todo 0
    ℹ duration_ms 65.239458
    

    The feature I propose is that before and after hooks that are outside describe run only once, similar to Mocha's global hooks.

    Desired output:

    LAUNCH my server only once
    
    ▶ bar
      ✔ 2 is 2 (0.349167ms)
    ▶ bar (0.671375ms)
    ▶ foo
      ✔ 1 is 1 (0.367583ms)
    ▶ foo (0.696583ms)
    ℹ tests 2
    ℹ suites 2
    ℹ pass 2
    ℹ fail 0
    ℹ cancelled 0
    ℹ skipped 0
    ℹ todo 0
    ℹ duration_ms 65.239458
    
    CLOSE my server only once
    
  4. avivkeller commented on Aug 31, 2024

    @avivkeller
    Member

    Correct me if I'm wrong, but wouldn't that be the same just running code before your test and then after without the use of hooks?

  5. ericfortis commented on Aug 31, 2024

    @ericfortis
    ContributorAuthor

    @redyetidev That example yes. Here's one that exports something (a browser page instance) from the setup to the tests.

    my-dir/setup.js

    import { launch } from 'puppeteer'
    import { before, after } from 'node:test'
    
    let browser, page
    
    before(async () => {
      console.log('LAUNCH puppeteer only once')
      browser = await launch()
      page = await browser.newPage()
    })
    after(() => {
      console.log('CLOSE puppeteer only once')
      browser?.close()
    })
    
    export default { page }

    my-dir/foo.test.js

    import { ok } from 'node:assert/strict'
    import { describe, it } from 'node:test'
    import P from './setup.js'
    
    describe('foo', () => {
      it('foo', () => ok(P.page.setViewport))
    })

    my-dir/bar.test.js

    import { ok } from 'node:assert/strict'
    import { describe, it } from 'node:test'
    import P from './setup.js'
    
    describe('bar', () => {
      it('bar', () => ok(P.page.setViewport))
    })
  6. avivkeller commented on Aug 31, 2024

    @avivkeller
    Member

    CC @nodejs/test_runner


    IMO theres not too much benefit to this feature, as it's pretty-much the equivalent of

    runStuffBeforeTesting()
    test() / run() / ...
    runStuffAfterTesting()

    But WDYT?

  7. ericfortis commented on Aug 31, 2024

    @ericfortis
    ContributorAuthor

    The benefit I propose is just speed, consider that runStuffBeforeTesting could be slow, such as launching puppeteer. Multiply that by 200 tests.

  8. ljharb commented on Sep 1, 2024

    @ljharb
    SponsorMember

    it sounds like you want a beforeAll, if before is actually beforeEach already?

  9. avivkeller commented on Sep 1, 2024

    @avivkeller
    Member

    it sounds like you want a beforeAll, if before is actually beforeEach already?

    I just don't see the benefit of that versus writing code before and after running tests? But it does seem like that's what is being asked.

  10. ericfortis commented on Sep 1, 2024

    @ericfortis
    ContributorAuthor

    @ljharb
    Yes, a beforeAll, or a context-sensitive before.
    Currently, a before in or outside a describe suite makes no difference. In Mocha, if it's outside a describe it's a root hook.


    @redyetidev
    Consider a setup that exports an instance, such as the second example. In that case, reusing the browser-page on every test saves a significant amount of time. I agree there's no functional benefit to this proposal, it's just a speed one.

  11. 1 remaining item

  12. avivkeller commented on Sep 1, 2024

    @avivkeller
    Member

    Have you tried the new isolation mode added in #53927 and released in Node v22.8.0? I think it would behave as you wantz

    FWIW this release is planned for tomorrow, so you won't be able to use this feature today (unless you build from main)

  13. cjihrig commented on Sep 1, 2024

    @cjihrig
    Contributor

    I think @mcollina is right here. Also, mocha root hooks are not global when run in parallel mode (which is similar to the default behavior in node:test). If anything, I think we'd want something similar to mocha's global fixtures, which is probably a duplicate of #49732.

  14. ericfortis commented on Sep 1, 2024

    @ericfortis
    ContributorAuthor

    It looks like before and after outside a describe suite do not get executed.

    $NODE --test  --experimental-test-isolation=none --import ./setup.js --test-concurrency=1
    ▶ bar
      ✔ bar0 (0.526125ms)
      ✔ bar1 (0.060583ms)
    ▶ bar (1.666959ms)
    ▶ foo
      ✔ foo0 (0.086416ms)
      ✔ foo1 (0.110709ms)
    ▶ foo (0.301167ms)
    ℹ tests 4
    ℹ suites 2
    ℹ pass 4
    ℹ fail 0
    ℹ cancelled 0
    ℹ skipped 0
    ℹ todo 0
    ℹ duration_ms 14.778708

    setup.js

    import { before, after } from 'node:test'
    before(() => { console.log('Before All') })
    after( () => { console.log('After All') })
    $NODE -v   # v23.0.0-pre (0c771c35)
  15. cjihrig commented on Sep 1, 2024

    @cjihrig
    Contributor

    It works with -r. There may be a bug specific to --import.

  16. cjihrig commented on Sep 1, 2024

    @cjihrig
    Contributor

    This appears to fix the issue related to --import and the existing test suite still passes:

    diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js
    index 9590ef8dcf..e2237dd73f 100644
    --- a/lib/internal/test_runner/runner.js
    +++ b/lib/internal/test_runner/runner.js
    @@ -34,6 +34,7 @@ const { spawn } = require('child_process');
     const { finished } = require('internal/streams/end-of-stream');
     const { resolve } = require('path');
     const { DefaultDeserializer, DefaultSerializer } = require('v8');
    +const { getOptionValue } = require('internal/options');
     const { Interface } = require('internal/readline/interface');
     const { deserializeError } = require('internal/error_serdes');
     const { Buffer } = require('buffer');
    @@ -697,6 +698,11 @@ function run(options = kEmptyObject) {
     
               root.harness.bootstrapPromise = promise;
     
    +          const userImports = getOptionValue('--import');
    +          for (let i = 0; i < userImports.length; i++) {
    +            await cascadedLoader.import(userImports[i], parentURL, kEmptyObject);
    +          }
    +
               for (let i = 0; i < testFiles.length; ++i) {
                 const testFile = testFiles[i];
                 const fileURL = pathToFileURL(testFile);
  17. ericfortis commented on Sep 1, 2024

    @ericfortis
    ContributorAuthor

    @cjihrig Thank you! Confirmed.

    after runs before the tests, but before is the important one and works well.

    $NODE --test  --experimental-test-isolation=none --import ./setup.js
    Before All
    After All
    ▶ bar
      ✔ bar0 (0.312333ms)
      ✔ bar1 (0.069458ms)
    ▶ bar (1.144417ms)
    ▶ foo
      ✔ foo0 (0.123875ms)
      ✔ foo1 (0.093833ms)
    ▶ foo (0.329041ms)
    ℹ tests 4
    ℹ suites 2
  18. avivkeller commented on Sep 1, 2024

    @avivkeller
    Member

    That might be a bug within itself.

  19. cjihrig commented on Sep 1, 2024

    @cjihrig
    Contributor

    That might be a bug within itself.

    To be clear, after() running before the tests is a bug.

  20. avivkeller commented on Sep 1, 2024

    @avivkeller
    Member

    To be clear, after() running before the tests is a bug.

    👍 I'll look into it

  21. avivkeller commented on Sep 1, 2024

    @avivkeller
    Member

    after runs before the tests, but before is the important one and works well.

    After looking into it, I don't think this is the case.

    import * as common from '../common/index.mjs';
    import * as fixtures from '../common/fixtures.mjs';
    import { test } from 'node:test';
    
    const testArguments = [
      '--test',
      '--experimental-test-isolation=none',
    ]
    
    const testFiles = [
      fixtures.path('test-runner', 'no-isolation', 'one.test.js'),
      fixtures.path('test-runner', 'no-isolation', 'two.test.js'),
    ]
    
    const hookFixture = fixtures.path('test-runner', 'no-isolation', 'global-hooks.js');
    
    const order = [
      'before(): global',
    
      'before one: <root>',
      'suite one',
    
      'before two: <root>',
      'suite two',
    
      'beforeEach(): global',
      'beforeEach one: suite one - test',
      'beforeEach two: suite one - test',
    
      'suite one - test',
      'afterEach(): global',
      'afterEach one: suite one - test',
      'afterEach two: suite one - test',
    
      'beforeEach(): global',
      'beforeEach one: test one',
      'beforeEach two: test one',
      'test one',
    
      'afterEach(): global',
      'afterEach one: test one',
      'afterEach two: test one',
    
      'before suite two: suite two',
      'beforeEach(): global',
      'beforeEach one: suite two - test',
      'beforeEach two: suite two - test',
    
      'suite two - test',
      'afterEach(): global',
      'afterEach one: suite two - test',
      'afterEach two: suite two - test',
    
      'after(): global',
      'after one: <root>',
      'after two: <root>',
    ]
    
    test('Using --require to define global hooks works', async (t) => {
      const spawned = await common.spawnPromisified(process.execPath, [
        ...testArguments,
        '--require', hookFixture,
        ...testFiles
      ]);
    
      t.assert.ok(spawned.stdout.includes(order.join('\n')));
    });

    I'm opening a PR now to fix the --import and add this test

  22. ericfortis commented on Sep 1, 2024

    @ericfortis
    ContributorAuthor

    @redyetidev I see what you meant. I didn't know that importing from the setup.js had no side effects.

    Thank you for your assistance.

  23. avivkeller commented on Sep 13, 2024

    @avivkeller
    Member

    Can this be closed?

  24. cjihrig commented on Sep 14, 2024

    @cjihrig
    Contributor

    I think we can close this in favor of #49732.

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

    feature requestIssues requesting new Node.js features.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