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

Modify Profile Test to Fix Intermittent Failures #154

Merged
merged 6 commits into from
Aug 19, 2021
Merged

Conversation

hreineck
Copy link
Contributor

Changed the profile test to use waitFor() instead of an await
on findByText().
Occasionally, certain conditions caused findByText() to throw an error,
likely due to the long load times for the imported controls in the
profile viewer. The errors caused the tests to fail even though the
code should pass.
This change should resolve these errors as waitFor() retries getByText()
until it does not throw an error, more resiliently handling the
conditions that caused findByText() to throw errors.

Changed the profile test to use waitFor() instead of an await
on findByText().
Occasionally, certain conditions caused findByText() to throw an error,
likely due to the long load times for the imported controls in the
profile viewer. The errors caused the tests to fail even though the
code should pass.
This change should resolve these errors as waitFor() retries getByText()
until it does not throw an error, more resiliently handling the
conditions that caused findByText() to throw errors.
Cleaned up the imports and addressed a couple linter issues.
@hreineck
Copy link
Contributor Author

Might need some help testing this one - this change seems to work but I may just be getting really lucky (unlucky?) with the tests passing all the time. Before the fix, the test giving a false negative was pretty rare, so if anyone sees that behavior occur again even with this change please drop a comment in this PR.

@hreineck hreineck self-assigned this Aug 16, 2021
@hreineck hreineck linked an issue Aug 16, 2021 that may be closed by this pull request
@snorouzzadeh
Copy link
Contributor

I do not see any behavior occur during my testing. I think this should be good to go.

@hreineck
Copy link
Contributor Author

Turned up some other weird issues while testing; seems that the explicit timeout of 10000 ms doesn't actually do anything. A failing test will now return this output:
image
This is because jest sets a timeout for every test that takes precedence over the timeout field of waitFor()
As the error message says this can be remedied by using jest.setTimeout() to make that timeout higher than 10000ms, which gives this error instead:
image
I believe the second error message is preferable on a failed test because it's a bit more descriptive of the actual error.
For now I'm going to reduce the explicit timeouts to 5000ms and call jest.setTimout(6000)because that produces the more descriptive error and 5000ms seems to be enough time anyway. But if there's a better way of doing this, or a more preferable timeout, please let me know.
This may also be something to look into if it affects the other tests as well.

Change the timeouts on the profile test to explicity be 5000ms.
Before, the explicit timeout was longer than the default test
timeout of 5000ms set by jest. This was causing a less descriptive
error message to appear on a failed test.
This change sets the timeouts to 5000ms and sets the overall test
timeout to the slightly higher 6000ms in order to keep the optimal
timeout time of 5000ms while also producing the correct error message
on a failed timeout.
@mikeisen1
Copy link
Contributor

I noticed that you didn't use waitFor on the other tests in the profile test file. Why not use waitFor for findByText and findAllByText as well?

When testing the code for this branch, I did not see any profile test failures, but still am curious as to the above question.

@hreineck
Copy link
Contributor Author

I noticed that you didn't use waitFor on the other tests in the profile test file. Why not use waitFor for findByText and findAllByText as well?

When testing the code for this branch, I did not see any profile test failures, but still am curious as to the above question.

Good point. I only changed what was needed to fix the tests, but it would be better if the whole test was consistent. I'll make that change now.

Modify the tests within the profile test to all use waitFor()
so that the three tests are consistent with each other.
Copy link
Contributor

@mikeisen1 mikeisen1 left a comment

Choose a reason for hiding this comment

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

The profile test does not fail and everything seems good to go.

Added a missing `uuid` prop to component definition test data
to resolve the "missing key prop" warning.
The component ControlImplementationImplReq was expecting this
uuid to be passed down so it can be used as the "key" in its
list of elements, but it was missing.
Copy link
Contributor

@snorouzzadeh snorouzzadeh left a comment

Choose a reason for hiding this comment

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

No test is failing. This looks good to me. Great work!

@mikeisen1 mikeisen1 merged commit f014540 into develop Aug 19, 2021
@rgauss rgauss deleted the EGRC-433 branch August 25, 2021 13:52
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.

Intermittent Failure of Profile Test
3 participants