fix: [M3-8602] - Fix plan-selection.spec.ts test failures on retries
#10976
Conversation
jdamore-linode
requested review from
bnussman-akamai and
coliu-akamai
and removed request for
a team
| /* Memoize popper callback to prevent unnecessary popper re-rendering. */ | ||
| const customPopper = useCallback( | ||
| (props: PopperProps) => { | ||
| return <CustomPopper {...props} sx={sxPopperComponent} />; | ||
| }, | ||
| [sxPopperComponent] | ||
| ); | ||
|
|
There was a problem hiding this comment.
This was a suggestion I saw on a StackOverflow thread, but hoping that somebody with more React expertise than me could take a look at this and double check that this is a sensible solution.
It seems to make the tests very happy, and I didn't notice any regressions during my manual testing, but I'd be lying if I said I fully understand the implications of wrapping this in useCallback.
There was a problem hiding this comment.
Hey Joe! Take a look at #10978. Do you think this could be a better way to approach it and would it fix the unexpected re-render / arrow function issue?
There was a problem hiding this comment.
Thank you @bnussman-akamai!
Commented on your PR, but I think your solution is hands down better: it fixes the test failures and I'm guessing it's a lot more idiomatic / in line with the way we normally do things. Happy to either integrate your implementation into this PR or let you take the reins with #10978!
There was a problem hiding this comment.
@jdamore-linode Do you happen to have the link to the StackOverflow thread? I'm curious why useCallback was used instead of useMemo and would be interested in reading more into it. Anyway, @bnussman-akamai provided a very good alternative and I see it's been merged.
There was a problem hiding this comment.
@carrillo-erik not on hand, no -- I could try to find it, but I was already a bit doubtful about useCallback as a solution for this, and I think Bank's solution demonstrates that it wasn't really necessary in the first place, so I don't think I'd consider the answer in that thread as authoritative in this case
jdamore-linode
changed the title
jdamore-linode
requested review from
AzureLatte
and removed request for
a team
| @@ -124,7 +124,7 @@ const notices = { | |||
|
|
|||
| authenticate(); | |||
| describe('displays linode plans panel based on availability', () => { | |||
| before(() => { | |||
| beforeEach(() => { | |||
There was a problem hiding this comment.
Using before here instead of beforeEach causes the mocks not to get applied on re-attempts which leads to failures.
The initial failure is caused by the cy.click() issue, but plan-selection.spec.ts failures are overrepresented in CI due to the re-attempts being guaranteed to fail.
|
Coverage Report: ✅ |
jdamore-linode
changed the title
plan-selection.spec.ts test failures on retries
…evert changes to `Autocomplete.tsx`
Description 📝
This fixes a minor test flaw in the
plan-selection.spec.tsspec where mocks are getting set up inbeforehooks instead ofbeforeEachhooks. This prevents mocks from being applied on test retries (because mocks get cleared between tests, butbeforehooks only get called once), causing retries in this spec to be guaranteed to fail.Changes 🔄
beforehooks withbeforeEachinplan-selection.spec.tsto ensure mocks get applied on test retriesTarget release date 🗓️
N/A
How to test 🧪
Reproduction steps
In
develop(edit: the issue causing the initial failure has been fixed, so reproducing this is now pretty difficult), build Cloud viayarn && yarn build && yarn start:manager:ciand then:repeat 5 yarn cy:run -s "cypress/e2e/core/linodes/plan-selection.spec.tsObserve flakiness (in my case, the spec failed 4 out of 5 times).
Verification steps
Check out this branch, build Cloud via
yarn && yarn build && yarn start:manager:ciand then:repeat 5 yarn cy:run -s "cypress/e2e/core/linodes/plan-selection.spec.tsObserve the test passes each time. I was able to run the test 50 times on repeat without failures.
We can also rely on CI to some extent -- this issue theoretically impacts all of our Autocompletes, so any test failures involving an Autocomplete (including region selects, etc.) should receive extra scrutiny.
Manual verification
Some extra attention should be given to the "Phone Verification" field at
/profile/authsince it's the only Autocomplete I'm aware of that takes advantage of the recent changes to thePopperComponentprop, so it may be at more risk of being broken by these changes than other Autocompletes.As an Author I have considered 🤔
Check all that apply
Autocompletechange since 10780 hasn't been released yet. Thoughts?