Skip to content
Prev Previous commit
Next Next commit
* Maintaining internal mapCoverage, map only for files which are inst…
…rumented by us

* Feat: Return sourcemap from babel-jest
  • Loading branch information
Aftabnack committed Feb 15, 2018
commit fdefcaac5cbf7725cec9844eb215ff2e31fdd1f4
3 changes: 0 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,9 @@

### Chore & Maintenance

<<<<<<< HEAD
* `[filenames]` Standardize file names in root
([#5500](https://github.com/facebook/jest/pull/5500))

=======
>>>>>>> 429db664... * Removed usage of mapCoverage.
## jest 22.2.1

### Fixes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

const {readFileSync} = require('fs');
const path = require('path');
const skipOnWindows = require('../../scripts/skip_on_windows');
const skipOnWindows = require('../../scripts/SkipOnWindows');
const {cleanup, run} = require('../utils');
const runJest = require('../runJest');

Expand Down
17 changes: 8 additions & 9 deletions packages/babel-jest/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const createTransformer = (options: any): Transformer => {
options = Object.assign({}, options, {
plugins: (options && options.plugins) || [],
presets: ((options && options.presets) || []).concat([jestPreset]),
sourceMaps: 'both',
});
delete options.cacheDirectory;
delete options.filename;
Expand Down Expand Up @@ -110,18 +111,11 @@ const createTransformer = (options: any): Transformer => {
const altExts = config.moduleFileExtensions.map(
extension => '.' + extension,
);
const shouldUseInline =
transformOptions &&
(transformOptions.returnSourceString == null ||
transformOptions.returnSourceString);
const sourceMapOpts = {
sourceMaps: shouldUseInline ? 'inline' : true,
};
if (babelUtil && !babelUtil.canCompile(filename, altExts)) {
return src;
}

const theseOptions = Object.assign({filename}, options, sourceMapOpts);
const theseOptions = Object.assign({filename}, options);
if (transformOptions && transformOptions.instrument) {
theseOptions.auxiliaryCommentBefore = ' istanbul ignore next ';
// Copied from jest-runtime transform.js
Expand All @@ -144,7 +138,12 @@ const createTransformer = (options: any): Transformer => {
return src;
}

return shouldUseInline ? transformResult.code : transformResult;
const shouldReturnCodeOnly =
Copy link
Member

Choose a reason for hiding this comment

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

I think this condition is wrong.

Should be

const shouldReturnCodeOnly = !transformOptions || transformOptions.returnSourceString == null || transformOptions.returnSourceString;

Copy link
Member

Choose a reason for hiding this comment

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

so that if transformOptions is not passed in shouldReturnCodeOnly === true

Copy link
Member

Choose a reason for hiding this comment

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

potentially

const shouldReturnCodeOnly =
  transformOptions == null ||
  transformOptions.returnSourceString == null ||
  transformOptions.returnSourceString === true;

to be explicit

Copy link
Member

Choose a reason for hiding this comment

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

Waiting for this change before merging.

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'm outside. I'll change n push in an hour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pushed!

Copy link
Member

Choose a reason for hiding this comment

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

we should have a test for this - mind adding one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will add and push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

transformOptions &&
(transformOptions.returnSourceString == null ||
transformOptions.returnSourceString);

return shouldReturnCodeOnly ? transformResult.code : transformResult;
},
};
};
Expand Down
20 changes: 7 additions & 13 deletions packages/jest-cli/src/reporters/coverage_reporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@ export default class CoverageReporter extends BaseReporter {
delete testResult.coverage;

Object.keys(testResult.sourceMaps).forEach(sourcePath => {
let coverage: FileCoverage, inputSourceMap: ?Object;
let inputSourceMap: ?Object;
try {
coverage = this._coverageMap.fileCoverageFor(sourcePath);
const coverage: FileCoverage = this._coverageMap.fileCoverageFor(
sourcePath,
);
({inputSourceMap} = coverage.toJSON());
} finally {
if (inputSourceMap) {
Expand All @@ -81,17 +83,9 @@ export default class CoverageReporter extends BaseReporter {
aggregatedResults: AggregatedResult,
) {
await this._addUntestedFiles(this._globalConfig, contexts);
let map = this._coverageMap;
let sourceFinder: Object;
const transformedCoverage = this._sourceMapStore.transformCoverage(map);

if (
transformedCoverage.map &&
transformedCoverage.map.data &&
Object.keys(transformedCoverage.map.data).length > 0
) {
({map, sourceFinder} = transformedCoverage);
}
const {map, sourceFinder} = this._sourceMapStore.transformCoverage(
this._coverageMap,
);

const reporter = createReporter();
try {
Expand Down
4 changes: 1 addition & 3 deletions packages/jest-runner/src/run_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,7 @@ async function runTestInternal(
result.perfStats = {end: Date.now(), start};
result.testFilePath = path;
result.coverage = runtime.getAllCoverageInfoCopy();
result.sourceMaps = runtime.getSourceMapInfo(
Object.keys(result.coverage || {}),
);
result.sourceMaps = runtime.getSourceMapInfo();
result.console = testConsole.getBuffer();
result.skipped = testCount === result.numPendingTests;
result.displayName = config.displayName;
Expand Down
9 changes: 7 additions & 2 deletions packages/jest-runtime/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class Runtime {
_mockRegistry: {[key: string]: any, __proto__: null};
_moduleMocker: ModuleMocker;
_moduleRegistry: ModuleRegistry;
_needsCoverageMapped: string[];
_resolver: Resolver;
_shouldAutoMock: boolean;
_shouldMockModuleCache: BooleanObject;
Expand Down Expand Up @@ -129,6 +130,7 @@ class Runtime {
this._mockRegistry = Object.create(null);
this._moduleMocker = this._environment.moduleMocker;
this._moduleRegistry = Object.create(null);
this._needsCoverageMapped = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a Set instead of an array for faster lookups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

this._resolver = resolver;
this._scriptTransformer = new ScriptTransformer(config);
this._shouldAutoMock = config.automock;
Expand Down Expand Up @@ -435,10 +437,10 @@ class Runtime {
return deepCyclicCopy(this._environment.global.__coverage__);
}

getSourceMapInfo(coveredFiles: Array<string>) {
getSourceMapInfo() {
return Object.keys(this._sourceMapRegistry).reduce((result, sourcePath) => {
if (
coveredFiles.includes(sourcePath) &&
this._needsCoverageMapped.includes(sourcePath) &&
fs.existsSync(this._sourceMapRegistry[sourcePath])
) {
result[sourcePath] = this._sourceMapRegistry[sourcePath];
Expand Down Expand Up @@ -533,6 +535,9 @@ class Runtime {

if (transformedFile.sourceMapPath) {
this._sourceMapRegistry[filename] = transformedFile.sourceMapPath;
if (transformedFile.mapCoverage) {
this._needsCoverageMapped.push(filename);
}
}

const wrapper = this._environment.runScript(transformedFile.script)[
Expand Down
25 changes: 19 additions & 6 deletions packages/jest-runtime/src/script_transformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,26 @@ export default class ScriptTransformer {
// Ignore cache if `config.cache` is set (--no-cache)
let code = this._config.cache ? readCodeCacheFile(cacheFilePath) : null;

const shouldCallTransform =
transform && shouldTransform(filename, this._config);

// That means that the transform has a custom instrumentation
// logic and will handle it based on `config.collectCoverage` option
const transformWillInstrument =
shouldCallTransform && transform && transform.canInstrument;

// If we handle the coverage instrumentation, we should try to map code
// coverage against original source with any provided source map
const mapCoverage = instrument && !transformWillInstrument;

if (code) {
// This is broken: we return the code, and a path for the source map
// directly from the cache. But, nothing ensures the source map actually
// matches that source code. They could have gotten out-of-sync in case
// two separate processes write concurrently to the same cache files.
return {
code,
mapCoverage,
sourceMapPath,
};
}
Expand All @@ -201,7 +214,7 @@ export default class ScriptTransformer {
map: null,
};

if (transform && shouldTransform(filename, this._config)) {
if (transform && shouldCallTransform) {
const processed = transform.process(content, filename, this._config, {
instrument,
returnSourceString: false,
Copy link
Member

Choose a reason for hiding this comment

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

We need this option to not have a breaking change on our hands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. But in the future it is going to be removed right? After giving proper deprecation messages?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should remove it in the next major of babel-jest :)

Expand All @@ -228,11 +241,7 @@ export default class ScriptTransformer {
}
}

// That means that the transform has a custom instrumentation
// logic and will handle it based on `config.collectCoverage` option
const transformDidInstrument = transform && transform.canInstrument;

if (!transformDidInstrument && instrument) {
if (!transformWillInstrument && instrument) {
code = this._instrumentFile(filename, transformed.code);
} else {
code = transformed.code;
Expand All @@ -252,6 +261,7 @@ export default class ScriptTransformer {

return {
code,
mapCoverage,
sourceMapPath,
};
}
Expand All @@ -270,6 +280,7 @@ export default class ScriptTransformer {

let wrappedCode: string;
let sourceMapPath: ?string = null;
let mapCoverage = false;

const willTransform =
!isInternalModule &&
Expand All @@ -286,11 +297,13 @@ export default class ScriptTransformer {

wrappedCode = wrap(transformedSource.code);
sourceMapPath = transformedSource.sourceMapPath;
mapCoverage = transformedSource.mapCoverage;
} else {
wrappedCode = wrap(content);
}

return {
mapCoverage,
script: new vm.Script(wrappedCode, {
displayErrors: true,
filename: isCoreModule ? 'jest-nodejs-core-' + filename : filename,
Expand Down
1 change: 1 addition & 0 deletions types/Transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type TransformedSource = {|

export type TransformResult = {|
script: Script,
mapCoverage: boolean,
sourceMapPath: ?string,
|};

Expand Down