perf(changed-files): limit git and hg commands to specified roots #6732
Conversation
| @@ -46,33 +46,44 @@ const findChangedFilesUsingCommand = async ( | |||
| const adapter: SCMAdapter = { | |||
| findChangedFiles: async ( | |||
| cwd: string, | |||
| roots: Array<Path>, | |||
| options?: Options, | |||
There was a problem hiding this comment.
How about moving roots to Options? This way we could keep it backwards compatible.
There was a problem hiding this comment.
is the idea that we would keep roots as optional within Options but ensure that getChangedFilesForRoots always provides it?
There was a problem hiding this comment.
Yup, ideally with the default behavior being the same as current one (pre this PR)
There was a problem hiding this comment.
Just pushed the fix. Let me know what you think. Thanks!
…etChangedFilesForRoots
|
@thymikee - Just rebased that last commit with some tweaks. Let me know if that works for your migration path. |
Codecov Report
@@ Coverage Diff @@
## master #6732 +/- ##
==========================================
- Coverage 63.68% 63.68% -0.01%
==========================================
Files 235 235
Lines 9003 9007 +4
Branches 4 4
==========================================
+ Hits 5734 5736 +2
- Misses 3268 3270 +2
Partials 1 1
Continue to review full report at Codecov.
|
| @@ -326,3 +347,31 @@ test('gets changed files for hg', async () => { | |||
| .sort(), | |||
| ).toEqual(['file5.txt']); | |||
| }); | |||
|
|
|||
| test('should only monitor root paths for hg', async () => { | |||
There was a problem hiding this comment.
we should avoid using should in test titles :)
use it('monitors only root paths for hg'...
|
|
||
| run(`${GIT} init`, DIR); | ||
|
|
||
| const roots = ['nested_dir', 'nested_dir/second_nested_dir'].map(filename => |
There was a problem hiding this comment.
seems like nested_dir already includes nested_dir/second_nested_dir root
There was a problem hiding this comment.
Updated / removed redundant root!
|
Sounds great! Let me know if there's anything you all would like me to do prior to merge (squash rebase or any tweaks?). Otherwise, when might it make it to master? |
|
@SimenB you ok with changelog? I'm keen on merging this and let @aaronabramov test under some heavy load :) |
|
Go wild 😀 |
|
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
Given a monorepo setup under git version control, we saw
jest --watchcommands take significantly longer in a Docker container host mount (rw:cached) than when on the host directly. Tracked the issue down to the fact thatgitandhgcommand used to determinelastCommitchanges were traversing the entire repository, and not just paths specified in the Jest configrootsvalues. Given the mounted container latency, things like basiclstatgathering be intensive in a large repository. Therootswe had specified are rather shallow themselves and only contain things we want to test with Jest. When limiting the scope of thegitcommand to just therootspaths, we saw change determination drop from 2.5m to 3s in our container.This implementation seem to align more closely with the
rootsdocumentation which indicates:It did seem a little unexpected that Jest would be traversing our entire repo.
Test plan
Specs have been added for new functionality a build from this fork is currently being used in multiple environments.
For reference, here is our Jest configuration:
The
spec/puppeteersubdirectory contains only a handful of files whereas the project containing thepackage.jsonitself is quite expansive. A similar environment staging is probably easily recreated.