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

Remove itAsync from remaining relevant tests #12210

Merged
merged 12 commits into from
Jan 11, 2025

Conversation

jerelmiller
Copy link
Member

This PR completes the work to remove itAsync from our tests. It is still used by the hoc and component tests, but these will be deleted with 4.0 so we can ignore these for now. itAsync will be removed when we remove the query components and hoc on the 4.0 branch.

@jerelmiller jerelmiller requested a review from phryneas December 12, 2024 04:55
@svc-apollo-docs
Copy link

svc-apollo-docs commented Dec 12, 2024

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 30889cb03ddd87849c28058b

Copy link

changeset-bot bot commented Dec 12, 2024

⚠️ No Changeset found

Latest commit: 9c3de69

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

Copy link

pkg-pr-new bot commented Dec 12, 2024

npm i https://pkg.pr.new/@apollo/client@12210

commit: 9c3de69

@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-2 branch from c976713 to 6c82bf7 Compare December 12, 2024 17:48
@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-2 branch from 6c82bf7 to f2defe9 Compare January 8, 2025 01:08
@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-3 branch from 1e7d108 to 5bdd9db Compare January 8, 2025 01:14
@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-2 branch from f2defe9 to ba16e3d Compare January 8, 2025 01:28
@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-3 branch 3 times, most recently from 1ca4f08 to 79f6e3d Compare January 10, 2025 03:49
Comment on lines 63 to 65
stream.unsubscribe();

await expect(stream).not.toEmitAnything();
Copy link
Member

Choose a reason for hiding this comment

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

Same pattern as in the previous PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

if (obs === aObs) {
throw new Error("aQuery should not have been updated");
} else if (obs === bObs) {
expect(diff.result).toEqual({ b: "Bee" });
Copy link
Member

Choose a reason for hiding this comment

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

Most tests in this file feel like they need an assertion on the count of expect calls, otherwise these parts could also just never run and we wouldn't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I started that and meant to come back. I need to do a minor update to the toEmit* APIs because takeNext, etc. use expect(...).toEqual under the hood so some assertions get double counted (in other words, each expect(stream).toEmitValue(...) counts as 2 assertions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright got the take* functions updated in ObservableStream to avoid using expect in 70fbc12 and updated the tests to use assertion counts in a4bae62. That update in ObservableStream now avoids double counting assertions when using the .toEmit* matchers.

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

Left two comments, once those are addressed this is good to go.

@github-actions github-actions bot added the auto-cleanup 🤖 label Jan 10, 2025
Base automatically changed from jerel/remove-itasync-part-2 to main January 10, 2025 17:42
@jerelmiller jerelmiller force-pushed the jerel/remove-itasync-part-3 branch from 79f6e3d to c32e923 Compare January 10, 2025 17:43
Copy link
Contributor

github-actions bot commented Jan 10, 2025

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 40.66 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 50.07 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 47.18 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 36.18 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 33.58 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.24 KB (0%)
import { useQuery } from "dist/react/index.js" 5.21 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.29 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.7 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.78 KB (0%)
import { useMutation } from "dist/react/index.js" 3.62 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.84 KB (0%)
import { useSubscription } from "dist/react/index.js" 4.42 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 3.48 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.51 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.17 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 5.01 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.66 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.09 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.74 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.41 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.35 KB (0%)
import { useFragment } from "dist/react/index.js" 2.36 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.31 KB (0%)

Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 9c3de69
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/6781be68e61d9c0008e9336f
😎 Deploy Preview https://deploy-preview-12210--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jerelmiller jerelmiller merged commit cda321c into main Jan 11, 2025
46 checks passed
@jerelmiller jerelmiller deleted the jerel/remove-itasync-part-3 branch January 11, 2025 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants