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

changed files eager loading #3979

Merged

Conversation

aaronabramov
Copy link
Contributor

should solve the performance regression in www.
I'm not a fan of passing this promise all the way down, but there isn't really a nice way of doing this yet

cc @rogeliog can you look at the watch stuff? i wanna make sure i didn't break anything accidentally

@codecov-io
Copy link

codecov-io commented Jul 7, 2017

Codecov Report

Merging #3979 into master will increase coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3979      +/-   ##
=========================================
+ Coverage   59.87%   59.9%   +0.02%     
=========================================
  Files         196     197       +1     
  Lines        6794    6804      +10     
  Branches        6       6              
=========================================
+ Hits         4068    4076       +8     
- Misses       2723    2725       +2     
  Partials        3       3
Impacted Files Coverage Δ
packages/jest-cli/src/lib/get_test_path_pattern.js 52.94% <ø> (ø) ⬆️
packages/jest-changed-files/src/index.js 95.23% <ø> (ø) ⬆️
packages/jest-changed-files/src/git.js 92.59% <ø> (ø) ⬆️
packages/jest-changed-files/src/hg.js 32.25% <ø> (ø) ⬆️
packages/jest-cli/src/run_jest.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/search_source.js 76.66% <0%> (-2.65%) ⬇️
packages/jest-cli/src/watch.js 73.55% <100%> (+0.44%) ⬆️
packages/jest-cli/src/get_changed_files_promise.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8abb0dd...8b47815. Read the comment docs.

lastCommit: testSelectionConfig.lastCommit,
});
if (!changedFilesPromise) {
throw new Error('This promise must be presen when running with -o');
Copy link
Member

Choose a reason for hiding this comment

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

present

Copy link
Member

Choose a reason for hiding this comment

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

and end with

running with `-o`.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Solid work!

lastCommit: testSelectionConfig.lastCommit,
});
if (!changedFilesPromise) {
throw new Error('This promise must be present when running with -o.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change the error message to something like "'A Git or Mercurial repository must be present when running with -o."? So that it gives some info to the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is pretty much an invariant and should never throw. I only put it in there, because the promise is generated early in the flow and argv is modified during the flow. So potentially we can cause inconsistent state (ran with -o, but promise is not passed).
This error here just for catching potential bugs early :)

@@ -95,6 +97,7 @@ describe('Watch mode flows', () => {
pipe,
new TestWatcher({isWatchMode: true}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider changing the signature of run_jest to an object of all this options instead of positional arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's on my plan! bun not in this PR :)
I already started refactoring the CLI flow

@aaronabramov aaronabramov merged commit 6ae7b02 into jestjs:master Jul 7, 2017
@aaronabramov aaronabramov deleted the changed_files_eager_loading branch July 7, 2017 21:14
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* changed files eager loading

* Update search_source.js
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants