-
Notifications
You must be signed in to change notification settings - Fork 319
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
OKTA 561098 : Reminder element rerender timing fix #3077
OKTA 561098 : Reminder element rerender timing fix #3077
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason setSystemTime does not exist on the jest type
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can you try omitting the window
part and see what happens? It seems like it isn't necessary and they are equivalent.
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: