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

Jest to vitest migration alpha #5097

Merged
merged 13 commits into from
Mar 10, 2023

Conversation

ZackDeRose
Copy link
Contributor

Mostly a clone of #5081, but for the alpha branch (see discussion in that pr w/ @TkDodo)

Addresses #5074

@vercel
Copy link

vercel bot commented Mar 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
query ⬜️ Ignored (Inspect) Mar 10, 2023 at 9:57AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 8, 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 e9f3a1a:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -2.01 ⚠️

Comparison is base (d2af584) 90.39% compared to head (e9f3a1a) 88.39%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #5097      +/-   ##
==========================================
- Coverage   90.39%   88.39%   -2.01%     
==========================================
  Files         105       78      -27     
  Lines        3791     2877     -914     
  Branches      952      815     -137     
==========================================
- Hits         3427     2543     -884     
+ Misses        331      283      -48     
- Partials       33       51      +18     
Impacted Files Coverage Δ
packages/solid-query/src/createBaseQuery.ts 68.35% <ø> (ø)
packages/solid-query/src/createInfiniteQuery.ts 100.00% <ø> (ø)
packages/solid-query/src/createMutation.ts 100.00% <ø> (ø)
packages/solid-query/src/createQuery.ts 100.00% <ø> (ø)
packages/query-core/src/mutation.ts 97.82% <100.00%> (-0.12%) ⬇️
packages/query-core/src/mutationObserver.ts 78.72% <100.00%> (-3.10%) ⬇️
packages/query-core/src/tests/utils.ts 85.71% <100.00%> (-4.61%) ⬇️
packages/react-query/src/useQueries.ts 93.61% <100.00%> (-0.93%) ⬇️
packages/solid-query/src/createQueries.ts 100.00% <100.00%> (ø)
packages/solid-query/src/useIsFetching.ts 100.00% <100.00%> (ø)
... and 2 more

... and 74 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

Thank you 🙏 ❤️ . Please have a look at my comments:

Comment on lines 111 to -112
const handleError = (e: unknown) => {
expect(e).toBe(err)
done()
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does vitest know to wait for this expectation to occur ?

Copy link
Contributor Author

@ZackDeRose ZackDeRose Mar 10, 2023

Choose a reason for hiding this comment

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

I believe vitest will wait for any unresolved promises created by the test's callback closure to resolve before continuing. I affirmed this assumption by adding a false assertion (expect(true).toBe(false)) where the done() was and ensuring the test fails).

If we want we can make the test callback async and explicitely await a promise that will assert this assertion is called. e.g.:

  test('"onError" should be called when "func" throw error', async () => {
    await new Promise((res) => {
      const err = new Error('error')
      const handleError = (e: unknown) => {
        expect(e).toBe(err)
        res()
      }

      const testFunc = asyncThrottle(
        () => {
          throw err
        },
        { onError: handleError },
      )
      testFunc()
    })
  })

This appears to be the recommended solution for the deprecated done parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that looks good 👍

packages/query-core/src/tests/query.test.tsx Outdated Show resolved Hide resolved
Comment on lines 279 to 280
// Should not display the console error
// "Warning: Can't perform a React state update on an unmounted component"
Copy link
Collaborator

Choose a reason for hiding this comment

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

feel free to delete this whole test. this warning is no longer a thing in react18, so the test doesn't test anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

packages/react-query/src/__tests__/useQueries.test.tsx Outdated Show resolved Hide resolved
packages/react-query/src/__tests__/useQuery.test.tsx Outdated Show resolved Hide resolved
packages/solid-query/package.json Outdated Show resolved Hide resolved
Comment on lines 134 to 128
resolve(observer.fetchOptimistic(defaultedOptions.value))
resolve(observer.fetchOptimistic(defaultedOptions.value) as any)
} else {
stopWatch()
resolve(optimisticResult)
resolve(optimisticResult as any)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -4,10 +4,9 @@
"composite": true,
"rootDir": "./src",
"outDir": "./build/lib",
"tsBuildInfoFile": "./build/.tsbuildinfo"
"tsBuildInfoFile": "./build/.tsbuildinfo",
"module": "esnext"
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this change the output of the type definitions for the vue package?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZackDeRose Could you please explain why this change is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey folks - yes I needed top-level await for those mock files. The equivalent of jest's synchronous requireActual is vitest's async importActual. I'll revert this and see if I can fix those mocks some other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the error I was getting is only in the ts language server, so this is safe to revert. See comment in the mock file!


export const useBaseQuery = jest.fn(originImpl)
Copy link
Contributor

Choose a reason for hiding this comment

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

If vi.mock automatically wraps all original implementations with spy functions, then we could remove it.
Jest used the __mock__ directory for mocking modules, without necessity to write the same mock in multiple files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense - thanks!

Comment on lines +4 to +5
// @ts-expect-error - vitest uses esmodules; tsconfig is not set to use them
(await vi.importActual('../useBaseQuery')) as any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reverting my change to the tsconfig.json this error shows up in the IDE via the ts language service, but running pnpm test:ci passes just fine even with the error marked in the IDE.

I've added this // @ts-expect-error so we don't see this via the IDE, but lmk if there's a preferred fix I'm not aware of.

@ZackDeRose
Copy link
Contributor Author

Setting sights on coverage reports now

@ZackDeRose
Copy link
Contributor Author

let's see if that fixes coverage - confirming I see the reports generated now on my local filesystem

@ZackDeRose ZackDeRose requested review from TkDodo and DamianOsipiuk and removed request for TkDodo and DamianOsipiuk March 10, 2023 06:53
@TkDodo
Copy link
Collaborator

TkDodo commented Mar 10, 2023

ran into an error:

packages/react-query test:lib: FATAL ERROR: v8::FromJust Maybe value is Nothing.
packages/react-query test:lib:  1: 0xb09980 node::Abort() [node]
packages/react-query test:lib:  2: 0xa1c235 node::FatalError(char const*, char const*) [node]
packages/react-query test:lib:  3: 0xcf758a v8::Utils::ReportApiFailure(char const*, char const*) [node]
packages/react-query test:lib:  4: 0xb0d7dd node::fs::FileHandle::CloseReq::Resolve() [node]

I've re-run to see if this was just flaky

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 10, 2023

hmm the error seems to persist :/

@ZackDeRose
Copy link
Contributor Author

interesting - I'm getting passing pnpm test:ci locally :\

investigating now

@ZackDeRose
Copy link
Contributor Author

Was able to reproduce this FATAL ERROR: v8::FromJust Maybe value is Nothing. error with node v16.4.2

I'll add a bump this in the ci files to 16.19 (most recent v16), I don't seem to get the error on that version of node, let me know if that acceptable or if we need to stay on this version

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 10, 2023

That's fine 👍

@TkDodo TkDodo merged commit 4aaaa51 into TanStack:alpha Mar 10, 2023
This was referenced Mar 10, 2023
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.

4 participants