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

#1179: Specify default ontimeout at configuration level #1272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Areiser
Copy link

@Areiser Areiser commented Oct 11, 2023

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.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 11, 2023

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:

Sandbox Source
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a7b7252) 100.00% compared to head (a1adc19) 100.00%.

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     
Flag Coverage Δ
node-14 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)
node-18 100.00% <100.00%> (ø)
node-20 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@timdeschryver timdeschryver left a 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.

@@ -43,6 +43,23 @@ test('fake timer timeout', async () => {
).rejects.toMatchInlineSnapshot(`[Error: always throws]`)
})

test('fake timer timeout uses default ontimeout', async () => {
configure({
Copy link
Member

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

configure(originalConfig)

Copy link
Author

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.

@MatanBobi
Copy link
Member

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?

@timdeschryver
Copy link
Member

@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.

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.

3 participants