-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[WIP] Remove usage of retainLines
#5594
Changes from 8 commits
2b8e256
0c1dd5c
8f9e54f
ab81892
1189fe7
8e50d0f
6ad7202
6ec7b74
11948b8
765f78a
f0c6e08
d4b0600
7ce154d
3f2ccb8
e73423d
0a29175
e7ccbc6
2e0e0a3
736eb2b
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 |
---|---|---|
|
@@ -118,7 +118,7 @@ exports[`works with async failures 1`] = ` | |
+ \\"foo\\": \\"bar\\", | ||
} | ||
|
||
at ../../packages/expect/build/index.js:145:57 | ||
at ../../packages/expect/build/index.js:104:76 | ||
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. we should probably remove this line entirely 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. Feel free to remove the lilne:column mapping in a followup |
||
|
||
" | ||
`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,6 @@ it('adds correct location info when provided with flag', () => { | |
const assertions = result.testResults[0].assertionResults; | ||
expect(result.success).toBe(true); | ||
expect(result.numTotalTests).toBe(2); | ||
expect(assertions[0].location).toEqual({column: 1, line: 9}); | ||
expect(assertions[1].location).toEqual({column: 3, line: 14}); | ||
expect(assertions[0].location).toEqual({column: 1, line: 10}); | ||
expect(assertions[1].location).toEqual({column: 2, line: 15}); | ||
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. @thymikee your comment disappeared, but here's my response: I'm a bit unsure about the column change but line wise it was correct earlier as well up until I messed up the
{"column": 1, "line": 12}
{"column": 3, "line": 17} 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 a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it's the Unless it's zero-indexed, in which case the first one should be 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. Ah pardon me. Then te second column should be 3 instead of 2. 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, it should (I just tested with normal js - it's not zero-indexed) 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. This is a bug in either See this example: 'use strict';
const fs = require('fs');
const {transform} = require('babel-core');
const {SourceMapConsumer} = require('source-map');
const fileContent = fs.readFileSync(
'./integration-tests/location-in-results/__tests__/test.js',
'utf8',
);
const res = transform(fileContent, {
plugins: ['transform-strict-mode'],
sourceMaps: 'both',
});
const consumer = new SourceMapConsumer(JSON.stringify(res.map));
console.log(
// 17:3 in `res.code` has the `it('also works', () => {`
consumer.originalPositionFor({
column: 3,
line: 17,
}),
); Logs 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. Created https://github.com/SimenB/sourcemap-issues with the reproduction. @jwbay any ideas here? Should we go to babel or source-map and report it? Or am I doing something really weird? |
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,7 @@ import type {TestResult} from 'types/TestResult'; | |
import type Runtime from 'jest-runtime'; | ||
|
||
import path from 'path'; | ||
import fs from 'fs'; | ||
import callsites from 'callsites'; | ||
import {getCallsite} from 'jest-util'; | ||
import JasmineReporter from './reporter'; | ||
import {install as jasmineAsyncInstall} from './jasmine_async'; | ||
|
||
|
@@ -51,7 +50,7 @@ async function jasmine2( | |
if (config.testLocationInResults === true) { | ||
const originalIt = environment.global.it; | ||
environment.global.it = (...args) => { | ||
const stack = callsites()[1]; | ||
const stack = getCallsite(1, runtime.getSourceMaps()); | ||
const it = originalIt(...args); | ||
|
||
it.result.__callsite = stack; | ||
|
@@ -125,12 +124,11 @@ async function jasmine2( | |
environment: 'node', | ||
handleUncaughtExceptions: false, | ||
retrieveSourceMap: source => { | ||
if (runtime._sourceMapRegistry[source]) { | ||
const sourceMap = runtime.getSourceMapForFile(source); | ||
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. should probably use |
||
if (sourceMap) { | ||
try { | ||
return { | ||
map: JSON.parse( | ||
fs.readFileSync(runtime._sourceMapRegistry[source]), | ||
), | ||
map: JSON.parse(sourceMap), | ||
url: source, | ||
}; | ||
} catch (e) {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,13 +74,21 @@ async function runTestInternal( | |
RuntimeClass, | ||
>); | ||
|
||
let runtime = undefined; | ||
|
||
const consoleOut = globalConfig.useStderr ? process.stderr : process.stdout; | ||
const consoleFormatter = (type, message) => | ||
getConsoleOutput( | ||
config.cwd, | ||
!!globalConfig.verbose, | ||
// 4 = the console call is buried 4 stack frames deep | ||
BufferedConsole.write([], type, message, 4), | ||
BufferedConsole.write( | ||
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. oh, I didn't know this was the way it's hooked up. practical! 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. We should probably pass it to the |
||
[], | ||
type, | ||
message, | ||
4, | ||
runtime && runtime.getSourceMaps(), | ||
), | ||
); | ||
|
||
let testConsole; | ||
|
@@ -101,7 +109,7 @@ async function runTestInternal( | |
const cacheFS = {[path]: testSource}; | ||
setGlobal(environment.global, 'console', testConsole); | ||
|
||
const runtime = new Runtime(config, environment, resolver, cacheFS, { | ||
runtime = new Runtime(config, environment, resolver, cacheFS, { | ||
collectCoverage: globalConfig.collectCoverage, | ||
collectCoverageFrom: globalConfig.collectCoverageFrom, | ||
collectCoverageOnlyFrom: globalConfig.collectCoverageOnlyFrom, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import type {Context} from 'types/Context'; | |
import type {Jest, LocalModuleRequire} from 'types/Jest'; | ||
import type {ModuleMap} from 'jest-haste-map'; | ||
import type {MockFunctionMetadata, ModuleMocker} from 'types/Mock'; | ||
import type {SourceMapRegistry} from 'types/SourceMaps'; | ||
|
||
import path from 'path'; | ||
import HasteMap from 'jest-haste-map'; | ||
|
@@ -100,7 +101,7 @@ class Runtime { | |
_shouldAutoMock: boolean; | ||
_shouldMockModuleCache: BooleanObject; | ||
_shouldUnmockTransitiveDependenciesCache: BooleanObject; | ||
_sourceMapRegistry: {[key: string]: string, __proto__: null}; | ||
_sourceMapRegistry: SourceMapRegistry; | ||
_scriptTransformer: ScriptTransformer; | ||
_transitiveShouldMock: BooleanObject; | ||
_unmockList: ?RegExp; | ||
|
@@ -449,6 +450,21 @@ class Runtime { | |
}, {}); | ||
} | ||
|
||
getSourceMapForFile(filename: string): ?string { | ||
const sourceMap = this._sourceMapRegistry[filename]; | ||
|
||
try { | ||
return fs.readFileSync(sourceMap, 'utf8'); | ||
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 wonder if we should cache these instead of hitting the disk every time 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. can |
||
} catch (e) { | ||
// ignore | ||
} | ||
return null; | ||
} | ||
|
||
getSourceMaps(): SourceMapRegistry { | ||
return this._sourceMapRegistry; | ||
} | ||
|
||
setMock( | ||
from: string, | ||
moduleName: string, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import type {SourceMapRegistry} from 'types/SourceMaps'; | ||
|
||
import fs from 'fs'; | ||
import callsites from 'callsites'; | ||
import {SourceMapConsumer} from 'source-map'; | ||
|
||
// Copied from https://github.com/rexxars/sourcemap-decorate-callsites/blob/5b9735a156964973a75dc62fd2c7f0c1975458e8/lib/index.js#L113-L158 | ||
const addSourceMapConsumer = (callsite, consumer) => { | ||
try { | ||
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. this |
||
const getLineNumber = callsite.getLineNumber; | ||
const getColumnNumber = callsite.getColumnNumber; | ||
let position = null; | ||
|
||
function getPosition() { | ||
if (!position) { | ||
position = consumer.originalPositionFor({ | ||
column: getColumnNumber.call(callsite), | ||
line: getLineNumber.call(callsite), | ||
}); | ||
} | ||
|
||
return position; | ||
} | ||
|
||
Object.defineProperties(callsite, { | ||
getColumnNumber: { | ||
value() { | ||
return getPosition().column || getColumnNumber.call(callsite); | ||
}, | ||
writable: false, | ||
}, | ||
getLineNumber: { | ||
value() { | ||
return getPosition().line || getLineNumber.call(callsite); | ||
}, | ||
writable: false, | ||
}, | ||
}); | ||
} catch (e) { | ||
// ignored | ||
} | ||
}; | ||
|
||
export default (level: number, sourceMaps: SourceMapRegistry) => { | ||
const levelAfterThisCall = level + 1; | ||
const stack = callsites()[levelAfterThisCall]; | ||
const sourceMapFileName = sourceMaps && sourceMaps[stack.getFileName()]; | ||
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. Either add a |
||
|
||
try { | ||
if (sourceMapFileName) { | ||
const sourceMap = fs.readFileSync(sourceMapFileName, 'utf8'); | ||
if (sourceMap) { | ||
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. is this needed? |
||
addSourceMapConsumer(stack, new SourceMapConsumer(sourceMap)); | ||
} | ||
} | ||
|
||
return stack; | ||
} catch (e) { | ||
return null; | ||
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. it should |
||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/** | ||
* 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 | ||
*/ | ||
|
||
export type SourceMapRegistry = {[key: string]: string, __proto__: null}; |
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.
"...from babel", or just "Remove usage of
retainLines
"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.
My thinking was that people would go "what's
retainLines
, I've never passed that to jest before"