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

Removing the mapCoverage condition on reading inlineSourceMaps. #5177

Merged
merged 9 commits into from
Feb 15, 2018
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

### Features
* `[docs]` Add MongoDB guide ([#5571](https://github.com/facebook/jest/pull/5571))
* `[jest-runtime]` Deprecate mapCoverage option.
([#5177](https://github.com/facebook/jest/pull/5177))
* `[babel-jest]` Add option to return sourcemap from the transformer separately
from source. ([#5177](https://github.com/facebook/jest/pull/5177))

## jest 22.3.0

Expand Down Expand Up @@ -1595,4 +1599,4 @@ See https://facebook.github.io/jest/blog/2016/12/15/2016-in-jest.html

## <=0.4.0

* See commit history for changes in previous versions of jest.
* See commit history for changes in previous versions of jest.
1 change: 0 additions & 1 deletion TestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
lastCommit: false,
listTests: false,
logHeapUsage: false,
mapCoverage: false,
maxWorkers: 2,
noSCM: null,
noStackTrace: false,
Expand Down
28 changes: 0 additions & 28 deletions docs/Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -313,34 +313,6 @@ Default: `undefined`
This option allows the use of a custom global teardown module which exports an
async function that is triggered once after all test suites.

### `mapCoverage` [boolean]

##### available in Jest **20.0.0+**

Default: `false`

If you have [transformers](#transform-object-string-string) configured that emit
source maps, Jest will use them to try and map code coverage against the
original source code when writing [reports](#coveragereporters-array-string) and
checking [thresholds](#coveragethreshold-object). This is done on a best-effort
basis as some compile-to-JavaScript languages may provide more accurate source
maps than others. This can also be resource-intensive. If Jest is taking a long
Copy link
Member

Choose a reason for hiding this comment

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

The theory is that since babel-jest now returns {map, source}, it's not as intensive since we do not have to extract the inline sourcemap, meaning there is no reason to opt out of this behavior

time to calculate coverage at the end of a test run, try setting this option to
`false`.

Both inline source maps and source maps returned directly from a transformer are
supported. Source map URLs are not supported because Jest may not be able to
locate them. To return source maps from a transformer, the `process` function
can return an object like the following. The `map` property may either be the
source map object, or the source map object as a string.

```js
return {
code: 'the code',
map: 'the source map',
};
```

### `moduleFileExtensions` [array<string>]

Default: `["js", "json", "jsx", "node"]`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`code coverage for transform instrumented code 1`] = `
Object {
"covered.js": Object {
"_coverageSchema": "332fd63041d2c1bcb487cc26dd0d5f7d97098a6c",
"b": Object {},
"branchMap": Object {},
"f": Object {
"0": 1,
},
"fnMap": Object {
"0": Object {
"decl": Object {
"end": Object {
"column": 36,
"line": 9,
},
"start": Object {
"column": 26,
"line": 9,
},
},
"line": 9,
"loc": Object {
"end": Object {
"column": 1,
"line": 12,
},
"start": Object {
"column": 58,
"line": 9,
},
},
"name": "doES6Stuff",
},
},
"path": "covered.js",
"s": Object {
"0": 1,
"1": 1,
"2": 1,
},
"statementMap": Object {
"0": Object {
"end": Object {
"column": 2,
"line": 12,
},
"start": Object {
"column": 0,
"line": 9,
},
},
"1": Object {
"end": Object {
"column": 41,
"line": 10,
},
"start": Object {
"column": 34,
"line": 10,
},
},
"2": Object {
"end": Object {
"column": 33,
"line": 11,
},
"start": Object {
"column": 2,
"line": 11,
},
},
},
},
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ exports[`--showConfig outputs config info and exits 1`] = `
\\"globalSetup\\": null,
\\"globalTeardown\\": null,
\\"listTests\\": false,
\\"mapCoverage\\": false,
\\"maxWorkers\\": \\"[maxWorkers]\\",
\\"noStackTrace\\": false,
\\"nonFlagArgs\\": [],
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/__tests__/coverage_remapping.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ beforeAll(() => {

it('maps code coverage against original source', () => {
run('yarn', dir);
const result = runJest(dir, ['--coverage', '--mapCoverage', '--no-cache']);
const result = runJest(dir, ['--coverage', '--no-cache']);

expect(result.status).toBe(0);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

'use strict';

const {readFileSync} = require('fs');
const path = require('path');
const {cleanup, run} = require('../Utils');
const runJest = require('../runJest');

const dir = path.resolve(__dirname, '../coverage-transform-instrumented');
const coverageDir = path.join(dir, 'coverage');

beforeAll(() => {
cleanup(coverageDir);
});

it('code coverage for transform instrumented code', () => {
Copy link
Contributor

@jwbay jwbay Feb 10, 2018

Choose a reason for hiding this comment

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

I'm not sure if this your intention or not, but on a hunch I pulled this test out and ran it against master and it passed. So if you're trying to test some change in this PR, something should change. I'm still not sure what exactly you're going for here. If you're just trying to prevent future regressions for this use case, I suppose it's fine!

ETA: I just noticed your branch name is fix-coverage -- to be super clear, coverage is not broken in master, whether remapping or from inline source maps or whatever. Removing mapCoverage just unlocks some other possibilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was no test checking for coverage when used with a transform, that's why I added it.

Is there some other scenario that I could write test for too?

When I started, I thought coverage remapping was broken in master, but it turns out that it is not.

run('yarn', dir);
const result = runJest(dir, ['--coverage', '--no-cache']);

expect(result.status).toBe(0);

const coverageMapFile = path.join(coverageDir, 'coverage-final.json');
const coverageMap = JSON.parse(readFileSync(coverageMapFile, 'utf-8'));

// reduce absolute paths embedded in the coverage map to just filenames
Object.keys(coverageMap).forEach(filename => {
coverageMap[filename].path = path.basename(coverageMap[filename].path);
delete coverageMap[filename].hash;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather like coverageMap[filename].hash = "<<REPLACED_HASH>>";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hardcoded string <<REPLACED_HASH>>? If yes, is there any reason to add it? If no, how do I get <<REPLACED_HASH>>?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, hardcoded string. Just that we know that the object has the "hash" key, if that's relevant of course.

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 don't feel it's relevant. Since we are mainly looking for coverage informations like branchMap, statementMap etc

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, leave as is :)

coverageMap[path.basename(filename)] = coverageMap[filename];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here, we could save the key as "<<CWD>>/filename.js" or so, just to be sure we store absolute paths instead of relative ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I put <<CWD>>, won't it add my filesystem path to the snapshot and break when run anywhere else?

delete coverageMap[filename];
});
expect(coverageMap).toMatchSnapshot();
});
23 changes: 23 additions & 0 deletions integration-tests/__tests__/stack_trace_source_maps.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

'use strict';

const path = require('path');
const {run} = require('../Utils');
const runJest = require('../runJest');

it('processes stack traces and code frames with source maps', () => {
const dir = path.resolve(__dirname, '../stack-trace-source-maps');
run('yarn', dir);
const {stderr} = runJest(dir, ['--no-cache']);
expect(stderr).toMatch('> 14 | (() => expect(false).toBe(true))();');
expect(stderr).toMatch(`at __tests__/fails.ts:14:24
at Object.<anonymous> (__tests__/fails.ts:14:35)`);
});
43 changes: 43 additions & 0 deletions integration-tests/coverage-transform-instrumented/Preprocessor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

const jestPreset = require('babel-preset-jest');
const babelTransform = require('babel-core').transform;
const babelIstanbulPlugin = require('babel-plugin-istanbul').default;

const options = {
presets: ['env', jestPreset],
retainLines: true,
sourceMaps: 'inline',
};

module.exports = {
canInstrument: true,
process(src, filename, config, transformOptions) {
options.filename = filename;
if (transformOptions && transformOptions.instrument) {
options.auxiliaryCommentBefore = ' istanbul ignore next ';
options.plugins = [
[
babelIstanbulPlugin,
{
cwd: config.rootDir,
exclude: [],
},
],
];
}

const transformResult = babelTransform(src, options);

if (!transformResult) {
return src;
}

return transformResult.code;
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

const doES6Stuff = require('../covered.js');

it('works correctly', () => {
const someObj = {someNumber: 10, this: 'is irrelevant'};
expect(doES6Stuff(someObj, 2)).toBe(20);
});
12 changes: 12 additions & 0 deletions integration-tests/coverage-transform-instrumented/covered.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/* eslint-disable no-unused-vars */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing full copyright header

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!

/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

module.exports = function doES6Stuff(testObj, multiplier) {
const {someNumber, ...others} = testObj;
return someNumber * multiplier;
};
20 changes: 20 additions & 0 deletions integration-tests/coverage-transform-instrumented/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"jest": {
"rootDir": "./",
"transform": {
"^.+\\.(js)$": "<rootDir>/Preprocessor.js"
},
"testRegex": "/__tests__/.*\\.(js)$",
"testEnvironment": "node",
"moduleFileExtensions": ["js"]
},
"babel": {
"presets": ["env"]
},
"dependencies": {
"babel-core": "6.26.0",
"babel-plugin-istanbul": "4.1.5",
"babel-preset-env": "1.6.1",
"babel-preset-jest": "22.2.0"
}
}
Loading