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.

Sign up for GitHub

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

Jump to bottom

chore: make changedFiles option optional in shouldInstrument #9197

Merged
merged 3 commits into from
Nov 17, 2019
Merged

chore: make changedFiles option optional in shouldInstrument #9197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
454a68e
chore: make changedFiles option optional instead of `|undefined` in `…
SimenB Nov 17, 2019
6e7347e
always set collectCoverageFrom
SimenB Nov 17, 2019
cfc6848
update snap
SimenB Nov 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/showConfig.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ exports[`--showConfig outputs config info and exits 1`] = `
"bail": 0,
"changedFilesWithAncestor": false,
"collectCoverage": false,
"collectCoverageFrom": null,
"collectCoverageFrom": [],
"coverageDirectory": "<<REPLACED_ROOT_DIR>>/coverage",
"coverageReporters": [
"json",
Expand Down
2 changes: 2 additions & 0 deletions packages/jest-config/src/normalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,8 @@ export default function normalize(
}

newOptions.collectCoverageFrom = collectCoverageFrom;
} else if (!newOptions.collectCoverageFrom) {
newOptions.collectCoverageFrom = [];
}

return {
Expand Down
20 changes: 11 additions & 9 deletions ...ime/src/__tests__/instrumentation.test.js → ...ime/src/__tests__/instrumentation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@
*
*/

'use strict';

import vm from 'vm';
import path from 'path';
import os from 'os';
import * as vm from 'vm';
import * as path from 'path';
import * as os from 'os';
import {ScriptTransformer} from '@jest/transform';
import {makeProjectConfig} from '../../../../TestUtils';
import {makeGlobalConfig, makeProjectConfig} from '../../../../TestUtils';

jest.mock('vm');

Expand All @@ -27,9 +25,13 @@ it('instruments files', () => {
cacheDirectory: os.tmpdir(),
rootDir: '/',
});
const instrumented = new ScriptTransformer(
config,
).transform(FILE_PATH_TO_INSTRUMENT, {collectCoverage: true}).script;
const instrumented = new ScriptTransformer(config).transform(
FILE_PATH_TO_INSTRUMENT,
{
...makeGlobalConfig({collectCoverage: true}),
changedFiles: undefined,
},
).script;
expect(instrumented instanceof vm.Script).toBe(true);
// We can't really snapshot the resulting coverage, because it depends on
// absolute path of the file, which will be different on different
Expand Down
9 changes: 1 addition & 8 deletions packages/jest-runtime/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,7 @@ class Runtime {
}
}

// TODO: Make this `static shouldInstrument = shouldInstrument;` after https://github.com/facebook/jest/issues/7846
static shouldInstrument(
filename: Config.Path,
options: ShouldInstrumentOptions,
config: Config.ProjectConfig,
) {
return shouldInstrument(filename, options, config);
}
static shouldInstrument = shouldInstrument;

static createContext(
config: Config.ProjectConfig,
Expand Down
89 changes: 57 additions & 32 deletions packages/jest-transform/src/__tests__/script_transformer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
*
*/

'use strict';

import {makeProjectConfig} from '../../../../TestUtils';
import {makeGlobalConfig, makeProjectConfig} from '../../../../TestUtils';

jest
.mock('fs', () =>
Expand Down Expand Up @@ -214,9 +212,10 @@ describe('ScriptTransformer', () => {

it('transforms a file properly', () => {
const scriptTransformer = new ScriptTransformer(config);
const response = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
}).script;
const response = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({collectCoverage: true}),
).script;

expect(response instanceof vm.Script).toBe(true);
expect(vm.Script.mock.calls[0][0]).toMatchSnapshot();
Expand All @@ -226,24 +225,32 @@ describe('ScriptTransformer', () => {
expect(fs.readFileSync).toBeCalledWith('/fruits/banana.js', 'utf8');

// in-memory cache
const response2 = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
}).script;
const response2 = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({collectCoverage: true}),
).script;
expect(response2).toBe(response);

scriptTransformer.transform('/fruits/kiwi.js', {
collectCoverage: true,
});
scriptTransformer.transform(
'/fruits/kiwi.js',
makeGlobalConfig({collectCoverage: true}),
);
const snapshot = vm.Script.mock.calls[1][0];
expect(snapshot).toMatchSnapshot();

scriptTransformer.transform('/fruits/kiwi.js', {collectCoverage: true});
scriptTransformer.transform(
'/fruits/kiwi.js',
makeGlobalConfig({collectCoverage: true}),
);

expect(vm.Script.mock.calls[0][0]).not.toEqual(snapshot);
expect(vm.Script.mock.calls[0][0]).not.toMatch(/instrumented kiwi/);

// If we disable coverage, we get a different result.
scriptTransformer.transform('/fruits/kiwi.js', {collectCoverage: false});
scriptTransformer.transform(
'/fruits/kiwi.js',
makeGlobalConfig({collectCoverage: false}),
);
expect(vm.Script.mock.calls[1][0]).toEqual(snapshot);

scriptTransformer.transform('/fruits/banana.js', {
Expand Down Expand Up @@ -401,9 +408,12 @@ describe('ScriptTransformer', () => {
map,
});

const result = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
const result = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);
expect(result.sourceMapPath).toEqual(expect.any(String));
const mapStr = JSON.stringify(map);
expect(writeFileAtomic.sync).toBeCalledTimes(2);
Expand Down Expand Up @@ -431,9 +441,12 @@ describe('ScriptTransformer', () => {

require('preprocessor-with-sourcemaps').process.mockReturnValue(content);

const result = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
const result = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);
expect(result.sourceMapPath).toEqual(expect.any(String));
expect(writeFileAtomic.sync).toBeCalledTimes(2);
expect(writeFileAtomic.sync).toBeCalledWith(
Expand Down Expand Up @@ -468,9 +481,12 @@ describe('ScriptTransformer', () => {

require('preprocessor-with-sourcemaps').process.mockReturnValue(content);

const result = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
const result = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);
expect(result.sourceMapPath).toBeNull();
expect(writeFileAtomic.sync).toBeCalledTimes(1);

Expand All @@ -496,9 +512,12 @@ describe('ScriptTransformer', () => {
map,
});

const result = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
const result = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);
expect(result.sourceMapPath).toEqual(expect.any(String));
expect(writeFileAtomic.sync).toBeCalledTimes(2);
expect(writeFileAtomic.sync).toBeCalledWith(
Expand All @@ -522,9 +541,12 @@ describe('ScriptTransformer', () => {
map: null,
});

const result = scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
const result = scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);
expect(result.sourceMapPath).toBeFalsy();
expect(writeFileAtomic.sync).toHaveBeenCalledTimes(1);
});
Expand All @@ -533,9 +555,12 @@ describe('ScriptTransformer', () => {
config = {...config, transform: [['^.+\\.js$', 'test_preprocessor']]};
const scriptTransformer = new ScriptTransformer(config);

scriptTransformer.transform('/fruits/banana.js', {
collectCoverage: true,
});
scriptTransformer.transform(
'/fruits/banana.js',
makeGlobalConfig({
collectCoverage: true,
}),
);

const {getCacheKey} = require('test_preprocessor');
expect(getCacheKey.mock.calls[0][3]).toMatchSnapshot();
Expand Down
20 changes: 12 additions & 8 deletions ...m/src/__tests__/should_instrument.test.js → ...m/src/__tests__/should_instrument.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,27 @@
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

import {Config} from '@jest/types';
import shouldInstrument from '../shouldInstrument';
import {makeProjectConfig} from '../../../../TestUtils';
import {makeGlobalConfig, makeProjectConfig} from '../../../../TestUtils';
import {Options} from '../types';

describe('shouldInstrument', () => {
const defaultFilename = 'source_file.test.js';
const defaultOptions = {
collectCoverage: true,
const defaultOptions: Options = {
...makeGlobalConfig({
collectCoverage: true,
}),
changedFiles: undefined,
};
const defaultConfig = makeProjectConfig({rootDir: '/'});
describe('should return true', () => {
const testShouldInstrument = (
filename = defaultFilename,
options,
config,
options: Partial<Options>,
config: Partial<Config.ProjectConfig>,
) => {
const result = shouldInstrument(
filename,
Expand Down Expand Up @@ -110,8 +114,8 @@ describe('shouldInstrument', () => {
describe('should return false', () => {
const testShouldInstrument = (
filename = defaultFilename,
options,
config,
options: Partial<Options>,
config: Partial<Config.ProjectConfig>,
) => {
const result = shouldInstrument(
filename,
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-transform/src/shouldInstrument.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default function shouldInstrument(
if (
// still cover if `only` is specified
!options.collectCoverageOnlyFrom &&
options.collectCoverageFrom &&
options.collectCoverageFrom.length &&
Copy link
Member Author

Choose a reason for hiding this comment

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

sorta breaking? Only for people calling this manually though, and they'd get a type error. Fine for Jest 25.

Also a bug fix if you forgot to send it in

Copy link
Member Author

Choose a reason for hiding this comment

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

well, Jest didn't always pass it, so that's nice :P Looking forward to making normalize in jest-config actually be type safe...

micromatch(
[replacePathSepForGlob(path.relative(config.rootDir, filename))],
options.collectCoverageFrom,
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-transform/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export type ShouldInstrumentOptions = Pick<
Config.GlobalConfig,
'collectCoverage' | 'collectCoverageFrom' | 'collectCoverageOnlyFrom'
> & {
changedFiles: Set<Config.Path> | undefined;
changedFiles?: Set<Config.Path>;
};

export type Options = ShouldInstrumentOptions &
Expand Down