-
-
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
[WIP] Remove usage of retainLines
#5594
Changes from 1 commit
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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
"jest-matcher-utils": "^22.2.0", | ||
"jest-message-util": "^22.2.0", | ||
"jest-snapshot": "^22.2.0", | ||
"source-map": "^0.6.0", | ||
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. newest is 0.7.1, but they changed their API to be asynchronous, so we can't use it |
||
"source-map-support": "^0.5.0" | ||
}, | ||
"devDependencies": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,13 +15,48 @@ 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 {SourceMapConsumer} from 'source-map'; | ||
import JasmineReporter from './reporter'; | ||
import {install as jasmineAsyncInstall} from './jasmine_async'; | ||
|
||
const JASMINE = require.resolve('./jasmine/jasmine_light.js'); | ||
|
||
// Copied from https://github.com/rexxars/sourcemap-decorate-callsites/blob/5b9735a156964973a75dc62fd2c7f0c1975458e8/lib/index.js#L113-L158 | ||
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. @rexxars thoughts on exporting I wanted to use your module directly, but I don't want the searching for sourcemaps - I have it already. I also need this to be sync 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. Or just expose an API where I can pass ina callsite and a sourcemap, that would be even better (as long as it's sync) 🙂 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 welcome the change, but I'm absolutely swamped with work - be happy if anyone would submit a PR, though. |
||
function extendCallsite(callsite, consumer) { | ||
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, | ||
}, | ||
}); | ||
|
||
return callsite; | ||
} | ||
|
||
async function jasmine2( | ||
globalConfig: GlobalConfig, | ||
config: ProjectConfig, | ||
|
@@ -53,6 +88,17 @@ async function jasmine2( | |
environment.global.it = (...args) => { | ||
const stack = callsites()[1]; | ||
const it = originalIt(...args); | ||
const sourceMapForFile = runtime.getSourceMapForFile(stack.getFileName()); | ||
|
||
if (sourceMapForFile) { | ||
try { | ||
const consumer = new SourceMapConsumer(sourceMapForFile); | ||
|
||
extendCallsite(stack, consumer); | ||
} catch (e) { | ||
// ignored | ||
} | ||
} | ||
|
||
it.result.__callsite = stack; | ||
|
||
|
@@ -125,12 +171,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 |
---|---|---|
|
@@ -449,6 +449,16 @@ class Runtime { | |
}, {}); | ||
} | ||
|
||
getSourceMapForFile(filename: string): ?string { | ||
const sourceMap = this._sourceMapRegistry[filename]; | ||
|
||
if (sourceMap && fs.existsSync(sourceMap)) { | ||
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 |
||
} | ||
|
||
return null; | ||
} | ||
|
||
setMock( | ||
from: string, | ||
moduleName: string, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,7 @@ export default class BufferedConsole extends Console { | |
message: LogMessage, | ||
level: ?number, | ||
) { | ||
// TODO: This callsite needs to read sourcemaps | ||
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. The issue is here: https://github.com/facebook/jest/blob/430aebe49e9f144d29f144f5a29482315e269e4a/packages/jest-runner/src/run_test.js#L86-L108 I need to pass in |
||
const call = callsites()[level != null ? level : 2]; | ||
const origin = call.getFileName() + ':' + call.getLineNumber(); | ||
|
||
|
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.
we should probably remove this line entirely
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.
Feel free to remove the lilne:column mapping in a followup