Refactor makeTestResults function to use a stack for traversal #14760
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@SimenB thank you for review, I've just added a unit test. Here as an example how the test fails with the current implementation: Failed test output FAIL packages/jest-circus/src/__tests__/utils.test.ts
✕ does not thrown a stack overflow exception (3391 ms)
● does not thrown a stack overflow exception
Command failed with exit code 1: node /private/var/folders/hg/0p1p7xx91x3bf6j2jc47rf6m0000gp/T/76b8add5a46335ec6384215a9ba62432
/Users/mbelsky/gh/jest/packages/jest-circus/src/utils.ts:323
testResults.push(...makeTestResults(child));
^
RangeError: Maximum call stack size exceededOne issue I noticed: the new unit test is running for 5 seconds and that's quite slow. Do you think there is a more optimal way to test it? |
|
Thanks! Could you add the missing copyright header? You can add the minimal one found in other files 🙂 |
CHANGELOG.md
Outdated
| @@ -44,6 +44,7 @@ | |||
| - [**BREAKING**] Changes `testPathPattern` configuration option to `testPathPatterns`, which now takes a list of patterns instead of the regex. | |||
| - [**BREAKING**] `--testPathPattern` is now `--testPathPatterns` | |||
| - `[jest-reporters, jest-runner]` Unhandled errors without stack get correctly logged to console ([#14619](https://github.com/jestjs/jest/pull/14619)) | |||
| - `[jest-circus]` Replace recursive `makeTestResults` implementation with iterative one ([#14760](https://github.com/jestjs/jest/pull/14760)) | |||
There was a problem hiding this comment.
Could you put this alphabetically?
There was a problem hiding this comment.
I've put it higher and hope it's the right place 😅
sure, it's there now! |
I'm not sure - unless it does a lot of work, the test doesn't prove we don't end up with a SO. So I think it's fine 👍 |
There was a problem hiding this comment.
oooh, wait, I don't want a file that's almost 1M lines 😅 can we do expect(stdout.split('\n')).toHaveLength(100_000); or whatever the expectation is?
There was a problem hiding this comment.
yeah, I wasn't sure what approach is better here for this scenario 😅 Updated, thank you!
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
It fixes #14759
Test plan
All existing tests must pass