-
Notifications
You must be signed in to change notification settings - Fork 2
chore: setup testing libraries and tests for App.tsx #55
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.github/workflows/test.yml
Outdated
- name: Setup pnpm | ||
uses: pnpm/action-setup@v4 | ||
with: | ||
version: 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use 9 in dogfood (probably soon to be 10)
vitest.config.ts
Outdated
test: { | ||
globals: true, | ||
environment: 'happy-dom', | ||
// setupFiles: ['./src/test/setup.ts'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// setupFiles: ['./src/test/setup.ts'], |
vitest.config.ts
Outdated
coverage: { | ||
provider: 'v8', | ||
reporter: ['text', 'json', 'html'], | ||
exclude: [ | ||
'node_modules/', | ||
'src/test/', | ||
'**/*.d.ts', | ||
'**/*.config.*', | ||
'src/vite-env.d.ts' | ||
] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are we doing with the coverage information? do we want to upload it somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently nothing, a lot of the initial config was from trying to use AI to help me get something working that I eventually ditched. Sorry for forgetting to remove it from the PR.
vitest.config.ts
Outdated
resolve: { | ||
alias: { | ||
'@': path.resolve(__dirname, './src'), | ||
"monaco-editor": "./src/__mocks__/monaco-editor.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the docs recommend using test.alias
rather than resolve.alias
for mocks. I'm not sure that there's any mechanical different, but the separation of concerns does seem kind of nice.
vitest.config.ts
Outdated
'src/vite-env.d.ts' | ||
] | ||
}, | ||
// Mock WebAssembly for tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
"postcss": "^8.5.3", | ||
"tailwindcss": "3", | ||
"typescript": "~5.8.3", | ||
"typescript-eslint": "^8.30.1", | ||
"vercel": "^42.3.0", | ||
"vite": "^6.3.5", | ||
"vite-plugin-vercel": "^9.0.7" | ||
"vite-plugin-vercel": "^9.0.7", | ||
"vitest": "^3.2.4" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
\n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My editor removes trailing new lines, is there a reason to have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
short answer: yes.
long answer: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having some issues with getting helix not to strip the new line. But this will be fixed once this PR is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. the docs claim that insert-final-newline
defaults to true
, and it doesn't seem like you've had this issue on other files. weird.
.github/workflows/test.yml
Outdated
|
||
- name: Install dependencies | ||
run: pnpm install | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: Install dependencies | |
run: pnpm biome check | |
tho eslint is also in here I guess? do we need both or can we get rid of eslint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not using eslint in this. I want to do another PR where I set up knip and clean up the dev dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is said issues and I think I'll make it a fast follow to this PR. Even including the lint check in the PR is going to bloat the diff
import { ThemeProvider } from "@/client/contexts/theme"; | ||
import attachGPUExample from "@/examples/code/attach-gpu.tf?raw"; | ||
import defaultExample from "@/examples/code/default.tf?raw"; | ||
import { initWasm } from "@/utils/wasm"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually work? you're importing it before the mock is set up (I always avoid mocks because this sort of thing is so confusing to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah because vitest moves all mocks to the top of the file
src/client/App.test.tsx
Outdated
it("should not show the loading modal after the wasm has been loaded", async () => { | ||
render(<TestApp />); | ||
|
||
await waitFor(async () => { | ||
expect(initWasm).toHaveBeenCalled(); | ||
}); | ||
|
||
expect(screen.queryByTestId("wasm-loading-modal")).toBeNull(); | ||
expect(screen.queryByTestId("wasm-error-modal")).toBeNull(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have more confidence in this test if it was merged with "should show the loading modal while the wasm module is loading"
this is all kind of one behavior. it should show when it's loading, and shouldn't when it's not. seeing that it does in fact go away after being shown is the characteristic we'd really like to see here, but all this checks is that it's not their now. it might've never been, the test never asserts that.
vitest.config.ts
Outdated
@@ -0,0 +1,39 @@ | |||
/// <reference types="vitest" /> | |||
import { defineConfig } from 'vite' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this file is formatted differently from our usual "double quotes with semicolons"
package.json
Outdated
"vite-plugin-vercel": "^9.0.7", | ||
"vitest": "^3.2.4" | ||
}, | ||
"packageManager": "pnpm@10.14.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should get corepack use pnpm@10.14.0
to insert the hash here
.github/workflows/test.yml
Outdated
- name: Setup pnpm | ||
uses: pnpm/action-setup@v4 | ||
with: | ||
version: 10.14.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and speaking of corepack, I wonder if we should just lean on it here. at the very least, the docs for this action say that it'll infer version
from package.json and we should lean on that. https://github.com/pnpm/action-setup?tab=readme-ov-file#version
vitest.config.ts
Outdated
export default defineConfig({ | ||
plugins: [react()], | ||
test: { | ||
globals: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're using imports, not globals. can we remove this?
globals: true, |
vitest.config.ts
Outdated
test: { | ||
globals: true, | ||
environment: "happy-dom", | ||
css: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this one necessary? config files tend to bloat over time, so I'd love to keep it as lean as we can to begin with.
Sets up vitest and adds tests for App.tsx
Fist PR of two for #45