-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[No QA] IOURequestStepScan clean-up, Phase 1: Add comprehensive test coverage for multi-scan receipt handling
#80628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] IOURequestStepScan clean-up, Phase 1: Add comprehensive test coverage for multi-scan receipt handling
#80628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds automated tests intended to act as a safety net for upcoming IOURequestStepScan refactoring work, focusing on multi-scan receipt handling and draft transaction preservation.
Changes:
- Added a new UI test suite for
IOURequestStepScan - Added assertions around Onyx draft transaction persistence and
TransactionUtils.shouldReuseInitialTransactionbehavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
tests/ui/IOURequestStepScanTest.tsx
Outdated
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('Receipt and Draft State Management', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is the only nested describe, I think we should remove it. The reason being the beforeEach and afterEach will treat the describe block as one test. So Onyx.clear isn't actually running between each test, which can cause OOM errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, Thanks for catching that! Removed the nested describe block so beforeEach and afterEach now run between each test, to ensure proper Onyx.clear() cleanup.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
@roryabraham Looking at all test cases, looks like we just do some onyx merge and verify that it's merged. Is that what we really want to test? |
|
@bernhardoj you're right - I should have review these tests more carefully. They are not actually testing anything at all it seems. |
|
Thanks @bernhardoj for the careful review, you're right, the tests weren't actually asserting component behavior. I'm working on follow-up to update the tests so they render the Additionally when two draft transactions exist and the user "replaces" the first receipt with a new file, The tests will validate the component's behavior, not just that Onyx merge works. cc: @roryabraham |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.11-0 🚀
|
Explanation of Change
This PR implements Phase 1 of the
IOURequestStepScancleanup by adding a comprehensive test safety net to prevent regressions during refactoring.Fixed Issues
$ #79929
PROPOSAL: N/A
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari