-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: add caching to run failed and longer tests first #1541
feat: add caching to run failed and longer tests first #1541
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Looks good for the overall direction. |
c1bea1b
to
795518c
Compare
ae02421
to
553d285
Compare
@antfu I did some changes since your last review:
|
docs/guide/cli.md
Outdated
@@ -36,6 +36,10 @@ Useful to run with [`lint-staged`](https://github.com/okonet/lint-staged) or wit | |||
vitest related /src/index.ts /src/hello-world.js | |||
``` | |||
|
|||
### `vitest clearCache` |
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.
### `vitest clearCache` | |
### `vitest purge` |
I am not a fan of having pascal case in CLI. Maybe purge
, WDYT?
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.
Why purge
tho? Don't see any connection to cache. We can do clear-cache
, I guess.
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.
Just want to find a single word to represent it. As for reference, here is what package managers are doing:
npm cache clean --force
yarn cache clean
pnpm store prune
And for Vite: https://vitejs.dev/guide/dep-pre-bundling.html#file-system-cache
/cc @patak-dev in case we are also going to add a clear cache command for Vite, I think we better align the commands
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 can do vitect clean
, and pass cache
as an argument. In future we might add option to clear only some specific caches.
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've updated to proposed solution. What do you think, @antfu, @patak-dev?
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.
it is something you use a lot so it is better if it is a single word
Hardly disagree with using a lot - I don't know why you would even use it in Vitest, only to clear malformed cache maybe.
Also single words only look good, but confuse a lot. What --clean
means? What happens when you call it? I want to have cache
in the name 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.
I use --force
a lot because ya, I'm debugging Vite cache itself 👀
So I agree with you that normally isn't something that final users will be reaching a lot. I'm fine with --clearCache
(maybe good to check how it will look later for --clearCache="parts of 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.
maybe good to check how it will look later
It will look something like this:
vitest run --clearCache results
We use cac's notation
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.
@antfu what do you think?
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 will remove command for now, since I want this feature to be released today. Command will be in another PR.
…/order-tests-by-duration
Closes #1532
TODO