Test logs improvements #17477
Test logs improvements #17477
Conversation
jbazant
added
the
no-project
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
trezor-ci
force-pushed
the
test-logs-improvements
branch
from
81c3a36 to
14fc0fd
Compare
|
🚀 Expo preview is ready!
|
WalkthroughThe pull request introduces multiple changes across configuration, testing, and module behavior. The Jest configuration is updated to include a new setup script that provides polyfills for the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/test-utils/src/everstakeJestSetup.js (1)
1-5: Improve setImmediate polyfill to return timeout IDThe setImmediate polyfill should return the timeout ID so it can be properly cleared with clearImmediate.
if (!window.setImmediate) { window.setImmediate = function (callback) { - setTimeout(callback, 0); + return setTimeout(callback, 0); }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
jest.config.native.js(1 hunks)packages/suite/src/support/tests/hooksHelper.tsx(2 hunks)patches/README.md(1 hunks)patches/expo-modules-core+2.0.6.patch(1 hunks)suite-native/test-utils/src/everstakeJestSetup.js(1 hunks)suite-native/test-utils/src/expoMock.js(0 hunks)
💤 Files with no reviewable changes (1)
- suite-native/test-utils/src/expoMock.js
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (5)
jest.config.native.js (1)
32-32: Enhances Jest setup with immediate function polyfillsThis addition incorporates the newly created everstakeJestSetup.js file that provides polyfills for setImmediate and clearImmediate functions in the testing environment. A good enhancement to support proper asynchronous testing behavior.
patches/README.md (1)
16-19: Documentation of the expo-modules-core patch is clear and helpfulGood documentation of the patch that's being applied, including references to the relevant GitHub issues. This helps other developers understand why this patch is necessary and what problem it's solving.
patches/expo-modules-core+2.0.6.patch (1)
9-13: Patch successfully removes unnecessary warningThis patch appropriately removes the warning about the undefined process.env.EXPO_OS that was cluttering the test logs. This aligns perfectly with the PR objective of improving test logs by reducing unnecessary warnings.
suite-native/test-utils/src/everstakeJestSetup.js (1)
7-11: clearImmediate polyfill implementation is correctThe clearImmediate polyfill correctly uses clearTimeout to cancel the timer, which works well with the setImmediate implementation using setTimeout.
packages/suite/src/support/tests/hooksHelper.tsx (1)
87-87:❓ Verification inconclusive
Properly wrapped user interactions with act() to reduce test warnings.
These changes follow React Testing Library best practices by wrapping user interactions with
act(), which ensures that all state updates triggered by these interactions are properly processed before the test continues. This will help reduce "Warning: An update to Component inside a test was not wrapped in act(...)" messages in the test logs, aligning with the PR objective of improving test logs.While these changes are correct, it's worth noting that in recent versions of
@testing-library/react, user events are often already wrapped withact()internally. To verify if this double wrapping is necessary in your case:Also applies to: 95-95, 102-102, 104-104, 108-108
🏁 Script executed:
#!/bin/bash # Check the version of @testing-library/react and related packages grep -A 5 "@testing-library/react" package.json || npm list @testing-library/react @testing-library/user-event --depth=0Length of output: 170
Action Required: Verify Redundant act() Wrapping Based on Dependency Versions
The modifications in
packages/suite/src/support/tests/hooksHelper.tsxcorrectly wrap user interactions withinact(), which is aligned with best practices for reducing test warnings. However, since the attempted dependency check for@testing-library/reactand@testing-library/user-eventdid not yield any version information, we cannot conclusively determine whether these user events are already auto-wrapped in your current setup.Please manually verify that the installed versions of these libraries do not already provide internal
act()wrapping. If they do, you might consider removing the explicitact()wrapper in these cases to avoid redundancy. This concern applies to the following lines in the file:
- 87
- 95
- 102
- 104
- 108
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13724624961 |
trezor-ci
force-pushed
the
test-logs-improvements
branch
from
26b7824 to
120a26e
Compare
Description
Should get rid of some warnings that were bloating our tests pipeline log.
Not ideal solution though.
Related Issue
Resolve
Screenshots: