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

Migrating vitest to SDK #1672

Merged
merged 10 commits into from
Jan 27, 2024
Merged

Migrating vitest to SDK #1672

merged 10 commits into from
Jan 27, 2024

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Jan 24, 2024

You can try out the feature by running cabal run wasp-cli test client.

@@ -1,4 +1,8 @@
import matchers from '@testing-library/jest-dom/matchers'
import { expect } from 'vitest'
import { afterEach } from 'vitest'
Copy link
Contributor Author

@infomiho infomiho Jan 24, 2024

Choose a reason for hiding this comment

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

I've updated the vitest and all the accompanying libraries to the latest versions. I did this because there was an error with using jsx files from deps (in our case our SDK) which would mean we would need to bundle our SDK for vitest to work again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running with the latest versions seems to work fine 👍

@@ -20,7 +20,8 @@
"esModuleInterop": true,
"moduleResolution": "node",
"outDir": "dist",
"allowJs": true
"allowJs": true,
"types": ["@testing-library/jest-dom"],
Copy link
Contributor Author

@infomiho infomiho Jan 24, 2024

Choose a reason for hiding this comment

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

There were type errors related to the unit test matchers e.g. toBeInTheDocument was reported as undefined. Since we are copying the ext-src to sdk we also need these types here.

@@ -1,4 +1,4 @@
/.wasp/
/.env.server
/.env.client
/node_modules/
node_modules/
Copy link
Contributor Author

@infomiho infomiho Jan 24, 2024

Choose a reason for hiding this comment

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

Vitest creates some cache files in the src dir and it does it in node_modules/.vitest

@@ -48,36 +49,6 @@ export const MainPage = ({ user }: { user: User }) => {
)
}

function Todo({ id, isDone, description }: Task) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted this component so I can write a test for it.

@@ -16,11 +16,12 @@
"esnext"
],
"allowJs": true,
"types": ["@testing-library/jest-dom"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS complains about toBeInTheDocument and other matchers to be undefined if we don't include this.

This means that the user tsconfig.json needs to include this for the type support to work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do this differently somehow? @sodic

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, how did it work before restructuring?

I know this works, but I remember reading a TS issue recently where people complained about TS not looking for types in directories outside of @types. I'll try to find it and see what they decided.

Edit: It was this one, kind of related.

@infomiho infomiho marked this pull request as ready for review January 25, 2024 14:56
@@ -161,17 +158,6 @@ depsRequiredByTailwind spec =
]
else []

depsRequiredForTesting :: [AS.Dependency.Dependency]
Copy link
Contributor Author

@infomiho infomiho Jan 25, 2024

Choose a reason for hiding this comment

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

web-app needs to resolve to the deps in the root of the project (that's why we don't include any test deps in the web-app package) so that the setup.ts function can hook into the same object instances from various test libraries. Otherwise, we get an error that e.g. React can't be found.

@@ -39,7 +42,7 @@ watchAndTest testRunner = do
watchOrStartResult <- liftIO $ do
ongoingCompilationResultMVar <- newMVar (warnings, [])
let watchWaspProjectSource = watch waspRoot outDir ongoingCompilationResultMVar
watchWaspProjectSource `race` testRunner outDir
watchWaspProjectSource `race` testRunner waspRoot
Copy link
Contributor Author

@infomiho infomiho Jan 25, 2024

Choose a reason for hiding this comment

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

Instead of running from the .wasp/out dir, it now runs from the Wasp project root dir.

waspc/src/Wasp/Generator/Test.hs Show resolved Hide resolved
setupFiles: ["./src/test/vitest/setup.ts"],
// vitest is running from the root of the project, so we need
// to specify the path to the setup file relative to the root.
setupFiles: ["./.wasp/out/web-app/src/test/vitest/setup.ts"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is already templated, so let's use the knowledge we have about this folder instead of hardcoding it.

@@ -16,11 +16,12 @@
"esnext"
],
"allowJs": true,
"types": ["@testing-library/jest-dom"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, how did it work before restructuring?

I know this works, but I remember reading a TS issue recently where people complained about TS not looking for types in directories outside of @types. I'll try to find it and see what they decided.

Edit: It was this one, kind of related.

// vitest is running from the root of the project, so we need
// to specify the path to the setup file relative to the root.
setupFiles: {=& vitestSetupFilesArray =},
exclude: [...defaultExclude, ".wasp/**/*"]
Copy link
Contributor

@sodic sodic Jan 26, 2024

Choose a reason for hiding this comment

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

Same story as with setup files, .wasp is already defined somewhere in Haskell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I've now changed the template 👍

Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Great! Merging this.

@sodic sodic merged commit c8370bf into filip-sdk Jan 27, 2024
1 of 4 checks passed
@sodic sodic deleted the miho-vitest-sdk branch January 27, 2024 11:05
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.

2 participants