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

[CoverageReporter] Parallelize instrumentation of untested files. #3309

Closed

Conversation

gdborton
Copy link
Contributor

Summary

This changes the coverage reporter's _addUntestedFiles function to be
parallelized. This speeds up coverage collection on machines with multiple
cores for test suites with many untested files.

Test plan

Wrote tests for the new worker file, npm linkd jest-cli and tested against a large local repo, tested entire repo via yarn test.

@aaronabramov
Copy link
Contributor

this is beautiful! how much faster was it able to run on your project?

@gdborton
Copy link
Contributor Author

Currently gather timings, but we're estimating savings at close to 3 minutes for our CI runs.

@gdborton
Copy link
Contributor Author

Well, this is saving ~50 seconds locally. I expect more on our CI servers, but enabling that without publishing somewhere is proving to be a bit difficult.

@codecov-io
Copy link

codecov-io commented Apr 18, 2017

Codecov Report

Merging #3309 into master will decrease coverage by 0.02%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3309      +/-   ##
=========================================
- Coverage   64.93%   64.9%   -0.03%     
=========================================
  Files         176     177       +1     
  Lines        6514    6537      +23     
  Branches        4       4              
=========================================
+ Hits         4230    4243      +13     
- Misses       2283    2293      +10     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-cli/src/TestRunner.js 31.32% <0%> (ø) ⬆️
...ackages/jest-cli/src/reporters/CoverageReporter.js 55.81% <20%> (-4.72%) ⬇️
packages/jest-cli/src/reporters/CoverageWorker.js 84.61% <84.61%> (ø)

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 852be05...fb0b49f. Read the comment docs.

@gdborton gdborton force-pushed the parallelize-untested-coverage branch from 0b0a7fc to 4214d21 Compare April 19, 2017 01:19
@gdborton
Copy link
Contributor Author

Rebased on latest changes, I don't think I broke anything... seeing this in the output of yarn test.

Test Suites: 136 passed, 136 total
Tests:       1327 passed, 1327 total
Snapshots:   627 passed, 627 total
Time:        77.043s, estimated 79s
Ran all test suites.
✨  Done in 77.86s.
yarn run v0.20.3
$ node scripts/test_examples.js 
Testing example: /Users/gary_borton/airlab/repos/jest/examples/async
$ cd /Users/gary_borton/airlab/repos/jest/examples/async
  $ ln -fs,/Users/gary_borton/airlab/repos/jest/packages/babel-jest,/Users/gary_borton/airlab/repos/jest/examples/async/node_modules/

ln: /Users/gary_borton/airlab/repos/jest/examples/async/node_modules//babel-jest: Operation not permitted
/Users/gary_borton/airlab/repos/jest/scripts/_runCommand.js:38
    throw error;
    ^

@thymikee
Copy link
Collaborator

Can you run rm -rf examples/*/node_modules and check it again?

@thymikee
Copy link
Collaborator

It's fixed now, you can rebase once more.

This changes the coverage reporter's _addUntestedFiles function to be
parallelized. This speeds up coverage collection on machines with multiple
cores for test suites with many untested files.
@gdborton gdborton force-pushed the parallelize-untested-coverage branch from 4214d21 to fb0b49f Compare April 24, 2017 21:32
@gdborton
Copy link
Contributor Author

@thymikee Rebased and ran the command that you posted, now I'm seeing the below locally...

Skipping react-native on node v4.6.2.
$ cd /Users/gary_borton/airlab/repos/jest/examples/snapshot
$ ln -fs /Users/gary_borton/airlab/repos/jest/packages/babel-jest /Users/gary_borton/airlab/repos/jest/examples/snapshot/node_modules/

$ cd /Users/gary_borton/airlab/repos/jest/examples/timer
$ ln -fs /Users/gary_borton/airlab/repos/jest/packages/babel-jest /Users/gary_borton/airlab/repos/jest/examples/timer/node_modules/

$ cd /Users/gary_borton/airlab/repos/jest/examples/typescript
$ ln -fs /Users/gary_borton/airlab/repos/jest/packages/babel-jest /Users/gary_borton/airlab/repos/jest/examples/typescript/node_modules/

$ cd /Users/gary_borton/airlab/repos/jest/examples
$ /Users/gary_borton/airlab/repos/jest/packages/jest-cli/bin/jest.js --experimentalProjects /Users/gary_borton/airlab/repos/jest/examples/async /Users/gary_borton/airlab/repos/jest/examples/enzyme /Users/gary_borton/airlab/repos/jest/examples/getting_started /Users/gary_borton/airlab/repos/jest/examples/jquery /Users/gary_borton/airlab/repos/jest/examples/manual_mocks /Users/gary_borton/airlab/repos/jest/examples/react /Users/gary_borton/airlab/repos/jest/examples/react-native /Users/gary_borton/airlab/repos/jest/examples/snapshot /Users/gary_borton/airlab/repos/jest/examples/timer /Users/gary_borton/airlab/repos/jest/examples/typescript

● Validation Error:

  Preset react-native not found.

  Configuration Documentation:
  https://facebook.github.io/jest/docs/configuration.html

/Users/gary_borton/airlab/repos/jest/scripts/_runCommand.js:31
    throw error;
    ^

Error running command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.

This is a side note, and probably worthy of a separate issue, but I think the contributing guide can use some work... I'm entirely unfamiliar with lerna, and as of last week npm install -g yarn; yarn install was broken in this repo. I had to grep through Travis's build output on master to find a version of yarn that worked.

@thymikee
Copy link
Collaborator

Thanks for reporting! I'll take a look a this later. If you have any ideas on how to make contributing guide more comprehensive, please send a PR :)

@gdborton
Copy link
Contributor Author

I wish I did... If it weren't giving me problems I'd PR updates, sort of a catch 22. Maybe when I get some spare time I'll further familiarize myself.

@gdborton
Copy link
Contributor Author

@DmitriiAbramov @thymikee ping (i don't know how to open source and don't yet have a feel for what's too much noise) pong

@thymikee
Copy link
Collaborator

Let's just wait until Dmitrii finds some time to review this. In the meantime it's pretty ok to ping him once in a couple of days, so he'll eventually get there :D

@aaronabramov
Copy link
Contributor

sorry :) i'll be updating Jest internally at fb again in a couple of days and i'll test this feature!
The code looks awesome :)

@cpojer cpojer added this to the Jest 20 milestone Apr 28, 2017
@aaronabramov aaronabramov mentioned this pull request Apr 28, 2017
@aaronabramov
Copy link
Contributor

i had to resolve some issues with the current master (we kinda rewrote everything in those files 🙂)
here's a PR that i based on this one #3407

@gdborton gdborton deleted the parallelize-untested-coverage branch May 1, 2017 20:18
@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.

6 participants