TestContext not passed to hooks in node:test - #26185
Closed
jeansouzak wants to merge 2 commits into
Closed
Conversation
Contributor
WalkthroughModifies hook creation to accept and propagate TestContext through hook functions. Updates HookFn type signature to accept an optional TestContext parameter, allowing hooks to optionally receive test context during execution. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Thank you for this contribution! This was fixed in #32631: Closing as already fixed. Thanks again for the investigation and patch! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #26170
What does this PR do?
This PR fixes a bug where
TestContextwas not being passed to hook functions when usingt.after(),t.afterEach(),t.before(), andt.beforeEach()in thenode:testmodule.The Problem
When registering hooks via
TestContextmethods (e.g.,t.after((ctx) => ctx.diagnostic(...))), the hook function was not receiving theTestContextas a parameter, causingTypeError: undefined is not an object (evaluating 't.diagnostic')errors.The Solution
The fix modifies the
createHook()function to accept an optionalTestContextparameter and pass it to the hook function when available. The changes include:Updated hook methods to pass
this(theTestContextinstance) tocreateHook():TestContext.before()TestContext.after()TestContext.beforeEach()TestContext.afterEach()Modified
createHook()function to accept an optionalcontext?: TestContextparameter and pass it to the hook function when calling it.Updated type definition for
HookFnto accept an optionalTestContextparameter:(ctx?: TestContext) => unknown | Promise<unknown>Maintained backward compatibility - hooks that don't expect a context parameter continue to work as before.
Example
Before (broken):
After (fixed):
How did you verify your code works?
Created comprehensive tests in
execution-test/test-context-hooks.test.tsthat verify:after()hook receivesTestContextcorrectlyafterEach()hook receivesTestContextcorrectlyVerified the original bug is fixed:
bun bd test ./execution-test/test.test.tsThe test that previously failed with
TypeError: undefined is not an object (evaluating 't.diagnostic')now passes successfully.Ran all new tests:
bun bd test ./execution-test/test-context-hooks.test.tsAll 8 tests pass, confirming that:
TestContextis correctly passed to hooksTested with the original issue reproduction case:
This now works correctly, printing both "Hello World" and "finished running example test" as expected.
Changes Made
src/js/node/test.ts:TestContext.before(),after(),beforeEach(), andafterEach()to passthistocreateHook()createHook()function signature to accept optionalcontext?: TestContextparameterresult = context ? fn(context) : fn()HookFntype to accept optionalTestContextparameterRelated Issue
#26170