Skip to content
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

✅ DOP-4302 handles test with lazy loading a component #1005

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

caesarbell
Copy link
Collaborator

@caesarbell caesarbell commented Feb 7, 2024

Stories/Links:

DOP-4302

Current Behavior:

Flaky test

Staging Links:

Passing test

Notes:

It seems according to Jest's test method documentation, there is a way of making a test wait for the promise to resolve before letting the test complete.

In our case, the solution is to return the promise from the waitFor async function from RTL. Since this test now waits for the promise to resolve, we can remove the optional timeout.

@caesarbell caesarbell marked this pull request as ready for review February 7, 2024 20:23
@caesarbell
Copy link
Collaborator Author

caesarbell commented Feb 7, 2024

I re-ran the jobs about three times, each one passing, assuming that would be equivalent to pushing up multiple commits to trigger the builds. Each time, the test passes.

To be extra sure, I even pushed up an empty commit, to trigger the builds to see if tests contniue to pass.

@@ -58,12 +62,7 @@ describe('DocumentBody', () => {
},
{ timeout: 8000 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about returning waitFor would be the magic bullet. But wondering if that means we still need the timeout or no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ btw, I'm totally fine with it!! Just wondering if it's necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmeigs good point. I know we don't need the test method's option timeout param, however, the timeout option is still needed for the waitFor method. waitFor runs the callback a number of times until the timeout is reached, and the default value is 1000ms. In our use case we need 8000ms, but we won't need to set the timeout in the test method.

@caesarbell caesarbell merged commit f22eeea into main Feb 8, 2024
4 checks passed
@caesarbell caesarbell deleted the DOP-4302 branch February 8, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants