[No QA] Add characterization tests for NumberWithSymbolForm - #98292
Conversation
| // Quirk locked in on purpose: `addLeadingZero('-.', true)` returns `-0-.` (it prepends `-0` to the whole | ||
| // string instead of inserting the zero after the sign), so the value fails validation and is dropped. |
There was a problem hiding this comment.
nit/question: as I understand it, these quirk tests exist to preserve compatibility during the refactor - are we planning to drop them or fix the behavior to the "proper" one once all the migration steps are done? 😄
There was a problem hiding this comment.
Yes, these tests just freeze current behavior so the refactor doesn’t change it by accident. We’ll keep them through the migration and then fix it in a follow-up
|
1 minor NIT comment and 1 question regarding future steps - otherwise LGTM 🙏 |
dariusz-biela
left a comment
There was a problem hiding this comment.
Everything else looks fine
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
This makes me think that it would be great if we had a visual regression testing tool to catch unintentional visual changes when doing this kind of refactor. |
|
@bernhardoj Thanks for the feedback, I've addressed all of your code comments Regarding the visual regression tool, I like the idea and agree it would be helpful for catching unexpected UI shifts in such refactors. However, setting it up might be a bit tricky without a clear spec or specific toolchain in mind though. A proposal would be the best way forward if you want to run with this. |
Yeah. I don't mean to do it here, just wanna share my thought haha |
|
@bernhardoj can you add checklist please? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@mountiny Completely forgot about the checklist. Added now. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Adds characterization UI tests for
NumberWithSymbolFormahead of the composition refactor.NumberWithSymbolFormpreviously had no dedicated coverage; only the portrait path was exercised indirectly via other screens. This PR addstests/ui/NumberWithSymbolFormTest.tsxand locks current behavior for all three render paths plus shared contracts, with no production code changes:displayAsTextInput— plainTextInput(no pad /ScrollView),setFormattedNumber/addLeadingZero, internal flip viahandleFlipPress, currency button propssetNewNumber, flip via callertoggleNegative, currency viaonSymbolButtonPressNumberWithSymbolFormRef(getNumber/updateNumber/clearSelection), selection handling, validation (maxLength/decimals/ negatives),clearNegativeon backspace of an empty negative value, ScrollView rendering and button interactionsThese tests are the regression net for later migration batches.
Fixed Issues
$ #98168
PROPOSAL #97970
Tests
Run
npm run test -- tests/ui/NumberWithSymbolFormTest.tsxand confirm all suites passConfirm the suite covers:
displayAsTextInputpath (no pad / ScrollView; leading zero; flip; currency)toggleNegative, currency)getNumber,updateNumber,clearSelection)clearNegativeon empty negative backspaceConfirm
src/components/NumberWithSymbolForm.tsxis unchanged (diff is test-only)Verify that no errors appear in the JS console
Offline tests
N/A — test-only change; no runtime / network behavior.
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.