change `waitFor` brace style, switch `interval` default #10662

phryneas · 2023-03-17T09:20:13Z

do not merge before #10661 has been merged

This PR changes from

waitFor(() => {
  expect(...)
}, {interval: 1}
)

to

waitFor(() => expect(...))

in a ton of places,
and in return replaced

waitFor(() => {
  expect(...)
})

with

waitFor(
  () => expect(...),
  {interval: 50}
)

We used interval: 1 a lot more often than skipping the parameter, and like this, it makes it much more obvious where we still had the slower default. We might want to revisit those places.

I've also highlighted some waitFor instances that just seem wrong, but I didn't change any runtime behaviour of the tests - that's for another PR.

I've also dropped the method body braces where there was only one statement in waitFor, as that compacts the tests a bit.

do not merge before #10661 has been merged

changeset-bot · 2023-03-17T09:20:17Z

⚠️ No Changeset found

Latest commit: f445736

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

lhmzhou

LGTM

benjamn

Yay for reducing test boilerplate!

Were you able to automate this with a codemod or editor tricks? If it was all by hand, we should try to get these changes merged sooner, so you don't have to keep rebasing.

alessbell

🎉🎉

jerelmiller

I'm gonna be the bad guy here 😜. Check out my comments and let me know what you think.

I'd also like to float the question: is this PR still relevant? We've had it open for some time now unmerged and wasn't sure if thats because it was forgotten or not.

jerelmiller · 2023-07-06T04:26:24Z

src/react/hooks/__tests__/useSuspenseQuery.test.tsx

@@ -5260,11 +5250,11 @@ describe("useSuspenseQuery", () => {
      },
    });

-    await waitFor(() => {
+    await waitFor(() =>


Any reason these needed to change? Perhaps I'm picky/selfish, but this seems like a change for the sake of change. I'd prefer to leave these alone for now.

jerelmiller · 2023-07-06T04:27:39Z

src/react/hooks/__tests__/useSuspenseQuery.test.tsx

I'll be the bad guy and say I'm not sure I see much meaningful change in this file. It feels like change for the sake of change. Any chance we could revert this?

jerelmiller · 2023-07-06T04:29:37Z

src/react/hooks/__tests__/useQuery.test.tsx

@@ -27,6 +32,9 @@ import { QueryResult } from "../../types/types";
 import { useQuery } from "../useQuery";
 import { useMutation } from "../useMutation";

+const waitFor: typeof _waitFor = (callback, options) =>


I see enough interval: 50 overrides in here to think that this change doesn't make as big of an impact as normal. Could you perhaps rename this to waitForOne and use that in places where interval: 1 is set? That way we can leave anything with interval: 50 to use the default waitFor function without needing the override.

jerelmiller · 2023-07-06T04:31:36Z

src/react/hooks/__tests__/useLazyQuery.test.tsx

@@ -19,6 +19,9 @@ import { QueryResult } from "../../types/types";

 const IS_REACT_18 = React.version.startsWith("18");

+const waitFor: typeof _waitFor = (callback, options) =>


Could we consider naming this waitForOne and use that everywhere we use waitFor with interval: 1? That way its easier to use the built-in waitFor without needing the override where necessary. Thoughts?

phryneas · 2023-07-06T09:18:53Z

This was a follow-up PR to the prettier PR, so I guess some of the style changes you are seeing might be prettier-related - with prettier the extra braces mean more newlines and one-line statements became four-line statements, so this is an attempt to undo some of that bloat.

I'm fine with either waitFor or waitForOne, I'd just like to reduce the 100-times-over-repeated {interval: 1}.

So yeah, still relevant, but something I'll probably have to redo after the prettier PR :)

phryneas · 2023-09-14T14:55:42Z

At this point, I guess this has lost relevance as the new testing style mostly works without waitFor. I'm closing this.

change waitFor brace style, switch interval default

f445736

phryneas mentioned this pull request Mar 17, 2023

format react folder with prettier #10661

Closed

lhmzhou approved these changes Mar 21, 2023

View reviewed changes

benjamn approved these changes Mar 31, 2023

View reviewed changes

alessbell approved these changes Mar 31, 2023

View reviewed changes

alessbell added the 🔬 testing-utilities label Apr 3, 2023

phryneas self-assigned this Jun 13, 2023

jerelmiller requested changes Jul 6, 2023

View reviewed changes

phryneas closed this Sep 14, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change `waitFor` brace style, switch `interval` default #10662

change `waitFor` brace style, switch `interval` default #10662

phryneas commented Mar 17, 2023

changeset-bot bot commented Mar 17, 2023

lhmzhou left a comment

benjamn left a comment •

edited

Loading

alessbell left a comment

jerelmiller left a comment

jerelmiller Jul 6, 2023

jerelmiller Jul 6, 2023

jerelmiller Jul 6, 2023

jerelmiller Jul 6, 2023

phryneas commented Jul 6, 2023 •

edited

Loading

phryneas commented Sep 14, 2023

		@@ -19,6 +19,9 @@ import { QueryResult } from "../../types/types";

		const IS_REACT_18 = React.version.startsWith("18");

		const waitFor: typeof _waitFor = (callback, options) =>

change waitFor brace style, switch interval default #10662

change waitFor brace style, switch interval default #10662

Conversation

phryneas commented Mar 17, 2023

do not merge before #10661 has been merged

do not merge before #10661 has been merged

changeset-bot bot commented Mar 17, 2023

⚠️ No Changeset found

lhmzhou left a comment

Choose a reason for hiding this comment

benjamn left a comment • edited Loading

Choose a reason for hiding this comment

alessbell left a comment

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

jerelmiller Jul 6, 2023

Choose a reason for hiding this comment

jerelmiller Jul 6, 2023

Choose a reason for hiding this comment

jerelmiller Jul 6, 2023

Choose a reason for hiding this comment

jerelmiller Jul 6, 2023

Choose a reason for hiding this comment

phryneas commented Jul 6, 2023 • edited Loading

phryneas commented Sep 14, 2023

change `waitFor` brace style, switch `interval` default #10662

change `waitFor` brace style, switch `interval` default #10662

benjamn left a comment •

edited

Loading

phryneas commented Jul 6, 2023 •

edited

Loading