[NoQA] Add useNewTransactions tests#64938
Conversation
|
@ahmedGaber93 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] |
| transactions: [], | ||
| }, | ||
| }); | ||
| await delay(10); // We need to wait to ensure that usePrevious hook updates correctly |
There was a problem hiding this comment.
I don't get why we need this delay 🤔. Deleting it fails the test for me, but it works fine when I test it externally — so I think something might be not correct.
function useTestHook(value = 0) {
const prev = usePrevious(value);
return prev;
}
it('should return the previous value', () => {
const {rerender, result} = renderHook((props) => useTestHook(props.value), {
initialProps: {
value: 0,
},
});
rerender({value: 1});
expect(result.current).toEqual(0);
rerender({value: 2});
expect(result.current).toEqual(1);
});There was a problem hiding this comment.
Interestingly enough if I remove this line:
await delay(10); // We need to wait to ensure that usePrevious hook updates correctlythe tests pass for me. I tested multiple times and it worked every time 🤷
There was a problem hiding this comment.
I added this particular delay just to keep all the tests consistent with each other and to be 100% sure that useEffect manages to be run. It doesn't specifically say anywhere that rerender awaits for all the useEffects to be finished, so I think it's better to wait that extra delay jsut to be sure.
It may be that on your machine there is some race condition and that's why the tests fail.
What's more deleting this line also doesn't break the test line in @Kicu case.
Are you deleting only this one particular delay, or every delay in the code, or every first delay?
There was a problem hiding this comment.
Aha! If the case is a race condition, I totally agree we should keep it.
Are you deleting only this one particular delay, or every delay in the code, or every first delay?
I removed all of them.
The case that failed with me before is in line 141, and it seems not related to usePrevious, because I also removed delay(1000) that related to skipFirstTransactionsChange by mistake while I was testing. So now all test cases are passed without delay(10) In my machine.
What is the test result with you after removing it?
There was a problem hiding this comment.
I've thought it through one more time and I decided to delete the delay(10). The reason I added it was a race condition that I thought would happen, but I double checked it and it actually doesn't happen.
The useNewTransaction works as expected in all the test cases so I think it's safe to delete usePrevious related delays, thanks for noticing that 😄
Kicu
left a comment
Kicu
left a comment
There was a problem hiding this comment.
Good job 👍
I think these are very good tests because I didn't have to go into implementation details of the function, yet I understood just by looking at test steps what should be happening in the code.
You can take a look at my comments and perhaps consider the await delay that the other reviewer pointed out, but on my end this looks pretty nice 👍
| transactions: [], | ||
| }, | ||
| }); | ||
| await delay(10); // We need to wait to ensure that usePrevious hook updates correctly |
There was a problem hiding this comment.
Interestingly enough if I remove this line:
await delay(10); // We need to wait to ensure that usePrevious hook updates correctlythe tests pass for me. I tested multiple times and it worked every time 🤷
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
mountiny
changed the title
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks! Great to have more tests like this
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.74-2 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.1.74-10 🚀
|
4 participants
Explanation of Change
Fixed Issues
$ #64936
PROPOSAL:
Tests
No QA
Offline tests
QA Steps
[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))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
MacOS: Desktop