-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Changes from all commits
00cb000
f927798
fdefcaa
c1f6712
970c94f
5f17ea3
936c4cc
62f3b31
520a141
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hardcoded string There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, leave as is :) |
||
coverageMap[path.basename(filename)] = coverageMap[filename]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here, we could save the key as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I put |
||
delete coverageMap[filename]; | ||
}); | ||
expect(coverageMap).toMatchSnapshot(); | ||
}); |
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)`); | ||
}); |
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); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* eslint-disable no-unused-vars */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing full copyright header There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
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" | ||
} | ||
} |
There was a problem hiding this comment.
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