OKTA 561098 : Reminder element rerender timing fix #3077
Conversation
leemchale-okta
changed the title
leemchale-okta
requested review from
glenfannin-okta,
lesterchoi-okta,
shawyu-okta and
shuowu-okta
as code owners
| act(() => { | ||
| jest.advanceTimersByTime(DEFAULT_TIMEOUT_MS); | ||
| // @ts-expect-error setSystemTime does not exist on type 'typeof jest' | ||
| jest.setSystemTime(Date.now() + DEFAULT_TIMEOUT_MS); |
There was a problem hiding this comment.
for some reason setSystemTime does not exist on the jest type
There was a problem hiding this comment.
I think we need to update @types/jest version to match the jest version. This should solve all of the type errors that we're ignoring @ts-expect-error
| beforeEach(() => { | ||
| // Mock system time for triggering resend email reminder element | ||
| let mockSystemTimeStamp = Date.now(); | ||
| Date.now = jest.fn(() => mockSystemTimeStamp); |
There was a problem hiding this comment.
Using jest.useFakeTimers('modern') here breaks our integration test and causes tests to fail. There seems to be an issue/conflict with the 'modern' fake timer and async calls. Upgrading to newer jest version did not resolve the issue so it seems like a deeper issue that requires more investigation. Possibly related info:
jestjs/jest#11103
jestjs/jest#13018
So I opted to mock the Date.now method here for now since we have more control and behavior is more predictable.
leemchale-okta
force-pushed
the
LM-reminder-element-rerender-fix-OKTA-561098
branch
from
cb20598
to
0f0a690
Compare
...v3/test/integration/__snapshots__/authenticator-reset-password-revoke-sessions.test.tsx.snap
Outdated
Show resolved
Hide resolved
src/v3/test/integration/__snapshots__/authenticator-verification-email.test.tsx.snap
Outdated
Show resolved
Hide resolved
src/v3/test/integration/authenticator-verification-email.test.tsx
Outdated
Show resolved
Hide resolved
src/v3/test/integration/authenticator-verification-email.test.tsx
Outdated
Show resolved
Hide resolved
| if (timerRef) { | ||
| window.clearTimeout(timerRef.current); | ||
| window.clearInterval(timerRef.current); |
There was a problem hiding this comment.
What happens when you omit the window part.
Based on light research, window.clearInterval and clearInterval are equivalent so you should be able to leave off the window part
| const timeout = typeof customTimeout === 'number' ? customTimeout : DEFAULT_TIMEOUT_MS; | ||
|
|
||
| timerRef.current = window.setTimeout(() => setShow(true), timeout); | ||
| timerRef.current = window.setInterval(() => { |
There was a problem hiding this comment.
Same here, can you try omitting the window part and see what happens? It seems like it isn't necessary and they are equivalent.
src/v3/test/integration/authenticator-verification-email.test.tsx
Outdated
Show resolved
Hide resolved
Description:
This PR fixes the issue where the Reminder element timer is reset on a re-render, causing the user to have to wait another 30 seconds.
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Downstream Monolith Build: