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
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -3,6 +3,7 @@
### Features

- `[@jest/globals]` New package so Jest's globals can be explicitly imported ([#9801](https://github.com/facebook/jest/pull/9801))
- `[jest-core]` Show coverage of sources related to tests in changed files ([#9769](https://github.com/facebook/jest/pull/9769))
- `[jest-runtime]` Populate `require.cache` ([#9841](https://github.com/facebook/jest/pull/9841))

### Fixes
Expand Down
33 changes: 33 additions & 0 deletions e2e/__tests__/onlyChanged.test.ts
Expand Up @@ -137,6 +137,39 @@ test('report test coverage for only changed files', () => {
expect(stdout).not.toMatch('b.js');
});

test('report test coverage of source on test file change under only changed files', () => {
writeFiles(DIR, {
'__tests__/a.test.js': `
require('../a');
test('a1', () => expect(1).toBe(1));
`,
'a.js': 'module.exports = {}',
'package.json': JSON.stringify({
jest: {
collectCoverage: true,
coverageReporters: ['text'],
testEnvironment: 'node',
},
}),
});

run(`${GIT} init`, DIR);
run(`${GIT} add .`, DIR);
run(`${GIT} commit --no-gpg-sign -m "first"`, DIR);

writeFiles(DIR, {
'__tests__/a.test.js': `
require('../a');
test('a1', () => expect(1).toBe(1));
test('a2', () => expect(2).toBe(2));
`,
});

const {stdout} = runJest(DIR, ['--only-changed']);

expect(stdout).toMatch('a.js');
});

test('do not pickup non-tested files when reporting coverage on only changed files', () => {
writeFiles(DIR, {
'a.js': 'module.exports = {}',
Expand Down
45 changes: 37 additions & 8 deletions packages/jest-core/src/SearchSource.ts
Expand Up @@ -51,6 +51,13 @@ const toTests = (context: Context, tests: Array<Config.Path>) =>
path,
}));

const hasSCM = (changedFilesInfo: ChangedFiles) => {
const {repos} = changedFilesInfo;
// no SCM (git/hg/...) is found in any of the roots.
const noSCM = Object.values(repos).every(scm => scm.size === 0);
return !noSCM;
};

export default class SearchSource {
private _context: Context;
private _testPathCases: TestPathCases = [];
Expand Down Expand Up @@ -240,14 +247,11 @@ export default class SearchSource {
changedFilesInfo: ChangedFiles,
collectCoverage: boolean,
): SearchResult {
const {repos, changedFiles} = changedFilesInfo;
// no SCM (git/hg/...) is found in any of the roots.
const noSCM = (Object.keys(repos) as Array<
keyof ChangedFiles['repos']
>).every(scm => repos[scm].size === 0);
return noSCM
? {noSCM: true, tests: []}
: this.findRelatedTests(changedFiles, collectCoverage);
if (!hasSCM(changedFilesInfo)) {
return {noSCM: true, tests: []};
}
const {changedFiles} = changedFilesInfo;
return this.findRelatedTests(changedFiles, collectCoverage);
}

private _getTestPaths(
Expand Down Expand Up @@ -328,4 +332,29 @@ export default class SearchSource {

return searchResult;
}

findRelatedSourcesFromTestsInChangedFiles(
changedFilesInfo: ChangedFiles,
): Array<string> {
if (!hasSCM(changedFilesInfo)) {
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?

this._context.resolver,
this._context.hasteFS,
buildSnapshotResolver(this._context.config),
);
const relatedSourcesSet = new Set<string>();
changedFiles.forEach(filePath => {
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) {

const sourcePaths = dependencyResolver.resolve(filePath, {
skipNodeResolution: this._context.config.skipNodeResolution,
});
sourcePaths.forEach(sourcePath => relatedSourcesSet.add(sourcePath));
}
});
return Array.from(relatedSourcesSet);
}
}
13 changes: 10 additions & 3 deletions packages/jest-core/src/TestScheduler.ts
Expand Up @@ -44,6 +44,7 @@ export type TestSchedulerContext = {
firstRun: boolean;
previousSuccess: boolean;
changedFiles?: Set<Config.Path>;
sourcesRelatedToTestsInChangedFiles?: Set<Config.Path>;
};
export default class TestScheduler {
private _dispatcher: ReporterDispatcher;
Expand Down Expand Up @@ -180,7 +181,9 @@ export default class TestScheduler {
if (!testRunners[config.runner]) {
const Runner: typeof TestRunner = require(config.runner);
testRunners[config.runner] = new Runner(this._globalConfig, {
changedFiles: this._context && this._context.changedFiles,
changedFiles: this._context?.changedFiles,
sourcesRelatedToTestsInChangedFiles: this._context
?.sourcesRelatedToTestsInChangedFiles,
});
}
});
Expand Down Expand Up @@ -272,7 +275,9 @@ export default class TestScheduler {
if (!isDefault && collectCoverage) {
this.addReporter(
new CoverageReporter(this._globalConfig, {
changedFiles: this._context && this._context.changedFiles,
changedFiles: this._context?.changedFiles,
sourcesRelatedToTestsInChangedFiles: this._context
?.sourcesRelatedToTestsInChangedFiles,
}),
);
}
Expand Down Expand Up @@ -302,7 +307,9 @@ export default class TestScheduler {
if (collectCoverage) {
this.addReporter(
new CoverageReporter(this._globalConfig, {
changedFiles: this._context && this._context.changedFiles,
changedFiles: this._context?.changedFiles,
sourcesRelatedToTestsInChangedFiles: this._context
?.sourcesRelatedToTestsInChangedFiles,
}),
);
}
Expand Down
61 changes: 61 additions & 0 deletions packages/jest-core/src/__tests__/SearchSource.test.ts
Expand Up @@ -531,4 +531,65 @@ describe('SearchSource', () => {
}
});
});

describe('findRelatedSourcesFromTestsInChangedFiles', () => {
const rootDir = path.resolve(
__dirname,
'../../../jest-runtime/src/__tests__/test_root',
);

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


it('return empty set if no SCM', () => {
const requireRegularModule = path.join(
rootDir,
'RequireRegularModule.js',
);
const sources = searchSource.findRelatedSourcesFromTestsInChangedFiles({
changedFiles: new Set([requireRegularModule]),
repos: {
git: new Set(),
hg: new Set(),
},
});
expect(sources).toEqual([]);
});

it('return sources required by tests', () => {
const regularModule = path.join(rootDir, 'RegularModule.js');
const requireRegularModule = path.join(
rootDir,
'RequireRegularModule.js',
);
const sources = searchSource.findRelatedSourcesFromTestsInChangedFiles({
changedFiles: new Set([requireRegularModule]),
repos: {
git: new Set('/path/to/git'),
hg: new Set(),
},
});
expect(sources).toEqual([regularModule]);
});
});
});
19 changes: 16 additions & 3 deletions packages/jest-core/src/runJest.ts
Expand Up @@ -242,9 +242,22 @@ export default async function runJest({
}

if (changedFilesPromise) {
testSchedulerContext.changedFiles = (
await changedFilesPromise
).changedFiles;
const changedFilesInfo = await changedFilesPromise;
if (changedFilesInfo.changedFiles) {
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.

const relatedSourceFromTestsInChangedFiles = searchSource.findRelatedSourcesFromTestsInChangedFiles(
changedFilesInfo,
);
return relatedSourceFromTestsInChangedFiles;
})
.reduce((total, paths) => total.concat(paths), []);
testSchedulerContext.sourcesRelatedToTestsInChangedFiles = new Set(
sourcesRelatedToTestsInChangedFilesArray,
);
}
}

const results = await new TestScheduler(
Expand Down
Expand Up @@ -41,6 +41,7 @@ test('resolves to the result of generateEmptyCoverage upon success', async () =>
globalConfig,
config,
undefined,
undefined,
);

expect(result).toEqual(42);
Expand Down
3 changes: 3 additions & 0 deletions packages/jest-reporters/src/coverage_reporter.ts
Expand Up @@ -204,6 +204,9 @@ export default class CoverageReporter extends BaseReporter {
changedFiles:
this._options.changedFiles &&
Array.from(this._options.changedFiles),
sourcesRelatedToTestsInChangedFiles:
this._options.sourcesRelatedToTestsInChangedFiles &&
Array.from(this._options.sourcesRelatedToTestsInChangedFiles),
},
path: filename,
});
Expand Down
4 changes: 3 additions & 1 deletion packages/jest-reporters/src/coverage_worker.ts
Expand Up @@ -40,6 +40,8 @@ export function worker({
path,
globalConfig,
config,
options && options.changedFiles && new Set(options.changedFiles),
options?.changedFiles && new Set(options.changedFiles),
options?.sourcesRelatedToTestsInChangedFiles &&
new Set(options.sourcesRelatedToTestsInChangedFiles),
);
}
2 changes: 2 additions & 0 deletions packages/jest-reporters/src/generateEmptyCoverage.ts
Expand Up @@ -31,13 +31,15 @@ export default function (
globalConfig: Config.GlobalConfig,
config: Config.ProjectConfig,
changedFiles?: Set<Config.Path>,
sourcesRelatedToTestsInChangedFiles?: Set<Config.Path>,
): CoverageWorkerResult | null {
const coverageOptions = {
changedFiles,
collectCoverage: globalConfig.collectCoverage,
collectCoverageFrom: globalConfig.collectCoverageFrom,
collectCoverageOnlyFrom: globalConfig.collectCoverageOnlyFrom,
coverageProvider: globalConfig.coverageProvider,
sourcesRelatedToTestsInChangedFiles,
};
let coverageWorkerResult: CoverageWorkerResult | null = null;
if (shouldInstrument(filename, coverageOptions, config)) {
Expand Down
2 changes: 2 additions & 0 deletions packages/jest-reporters/src/types.ts
Expand Up @@ -37,10 +37,12 @@ export type CoverageWorker = {worker: typeof worker};

export type CoverageReporterOptions = {
changedFiles?: Set<Config.Path>;
sourcesRelatedToTestsInChangedFiles?: Set<Config.Path>;
};

export type CoverageReporterSerializedOptions = {
changedFiles?: Array<Config.Path>;
sourcesRelatedToTestsInChangedFiles?: Array<Config.Path>;
};

export type OnTestStart = (test: Test) => Promise<void>;
Expand Down
4 changes: 3 additions & 1 deletion packages/jest-runner/src/runTest.ts
Expand Up @@ -147,11 +147,13 @@ async function runTestInternal(
setGlobal(environment.global, 'console', testConsole);

const runtime = new Runtime(config, environment, resolver, cacheFS, {
changedFiles: context && context.changedFiles,
changedFiles: context?.changedFiles,
collectCoverage: globalConfig.collectCoverage,
collectCoverageFrom: globalConfig.collectCoverageFrom,
collectCoverageOnlyFrom: globalConfig.collectCoverageOnlyFrom,
coverageProvider: globalConfig.coverageProvider,
sourcesRelatedToTestsInChangedFiles:
context?.sourcesRelatedToTestsInChangedFiles,
});

const start = Date.now();
Expand Down
1 change: 1 addition & 0 deletions packages/jest-runner/src/types.ts
Expand Up @@ -51,6 +51,7 @@ export type TestRunnerOptions = {

export type TestRunnerContext = {
changedFiles?: Set<Config.Path>;
sourcesRelatedToTestsInChangedFiles?: Set<Config.Path>;
};

export type TestRunnerSerializedContext = {
Expand Down
1 change: 1 addition & 0 deletions packages/jest-runtime/src/index.ts
Expand Up @@ -171,6 +171,7 @@ class Runtime {
collectCoverageFrom: [],
collectCoverageOnlyFrom: undefined,
coverageProvider: 'babel',
sourcesRelatedToTestsInChangedFiles: undefined,
};
this._currentlyExecutingModulePath = '';
this._environment = environment;
Expand Down
11 changes: 9 additions & 2 deletions packages/jest-transform/src/shouldInstrument.ts
Expand Up @@ -93,8 +93,15 @@ export default function shouldInstrument(
return false;
}

if (options.changedFiles && !options.changedFiles.has(filename)) {
return false;
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)) {

if (!options.sourcesRelatedToTestsInChangedFiles) {
return false;
}
if (!options.sourcesRelatedToTestsInChangedFiles.has(filename)) {
return false;
}
}
}

return true;
Expand Down
1 change: 1 addition & 0 deletions packages/jest-transform/src/types.ts
Expand Up @@ -16,6 +16,7 @@ export type ShouldInstrumentOptions = Pick<
| 'coverageProvider'
> & {
changedFiles?: Set<Config.Path>;
sourcesRelatedToTestsInChangedFiles?: Set<Config.Path>;
};

export type Options = ShouldInstrumentOptions &
Expand Down