Refactor -o & --coverage implementation | Fixes #6859 (#7611)

Author: MarcoScabbiolo

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 ### Fixes
 
+- `[jest-cli]` Refactor `-o` and `--coverage` combined ([#7611](https://github.com/facebook/jest/pull/7611))
+
 ### Chore & Maintenance
 
 - `[*]`: Setup building, linting and testing of TypeScript ([#7808](https://github.com/facebook/jest/pull/7808))

e2e/__tests__/onlyChanged.test.js

@@ -139,6 +139,30 @@ test('report test coverage for only changed files', () => {
   expect(stdout).not.toMatch('b.js');
 });
 
+test('do not pickup non-tested files when reporting coverage on only changed files', () => {
+  writeFiles(DIR, {
+    'a.js': 'module.exports = {}',
+    'b.test.js': 'module.exports = {}',
+    'package.json': JSON.stringify({name: 'original name'}),
+  });
+
+  run(`${GIT} init`, DIR);
+  run(`${GIT} add .`, DIR);
+  run(`${GIT} commit --no-gpg-sign -m "first"`, DIR);
+
+  writeFiles(DIR, {
+    'b.test.js': 'require("./package.json"); it("passes", () => {})',
+    'package.json': JSON.stringify({name: 'new name'}),
+  });
+
+  const {stderr, stdout} = runJest(DIR, ['-o', '--coverage']);
+
+  expect(stderr).toEqual(
+    expect.not.stringContaining('Failed to collect coverage from'),
+  );
+  expect(stdout).toEqual(expect.not.stringContaining('package.json'));
+});
+
 test('onlyChanged in config is overwritten by --all or testPathPattern', () => {
   writeFiles(DIR, {
     '.watchmanconfig': '',

packages/jest-cli/src/SearchSource.js

@@ -10,7 +10,7 @@
 import type {Context} from 'types/Context';
 import type {Glob, GlobalConfig, Path} from 'types/Config';
 import type {Test} from 'types/TestRunner';
-import type {ChangedFilesPromise} from 'types/ChangedFiles';
+import type {ChangedFilesInfo} from 'types/ChangedFiles';
 
 import path from 'path';
 import micromatch from 'micromatch';
@@ -24,7 +24,7 @@ import {replacePathSepForGlob} from 'jest-util';
 type SearchResult = {|
   noSCM?: boolean,
   stats?: {[key: string]: number},
-  collectCoverageFrom?: Array<string>,
+  collectCoverageFrom?: Set<string>,
   tests: Array<Test>,
   total?: number,
 |};
@@ -158,29 +158,55 @@ export default class SearchSource {
       buildSnapshotResolver(this._context.config),
     );
 
-    const tests = toTests(
-      this._context,
-      dependencyResolver.resolveInverse(
-        allPaths,
-        this.isTestFilePath.bind(this),
-        {
-          skipNodeResolution: this._context.config.skipNodeResolution,
-        },
-      ),
-    );
-    let collectCoverageFrom;
-
-    // If we are collecting coverage, also return collectCoverageFrom patterns
-    if (collectCoverage) {
-      collectCoverageFrom = Array.from(allPaths).map(filename => {
-        filename = replaceRootDirInPath(this._context.config.rootDir, filename);
-        return path.isAbsolute(filename)
-          ? path.relative(this._context.config.rootDir, filename)
-          : filename;
-      });
+    if (!collectCoverage) {
+      return {
+        tests: toTests(
+          this._context,
+          dependencyResolver.resolveInverse(
+            allPaths,
+            this.isTestFilePath.bind(this),
+            {skipNodeResolution: this._context.config.skipNodeResolution},
+          ),
+        ),
+      };
     }
 
-    return {collectCoverageFrom, tests};
+    const testModulesMap = dependencyResolver.resolveInverseModuleMap(
+      allPaths,
+      this.isTestFilePath.bind(this),
+      {skipNodeResolution: this._context.config.skipNodeResolution},
+    );
+
+    const allPathsAbsolute = Array.from(allPaths).map(p => path.resolve(p));
+
+    const collectCoverageFrom = new Set();
+
+    testModulesMap.forEach(testModule => {
+      if (!testModule.dependencies) {
+        return;
+      }
+
+      testModule.dependencies
+        .filter(p => allPathsAbsolute.includes(p))
+        .map(filename => {
+          filename = replaceRootDirInPath(
+            this._context.config.rootDir,
+            filename,
+          );
+          return path.isAbsolute(filename)
+            ? path.relative(this._context.config.rootDir, filename)
+            : filename;
+        })
+        .forEach(filename => collectCoverageFrom.add(filename));
+    });
+
+    return {
+      collectCoverageFrom,
+      tests: toTests(
+        this._context,
+        testModulesMap.map(testModule => testModule.file),
+      ),
+    };
   }
 
   findTestsByPaths(paths: Array<Path>): SearchResult {
@@ -207,11 +233,11 @@ export default class SearchSource {
     return {tests: []};
   }
 
-  async findTestRelatedToChangedFiles(
-    changedFilesPromise: ChangedFilesPromise,
+  findTestRelatedToChangedFiles(
+    changedFilesInfo: ChangedFilesInfo,
     collectCoverage: boolean,
   ) {
-    const {repos, changedFiles} = await changedFilesPromise;
+    const {repos, changedFiles} = changedFilesInfo;
     // no SCM (git/hg/...) is found in any of the roots.
     const noSCM = Object.keys(repos).every(scm => repos[scm].size === 0);
     return noSCM
@@ -221,42 +247,38 @@ export default class SearchSource {
 
   _getTestPaths(
     globalConfig: GlobalConfig,
-    changedFilesPromise: ?ChangedFilesPromise,
-  ): Promise<SearchResult> {
+    changedFiles: ?ChangedFilesInfo,
+  ): SearchResult {
     const paths = globalConfig.nonFlagArgs;
 
     if (globalConfig.onlyChanged) {
-      if (!changedFilesPromise) {
-        throw new Error('This promise must be present when running with -o.');
+      if (!changedFiles) {
+        throw new Error('Changed files must be set when running with -o.');
       }
 
       return this.findTestRelatedToChangedFiles(
-        changedFilesPromise,
+        changedFiles,
         globalConfig.collectCoverage,
       );
     } else if (globalConfig.runTestsByPath && paths && paths.length) {
-      return Promise.resolve(this.findTestsByPaths(paths));
+      return this.findTestsByPaths(paths);
     } else if (globalConfig.findRelatedTests && paths && paths.length) {
-      return Promise.resolve(
-        this.findRelatedTestsFromPattern(paths, globalConfig.collectCoverage),
+      return this.findRelatedTestsFromPattern(
+        paths,
+        globalConfig.collectCoverage,
       );
     } else if (globalConfig.testPathPattern != null) {
-      return Promise.resolve(
-        this.findMatchingTests(globalConfig.testPathPattern),
-      );
+      return this.findMatchingTests(globalConfig.testPathPattern);
     } else {
-      return Promise.resolve({tests: []});
+      return {tests: []};
     }
   }
 
   async getTestPaths(
     globalConfig: GlobalConfig,
-    changedFilesPromise: ?ChangedFilesPromise,
+    changedFiles: ?ChangedFilesInfo,
   ): Promise<SearchResult> {
-    const searchResult = await this._getTestPaths(
-      globalConfig,
-      changedFilesPromise,
-    );
+    const searchResult = this._getTestPaths(globalConfig, changedFiles);
 
     const filterPath = globalConfig.filter;
 

packages/jest-cli/src/TestScheduler.js

@@ -8,7 +8,7 @@
  */
 
 import type {AggregatedResult, TestResult} from 'types/TestResult';
-import type {GlobalConfig, ReporterConfig} from 'types/Config';
+import type {GlobalConfig, ReporterConfig, Path} from 'types/Config';
 import type {Context} from 'types/Context';
 import type {Reporter, Test} from 'types/TestRunner';
 
@@ -41,6 +41,7 @@ export type TestSchedulerOptions = {|
 export type TestSchedulerContext = {|
   firstRun: boolean,
   previousSuccess: boolean,
+  changedFiles?: Set<Path>,
 |};
 export default class TestScheduler {
   _dispatcher: ReporterDispatcher;
@@ -173,6 +174,7 @@ export default class TestScheduler {
         // $FlowFixMe
         testRunners[config.runner] = new (require(config.runner): TestRunner)(
           this._globalConfig,
+          {changedFiles: this._context && this._context.changedFiles},
         );
       }
     });
@@ -262,7 +264,11 @@ export default class TestScheduler {
     }
 
     if (!isDefault && collectCoverage) {
-      this.addReporter(new CoverageReporter(this._globalConfig));
+      this.addReporter(
+        new CoverageReporter(this._globalConfig, {
+          changedFiles: this._context && this._context.changedFiles,
+        }),
+      );
     }
 
     if (notify) {
@@ -288,7 +294,11 @@ export default class TestScheduler {
     );
 
     if (collectCoverage) {
-      this.addReporter(new CoverageReporter(this._globalConfig));
+      this.addReporter(
+        new CoverageReporter(this._globalConfig, {
+          changedFiles: this._context && this._context.changedFiles,
+        }),
+      );
     }
 
     this.addReporter(new SummaryReporter(this._globalConfig));

packages/jest-cli/src/__tests__/SearchSource.test.js

@@ -419,6 +419,20 @@ describe('SearchSource', () => {
         rootPath,
       ]);
     });
+
+    it('excludes untested files from coverage', () => {
+      const unrelatedFile = path.join(rootDir, 'JSONFile.json');
+      const regular = path.join(rootDir, 'RegularModule.js');
+      const requireRegular = path.join(rootDir, 'RequireRegularMode.js');
+
+      const data = searchSource.findRelatedTests(
+        new Set([regular, requireRegular, unrelatedFile]),
+        true,
+      );
+      expect(Array.from(data.collectCoverageFrom)).toEqual([
+        'RegularModule.js',
+      ]);
+    });
   });
 
   describe('findRelatedTestsFromPattern', () => {