fix: propagate chained Suite#timeout() to already-registered hooks#6104
Open
chatman-media wants to merge 1 commit into
Open
fix: propagate chained Suite#timeout() to already-registered hooks#6104chatman-media wants to merge 1 commit into
chatman-media wants to merge 1 commit into
Conversation
Suite#timeout(ms) retroactively applies the new timeout to child tests and suites (added in mochajs#5612 for mochajs#5422), but it skipped the four hook arrays (_beforeAll, _beforeEach, _afterAll, _afterEach). As a result, the chained form describe('foo', () => { before(async () => { /* slow setup */ }); it('bar', () => {}); }).timeout(10000); did not raise the timeout for the hooks: they were created during the describe callback with the suite's timeout at that moment (the default), and the subsequent .timeout() call never reached them. The equivalent function() form (this.timeout(N) inside the callback) and the inner it() were unaffected, making the behaviour inconsistent. Iterate the hook arrays alongside tests/suites in the setter so hooks are Runnable instances and already expose a .timeout() setter. Closes mochajs#6033
|
|
Contributor
|
👋 Hi @chatman-media, thanks for the pull request! A scan flagged a concern with it. Could you please take a look? [pr-task-completion] This PR's body is missing
Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a
|
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.
PR Checklist
status: accepting prs— it is currentlystatus: in triage; happy to wait for triage, opening early since the fix is small and self-containedOverview
Suite#timeout(ms)retroactively applies the new timeout to child tests and suites (added in #5612 for #5422), but it only iteratesthis.testsandthis.suites— it skips the four hook arrays (_beforeAll,_beforeEach,_afterAll,_afterEach).So the chained form does not raise the timeout for hooks:
The hooks are created during the
describecallback with the suite's timeout at that moment (the default), and the subsequent.timeout(10000)call never reaches them. The innerit()and thefunction () { this.timeout(N); ... }form both work, which makes the behaviour inconsistent (see the table in #6033).Fix
Iterate the hook arrays alongside
tests/suitesin the setter. Hooks areRunnableinstances and already expose a.timeout()setter, so this mirrors the existing loops exactly:Tests
Added a unit test in
test/unit/suite.spec.cjsundertimeout() > when argument is passedthat registers one hook of each kind, callssuite.timeout(5000), and asserts all four hooks report5000.expected 2000 to be 5000.test/unit/timeout.spec.cjs, 13 passing) remain green;eslintclean.