-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
add clearCache cli command #4430
Conversation
Thanks! Can you make sure |
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 need an integration test for this.
packages/jest-cli/src/cli/args.js
Outdated
clearCache: { | ||
default: undefined, | ||
description: | ||
'Clear default directory Jest uses for cache and exits.' + |
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.
Nit: Clears or exit, whatever the convention is right now (hard to access on a phone)
packages/jest-cli/src/cli/index.js
Outdated
clearDirectory(configs[0].cacheDirectory); | ||
process.stdout.write(`Cleared ${configs[0].cacheDirectory}\n`); | ||
process.exit(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 be able to clear the cache for certain project or all projects, right now it's only for the first one.
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 wasn't sure how this worked: should I just map over the configs array?
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.
yup, should be fine
packages/jest-util/src/index.js
Outdated
const clearDirectory = (path: string) => { | ||
if (fs.existsSync(path)) { | ||
fs.readdirSync(path).map(file => { | ||
const filePath = `${path}/${file}`; |
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 won't work on windows, use path.join
or path.resolve
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.
Didnt even think of that. Will research.
@thymikee I'm really not sure where to start on the integration test- Im assuming we just need to mock jest for the config object it generates, run the clearDirectory function then verify the directory is empty. Open to suggestions. |
You can check integration_tests for in this repo to see how to write one. You don't mock anything, just spawn a real jest instance with desired config and test the output |
packages/jest-util/src/index.js
Outdated
@@ -31,11 +33,26 @@ const createDirectory = (path: string) => { | |||
} | |||
}; | |||
|
|||
const clearDirectory = (directoryPath: string) => { |
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 not just rimraf
? (or del
or one other of the many other modules that does this)
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.
Only because I don't generally add dependencies to someone else's project unless they mention it. rimraf
is fine.
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.
Migrated to rimraf.
@thymikee Added an integration test, and it's passing. Refactored to use rimraf. Should be read for re-review.
Looks like appveyor is failing, but it doesn't appear to have anything to do with me. Everything else is passing. |
Looks like you didn't push the changes? |
Derp. Sorry. |
Codecov Report
@@ Coverage Diff @@
## master #4430 +/- ##
=======================================
Coverage 57.13% 57.13%
=======================================
Files 194 194
Lines 6565 6565
Branches 3 3
=======================================
Hits 3751 3751
Misses 2814 2814 Continue to review full report at Codecov.
|
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.
Sorry to hold this, but it just needs a bit more work. We're almost there 🙂
packages/jest-cli/src/cli/index.js
Outdated
let clearCacheError; | ||
|
||
configs.map(config => { | ||
rimraf(config.cacheDirectory, [], () => { |
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 afraid this is completely wrong and doesn't work at all.
Please refer to rimraf
tests on how to use it:
https://github.com/isaacs/rimraf/blob/d84fe2cc6646d30a401baadcee22ae105a2d4909/test/basic.js#L48-L55.
Also I see no point in using async API, since we're not doing anything else in this time. Please use rimraf.sync
, this way you'll avoid temporary clearCacheError
variable and perf overhead of async call.
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.
Not sure what I was thinking - process exited before async call finished. Will switch to sync. Will be easier than using a promise all similar.
`--cacheDirectory=${CACHE}`, | ||
]); | ||
|
||
expect(fs.existsSync(CACHE)).toBe(false); |
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's a false positive. This folder doesn't exist, because Jest didn't run any tests. We need to populate this cache earlier with a sample run, then run it once more. Or just create the dir and make sure it is present prior to running test.
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.
Sorry I thought the folder was created when calling runJest regardless of whether there are tests or not. Will add a stub test, verify the cache dir has contents, then runJest again with 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.
can you add
expect(fs.existsSync(CACHE)).toBe(true);
above the runJest
call?
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.
In the second test correct? Makes sense.
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.
Correct!
No problem, if it's not right you shouldn't pass it :) |
packages/jest-cli/src/cli/index.js
Outdated
@@ -67,6 +68,15 @@ const runCLI = async ( | |||
outputStream, | |||
); | |||
|
|||
if (argv.clearCache) { | |||
configs.map(config => { |
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.
forEach
over map
. (You ignore the return value anyways)
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 usually prefer map to prevent accidental mutation. I suppose here it doesn't matter as the process is about to end. I will defer to you on this.
packages/jest-cli/src/cli/args.js
Outdated
default: undefined, | ||
description: | ||
'Clears the configured Jest cache directory and then exits.' + | ||
'Default directory can be found by calling jest --showConfig', |
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.
Missing space before "Default"?
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.
Good catch.
|
||
const CACHE = path.resolve(os.tmpdir(), 'clear_cache_directory'); | ||
|
||
skipOnWindows.suite(); |
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.
Necessary, or copypasta? (Just asking, fine if needed 😄)
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.
skipOnWindows
? I assumed it was part of the test convention as I saw it everywhere. I can remove it if it isn't necessary.
There is currently an issue on master where we get the following stack trace messing up quite a few tests --including mine-- where tests are looking for a specific stderr output. Looks related to yargs, but the last breaking upgrade to yargs was before I started on this PR.
|
@tabrindle this PR seems like it's gotten lots of commits it shouldn't have. Care to rebase? Regarding you issue, does it help to do |
- map over all configs
- rename path arg to directoryPath for less confusion
- yarn run jest -- -t 'jest --clearCache' $ node ./packages/jest-cli/bin/jest.js "-t" "jest --clearCache" PASS integration_tests/__tests__/clear_cache.test.js Test Suites: 165 skipped, 1 passed, 1 of 166 total Tests: 1573 skipped, 1 passed, 1574 total Snapshots: 0 total
- actually create directory with real test - check for directory, then delete - use rimraf.sync - remove temp var
f6a577a
to
eeaa476
Compare
@SimenB Thanks for the tip - Rebased & looks like my new tests are passing, and the only test failing is the same one failing on master.
|
6f51489
to
82d01b3
Compare
CI is happy, that's great. The failing test you see is tracked in #4630 |
In an effort to double check, I was able to confirm the new cache directory creation, and it's deletion via On my machine: /var/folders/9h/tl11z92d59sd2zw5c3zcxn740000gn/T/clear_cache_directory is created then immediately destroyed via integration test. |
/** | ||
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved. | ||
* | ||
* This source code is licensed under the BSD-style license found in the |
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.
change the license header to be MIT, please
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.
Also found jest/integration_tests/__tests__/pass_with_no_tests.test.js
with BSD license. Should I take care of that here?
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.
Yes please
CI is red. Circle is not friendly to mobile, but I'm guessing a lint error? |
Indeed - appears .vscode/settings.json has prettier configured edit: didnt have prettier-vscode installed before, and somehow it was still formatting - once installed the config seems to work correctly. |
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.
LGTM! Thanks for sticking with us
Awesome! Can you also send a PR to update the documentation with this arg? |
Will do! |
* add to changelog for PR #4430 * add —clearCache to docs
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Occasionally Jest's cache needs to be cleared for debugging purposes.
This seems to be a fairly common question, to say nothing about caching being a difficult thing:
https://stackoverflow.com/questions/44866994/how-to-clear-jest-cache
It has also been mentioned in issues here: #3705
Even with a PR to explain how to do so. #4232
Seems silly to have a multi step operation to clear a cache - so I wrote a bash script to do it
jest --showConfig | grep cacheDirectory | awk -F\" '{ print $4 }' | xargs rm -rf
which in the end is dumb because jest has a cli already, and parsing JSON in bash like this is a bad idea.Test plan
jest --clearCache
This should do nothing more than delete the cacheDirectory and exit 0.