-
Notifications
You must be signed in to change notification settings - Fork 469
#1179: Specify default ontimeout at configuration level #1272
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
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a1adc19:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@ ## main #1272 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 24 Lines 1038 1040 +2 Branches 346 347 +1 ========================================= + Hits 1038 1040 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
This LGTM, but I have a comment about the test case.
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.
Since this updates global state, we should also revert it back to it's original state to prevent these changes from leaking through other test cases.
As an example see
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.
Thanks for the hint, I have reset the configuration after the tests as in the example you provided.
0e528c4 to
a1adc19
Compare
Thanks for this contribution. I'm hesitating whether this is a use-case that should be built-in in the library. Do we believe users will want to configure it on their own? Why not just add a hint that it was caused by a timeout in our codebase and not make it configurable?
@MatanBobi both can be useful.
Modifying the message can be helpful to a broader audience (is this a breaking change)?
But since we already allow that the message can be overwritten for single cases, I think having a global config for it is OK (as with other config properties). It's also a relative small change that doesn't have an impact to a big area.
What:
Make it possible to override the default onTimeout method for all waitFor methods in the config.
Why:
We use the testing library in our organization and would like to print a clear user information that the test ran into a timeout, since we sometimes struggle with flaky tests. Only printing the error leads some users to think that the condition did not time out.
This is my first contribution, I added a test, but if there are any change requests I'll gladly update this PR, as the feature would be very helpful for us.