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

ci: vitest workspaces #868

Merged
merged 5 commits into from
May 8, 2024
Merged

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Apr 30, 2024

🧰 Changes

Messing around with Vitest Workspaces and it seems pretty great. Previously, we had to cd into the correct subpackage in order to run npx vitest [path to file] but now we can just do it from the root 😌

I forked a test in oas-normalize into its own config + test file and made it so it uses chdir as part of the test. That way, we can run tests from the both the root and from the subpackages and it'll ✨ just work ✨

(no longer applicable)

the downside here is that in oas-normalize there's one test that will now fail unless it's being run from the project root. Feels like a worthy trade-off, but let me know if you disagree.

As part of this, I also upgraded our Vitest deps to the latest and added coverage into the mix.

🧬 QA & Testing

If tests (which should be faster now?) pass, we should be in good shape!

@kanadgupta kanadgupta requested a review from erunion April 30, 2024 18:45
@kanadgupta kanadgupta marked this pull request as ready for review April 30, 2024 18:45
@@ -29,12 +29,12 @@
"content": {
"application/json": {
"schema": {
"$ref": "./test/__fixtures__/bundle/pet.json"
"$ref": "./packages/oas-normalize/test/__fixtures__/bundle/pet.json"
Copy link
Member

Choose a reason for hiding this comment

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

does running npm test from inside packages/oas-normalize still work with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope 😕

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 mentioned this in the PR description — not sure which is better

Copy link
Member

Choose a reason for hiding this comment

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

can probably change this file from definition.json to definition.js and have it export the def + change this ref to use require.resolve('oas-normalize/test/__fixtures__/bundle/pet.json'). wonder if that'd work. it wouldn't be a relative import anymore but i don't think that matters too much since relative file refs are tested in json-schema-ref-parse and openapi-parser anyways.

@kanadgupta
Copy link
Member Author

it might be a fluke but looks like it's roughly ~7-11 seconds faster?

before:

after:

Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

cool with this as-is tho

@kanadgupta
Copy link
Member Author

@erunion mind taking another look? tried out a new approach, see this chunk of the updated PR description:

I forked a test in oas-normalize into its own config + test file and made it so it uses chdir as part of the test. That way, we can run tests from the both the root and from the subpackages and it'll ✨ just work ✨

@kanadgupta kanadgupta requested a review from erunion May 7, 2024 22:45
Copy link
Member

Choose a reason for hiding this comment

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

should oas-normalize have its own workspace config so if you run vitest from inside that package it'll run the chdir test too?

Copy link
Member Author

Choose a reason for hiding this comment

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

oooh i hadn't thought of that - let me play around!

Copy link
Member Author

Choose a reason for hiding this comment

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

added in eebc72b — works like a charm ✨

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 dislike how we have four config files now, but i can't seem to think of a simpler approach here that addresses everything)

@kanadgupta kanadgupta requested a review from erunion May 8, 2024 13:19
@kanadgupta kanadgupta merged commit 18abb90 into main May 8, 2024
6 checks passed
@kanadgupta kanadgupta deleted the kanad-2024-04-30/vitest-workspaces branch May 8, 2024 14:32
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