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

Show coverage of sources related to tests in changed files #9769

Merged

Conversation

chenesan
Copy link
Contributor

@chenesan chenesan commented Apr 5, 2020

Summary

This PR fixes #8545 . It will show coverage of sources related to tests when jest is in watch mode (--watch) or --onlyChange.

As for implementation, this PR added findRelatedSourceFromTestsInChangedFiles in SearchSource, and apply it when there are changedFiles, then pass the results into options of shouldInstrument. When changedFiles exist and the file is not in changedFiles nor sourcesRelatedToTestsInChangedFiles, we will not instrument it.

Test plan

Add an e2e test case for the use case and some unit test case for SearchSource.

@chenesan chenesan force-pushed the trigger-coverage-of-deps-on-test-file-change branch from 11f20c8 to e236946 Compare April 5, 2020 10:44
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for the thorough tests 👍

packages/jest-runner/src/runTest.ts Outdated Show resolved Hide resolved
packages/jest-reporters/src/coverage_worker.ts Outdated Show resolved Hide resolved
packages/jest-core/src/runJest.ts Outdated Show resolved Hide resolved
packages/jest-core/src/runJest.ts Outdated Show resolved Hide resolved
packages/jest-core/src/SearchSource.ts Outdated Show resolved Hide resolved
packages/jest-core/src/TestScheduler.ts Outdated Show resolved Hide resolved
packages/jest-core/src/TestScheduler.ts Outdated Show resolved Hide resolved
packages/jest-core/src/__tests__/SearchSource.test.ts Outdated Show resolved Hide resolved
packages/jest-core/src/__tests__/SearchSource.test.ts Outdated Show resolved Hide resolved
e2e/__tests__/onlyChanged.test.ts Outdated Show resolved Hide resolved
@SimenB SimenB requested review from jeysal and thymikee April 5, 2020 10:49
@codecov-io
Copy link

codecov-io commented Apr 5, 2020

Codecov Report

Merging #9769 into master will decrease coverage by 0.01%.
The diff coverage is 47.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9769      +/-   ##
==========================================
- Coverage   64.39%   64.37%   -0.02%     
==========================================
  Files         290      290              
  Lines       12361    12391      +30     
  Branches     3056     3059       +3     
==========================================
+ Hits         7960     7977      +17     
- Misses       3761     3770       +9     
- Partials      640      644       +4     
Impacted Files Coverage Δ
packages/jest-core/src/TestScheduler.ts 65.64% <ø> (+0.72%) ⬆️
packages/jest-reporters/src/coverage_reporter.ts 53.57% <0.00%> (-0.33%) ⬇️
packages/jest-reporters/src/coverage_worker.ts 33.33% <0.00%> (-6.67%) ⬇️
...ckages/jest-reporters/src/generateEmptyCoverage.ts 66.66% <ø> (ø)
packages/jest-runner/src/runTest.ts 2.15% <ø> (+0.02%) ⬆️
packages/jest-runtime/src/index.ts 58.71% <ø> (ø)
packages/jest-transform/src/shouldInstrument.ts 78.94% <0.00%> (-9.29%) ⬇️
packages/jest-core/src/runJest.ts 51.19% <27.27%> (-4.81%) ⬇️
packages/jest-core/src/SearchSource.ts 64.95% <75.00%> (+4.75%) ⬆️

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 32aaff8...0d50802. Read the comment docs.

@chenesan
Copy link
Contributor Author

chenesan commented Apr 5, 2020

@SimenB Thanks for the review :-)
Just address all review comments in 2f4c55b; Also use hasSCM() utility in findTestsRelatedToChangeFiles in 39a84f3 to prevent repetition.

CHANGELOG.md Outdated Show resolved Hide resolved
@chenesan
Copy link
Contributor Author

chenesan commented Apr 7, 2020

Hi @SimenB @thymikee @jeysal any news on this?

@SimenB
Copy link
Member

SimenB commented Apr 7, 2020

This is good to go from my side, I'd just like further review on it.

With Easter holidays and the coronavirus things have slowed down a bit, I'm sure they'll review this soon enough 🙂

@chenesan
Copy link
Contributor Author

chenesan commented Apr 7, 2020

Oh I see. Hope you all good during this epidemic period 🙏 Take care 😷

@chenesan chenesan force-pushed the trigger-coverage-of-deps-on-test-file-change branch from 754f365 to e724874 Compare April 22, 2020 15:13
@chenesan
Copy link
Contributor Author

Hi @thymikee @jeysal Sorry for repeatedly pinging, any review on this?

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍 Left a few notes on how I think we could make it make it a bit more performant

testSchedulerContext.changedFiles = changedFilesInfo.changedFiles;
const sourcesRelatedToTestsInChangedFilesArray = contexts
.map(context => {
const searchSource = new SearchSource(context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Creating the SearchSource for a given context is already done earlier, when finding related tests. We could optimize this by storing the searchSources in and array, when mapping contexts for testRunData and reuse it. I think it's worth not doubling the efforts we already made.

return [];
}
const {changedFiles} = changedFilesInfo;
const dependencyResolver = new DependencyResolver(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same DependencyResolver is also created for findRelatedTests. Can we cache it then?

@chenesan
Copy link
Contributor Author

Good idea 😸 Implement reusing SearchSource and DependencyResolver in 73271f0 and 0d50802.

private _testPathCases: TestPathCases = [];

constructor(context: Context) {
const {config} = context;
this._context = context;
this._dependencyResolver = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we may not immediately use DependencyResolver in SearchSource, so at first this._dependencyResolver will be null. When we need DependencyResolver, just call _getDependencyResolver to create/get it.

Comment on lines 355 to 356
const isTestFile = this.isTestFilePath(filePath);
if (isTestFile) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isTestFile = this.isTestFilePath(filePath);
if (isTestFile) {
if (this.isTestFilePath(filePath) {

Comment on lines 541 to 562
beforeEach(done => {
const {options: config} = normalize(
{
haste: {
hasteImplModulePath: path.resolve(
__dirname,
'../../../jest-haste-map/src/__tests__/haste_impl.js',
),
providesModuleNodeModules: [],
},
name: 'SearchSource-findRelatedSourcesFromTestsInChangedFiles-tests',
rootDir,
},
{} as Config.Argv,
);
Runtime.createContext(config, {maxWorkers, watchman: false}).then(
context => {
searchSource = new SearchSource(context);
done();
},
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
beforeEach(done => {
const {options: config} = normalize(
{
haste: {
hasteImplModulePath: path.resolve(
__dirname,
'../../../jest-haste-map/src/__tests__/haste_impl.js',
),
providesModuleNodeModules: [],
},
name: 'SearchSource-findRelatedSourcesFromTestsInChangedFiles-tests',
rootDir,
},
{} as Config.Argv,
);
Runtime.createContext(config, {maxWorkers, watchman: false}).then(
context => {
searchSource = new SearchSource(context);
done();
},
);
});
beforeEach(async () => {
const {options: config} = normalize(
{
haste: {
hasteImplModulePath: path.resolve(
__dirname,
'../../../jest-haste-map/src/__tests__/haste_impl.js',
),
providesModuleNodeModules: [],
},
name: 'SearchSource-findRelatedSourcesFromTestsInChangedFiles-tests',
rootDir,
},
{} as Config.Argv,
);
const context = await Runtime.createContext(config, {maxWorkers, watchman: false})
searchSource = new SearchSource(context);
});

diff looks big, but async await instead of .then and done

Comment on lines 96 to 97
if (options.changedFiles) {
if (!options.changedFiles.has(filename)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (options.changedFiles) {
if (!options.changedFiles.has(filename)) {
if (options.changedFiles && !options.changedFiles.has(filename)) {

@chenesan chenesan force-pushed the trigger-coverage-of-deps-on-test-file-change branch from 9df97b2 to 913617b Compare April 26, 2020 08:04
@thymikee thymikee merged commit 3f107a3 into jestjs:master Apr 26, 2020
jeysal added a commit to SimenB/jest that referenced this pull request Apr 26, 2020
* upstream/master:
  Prints the Symbol name into the error message with a custom asymmetric matcher (jestjs#9888)
  Show coverage of sources related to tests in changed files (jestjs#9769)
  fix: don't /// <reference types="jest" /> (jestjs#9875)
jeysal added a commit to mmkal/jest that referenced this pull request Apr 26, 2020
…pshots

* upstream/master: (39 commits)
  Prints the Symbol name into the error message with a custom asymmetric matcher (jestjs#9888)
  Show coverage of sources related to tests in changed files (jestjs#9769)
  fix: don't /// <reference types="jest" /> (jestjs#9875)
  noCodeFrame respects noStackTrace (jestjs#9866)
  chore: update example to react-native@0.62 (jestjs#9746)
  Improve source map handling when instrumenting transformed code (jestjs#9811)
  Update .vscode/launch.json settings (jestjs#9868)
  chore: verify all packages have the same engine requirement (jestjs#9871)
  fix: pass custom cached realpath function to `resolve` (jestjs#9873)
  chore: mock stealthy-require in tests (jestjs#9855)
  chore: update resolve (jestjs#9872)
  chore: run CircleCI on node 14 (jestjs#9870)
  Add an option to vscode settings to always use workspace TS (jestjs#9869)
  fix(esm): handle parallel imports (jestjs#9858)
  chore: run CI on Node 14 (jestjs#9861)
  feat: add `@jest/globals` package for importing globals explici… (jestjs#9849)
  chore: bump resolve package (jestjs#9859)
  chore(runtime): simplify `createJestObjectFor` (jestjs#9857)
  chore: fix symlink creation failures on windows in tests (jestjs#9852)
  chore: skip mercurial tests when no hg installed (jestjs#9840)
  ...
jeysal added a commit to mmkal/jest that referenced this pull request Apr 26, 2020
…pshots

* upstream/master: (39 commits)
  Prints the Symbol name into the error message with a custom asymmetric matcher (jestjs#9888)
  Show coverage of sources related to tests in changed files (jestjs#9769)
  fix: don't /// <reference types="jest" /> (jestjs#9875)
  noCodeFrame respects noStackTrace (jestjs#9866)
  chore: update example to react-native@0.62 (jestjs#9746)
  Improve source map handling when instrumenting transformed code (jestjs#9811)
  Update .vscode/launch.json settings (jestjs#9868)
  chore: verify all packages have the same engine requirement (jestjs#9871)
  fix: pass custom cached realpath function to `resolve` (jestjs#9873)
  chore: mock stealthy-require in tests (jestjs#9855)
  chore: update resolve (jestjs#9872)
  chore: run CircleCI on node 14 (jestjs#9870)
  Add an option to vscode settings to always use workspace TS (jestjs#9869)
  fix(esm): handle parallel imports (jestjs#9858)
  chore: run CI on Node 14 (jestjs#9861)
  feat: add `@jest/globals` package for importing globals explici… (jestjs#9849)
  chore: bump resolve package (jestjs#9859)
  chore(runtime): simplify `createJestObjectFor` (jestjs#9857)
  chore: fix symlink creation failures on windows in tests (jestjs#9852)
  chore: skip mercurial tests when no hg installed (jestjs#9840)
  ...
@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 11, 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.

Make test files change trigger coverage of source code under watch mode and version control
5 participants