Skip to content

Conversation

@190n
Copy link
Contributor

@190n 190n commented Apr 28, 2025

What does this PR do?

Fixes #19200

How did you verify your code works?

Added to node-test.test.ts

@robobun
Copy link
Collaborator

robobun commented Apr 28, 2025

Updated 11:21 AM PT - Jul 23rd, 2025

@190n, your commit 58de5d3 is building: #21284

@jordanebelanger
Copy link

jordanebelanger commented Apr 29, 2025

@190n If this can help you potentially debug, I can still make the tests be skipped under certain circumstances, take this snippet for example:

import { test, after } from 'node:test'
import { strict as assert } from 'node:assert'
import { build } from '../helper'

import type { FastifyInstance } from 'fastify'

after(async () => {
  // Cleanup logic if needed
})

test('resource routes', async () => {
  const app: FastifyInstance = await build()

  await test('should create and update a resource', async () => {
    throw new Error('this should fail the test')
  })
})

When this is ran, the inner test does not get executed (doesn't show up in the console list nor is the thrown error catched/visible)

@190n
Copy link
Contributor Author

190n commented Apr 29, 2025

It looks like we don't intend to support that yet:

bun/src/js/node/test.ts

Lines 146 to 148 in 3278969

if (this.#insideTest) {
throwNotImplemented("test() inside another test()", 5090, "Use `bun:test` in the interim.");
}

I think this will be an effort for a different PR. However, I will see if I can figure out why the error isn't being thrown, since that could probably be confusing.

@190n
Copy link
Contributor Author

190n commented Apr 29, 2025

@jordanebelanger thanks for bringing this up, the issue was we would only throw the error if you used the context, e.g. using t.test in test("outer", t => t.test("inner", ...)). I've fixed this and added a test that we emit the error here.

We'll add support for nested tests someday but that's outside the scope of this PR. I think providing an error in this case is still a worthwhile improvement over status quo.

@190n 190n requested review from a team and dylan-conway and removed request for a team May 1, 2025 22:41
@190n 190n requested a review from dylan-conway May 5, 2025 22:21
Copy link
Member

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments, looks good afterwards

@190n 190n requested a review from dylan-conway May 6, 2025 18:02
dylan-conway
dylan-conway previously approved these changes May 6, 2025
@190n 190n requested review from a team and alii and removed request for a team May 6, 2025 19:51
@190n 190n requested review from a team and removed request for alii May 12, 2025 19:03
@190n 190n requested review from dylan-conway and pfgithub July 22, 2025 01:36
@dylan-conway dylan-conway merged commit a1f756f into main Jul 24, 2025
57 of 62 checks passed
@dylan-conway dylan-conway deleted the ben/node-test-multiple branch July 24, 2025 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:test only one test file is executed, other files are ignored

7 participants