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

chore(tests): migrate to vitest #646

Merged
merged 55 commits into from May 4, 2023
Merged

Conversation

Aslemammad
Copy link
Member

Related Issues

#643

@vercel
Copy link

vercel bot commented Jan 30, 2023

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

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2023 11:36pm

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 30, 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 65ddb7c:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration

@Aslemammad
Copy link
Member Author

The tests are passing, I do not know what is happening with types in the lower versions.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

nice work!
added some comments for a start.

tests/basic.test.tsx Outdated Show resolved Hide resolved
@@ -64,7 +57,7 @@ describe('proxyWithComputed', () => {
})

it('computed getters and setters', async () => {
const computeDouble = jest.fn((x: number) => x * 2)
const computeDouble = vi.fn((x) => x * 2)
Copy link
Member

Choose a reason for hiding this comment

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

How is computeDouble typed without : number?

Copy link
Member

Choose a reason for hiding this comment

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

I assume it becomes any. Can we keep : number?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I did it!

vitest.config.ts Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
Comment on lines +34 to +35
"import/namespace": "off",
"import/named": "off",
Copy link
Member

Choose a reason for hiding this comment

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

curious what was the error.

Copy link
Member Author

@Aslemammad Aslemammad Apr 2, 2023

Choose a reason for hiding this comment

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

it's related to vitest ./utils import! so i solved it with this as suggested.

Copy link
Member

Choose a reason for hiding this comment

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

I'll revisit it later.

package.json Outdated Show resolved Hide resolved
vitest.config.ts Outdated Show resolved Hide resolved
vitest.config.ts Outdated Show resolved Hide resolved
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I don't know why __dirname is necessary, and it seems weird, but can't be helped.

@dai-shi dai-shi changed the title chore: migrate to vitest chore(tests): migrate to vitest May 1, 2023
@dai-shi
Copy link
Member

dai-shi commented May 1, 2023

@arjunvegda Could you have a closer look to find why 21db220 doesn't work? what's the difference from jotai and zustand...

@dai-shi
Copy link
Member

dai-shi commented May 1, 2023

Hmm? It looks like it's loading two instances. 🤔

image

@dai-shi
Copy link
Member

dai-shi commented May 1, 2023

Never mind. The last commit fixes the test, and it's probably because it somehow points to a different file.

@arjunvegda
Copy link
Collaborator

Hmm? It looks like it's loading two instances. 🤔

Ah! This explains it. if I change the import in utils/watch.ts#L1 to valtio/vanilla it works as expected 🤔

It appears that vite is resolving modules differently based on how we import the paths (relative vs absolute)?

- import { subscribe } from '../../vanilla.ts'
+ import { subscribe } from 'valtio/vanilla'

@arjunvegda
Copy link
Collaborator

@arjunvegda Could you have a closer look to find why 21db220 doesn't work? what's the difference from jotai and zustand...

Jotai and Zustand do not appear to have conflicting imports within test files, so it is working fine. However, we could reproduce the issue in those libs too.

This will cause modules to be duplicated and the test will fail.

import { expect, it } from 'vitest'
import { getDefaultStore } from 'jotai/vanilla/store'
import { getDefaultStore as getDefaultStoreFromVanilla } from 'jotai/vanilla'

it('should match the default store', () => {
  const storeA = getDefaultStoreFromVanilla()
  const storeB = getDefaultStore()
  expect(storeA).toBe(storeB)
})

This could be fixed by deduping imports using @rollup/plugin-node-resolve in vite.config.ts though

@dai-shi
Copy link
Member

dai-shi commented May 2, 2023

@arjunvegda Thanks for investigating. Things are clear now. I think for now what we have is fine.

@dai-shi dai-shi merged commit c815ac2 into pmndrs:main May 4, 2023
31 checks passed
@Aslemammad Aslemammad deleted the vitest-migrations branch May 4, 2023 16:31
@sheremet-va
Copy link

sheremet-va commented May 5, 2023

@arjunvegda Thanks for investigating. Things are clear now. I think for now what we have is fine.

This happens because relative paths in aliases are used. Vitest doesn't resolve them relative to root. Each time "valtio" is mentioned it will try to load it relative to the file it is imported from and fail. But Vitest fallbacks to the original replacement. This creates two different instances in the modules graph. The fix is to never use relative paths in aliases.

The code you have here will probably break in the next Vitest release where we fixed this.

@Aslemammad
Copy link
Member Author

Aslemammad commented May 5, 2023

The code you have here will probably break in the next Vitest release where we fixed this.

@sheremet-va Thank you for the explanation. We'll try removing it in the next release of Vitest. cc @dai-shi

@dai-shi
Copy link
Member

dai-shi commented May 5, 2023

So, always to use __dirname? That's also clear (as it's consistent).

@sheremet-va
Copy link

So, always to use __dirname? That's also clear (as it's consistent).

Yes. Or import.meta.url.

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.

None yet

4 participants