-
-
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 all 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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -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; | ||
|
@@ -90,7 +98,7 @@ async function runTestInternal( | |
} else if (globalConfig.verbose) { | ||
testConsole = new Console(consoleOut, process.stderr, consoleFormatter); | ||
} else { | ||
testConsole = new BufferedConsole(); | ||
testConsole = new BufferedConsole(() => runtime && runtime.getSourceMaps()); | ||
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. went with a getter 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. looks great fwiw i don't think there's an integration test for this path |
||
} | ||
|
||
const environment = new TestEnvironment(config, {console: 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 |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import fs from 'fs'; | ||
import SourceMap from 'source-map'; | ||
import getCallsite from '../get_callsite'; | ||
|
||
jest.mock('fs'); | ||
|
||
describe('getCallsite', () => { | ||
test('without source map', () => { | ||
const site = getCallsite(0); | ||
|
||
expect(site.getFileName()).toEqual(__filename); | ||
expect(site.getColumnNumber()).toEqual(expect.any(Number)); | ||
expect(site.getLineNumber()).toEqual(expect.any(Number)); | ||
expect(fs.readFileSync).not.toHaveBeenCalled(); | ||
}); | ||
|
||
test('ignores errors when fs throws', () => { | ||
fs.readFileSync.mockImplementation(() => { | ||
throw new Error('Mock error'); | ||
}); | ||
|
||
const site = getCallsite(0, {[__filename]: 'mockedSourceMapFile'}); | ||
|
||
expect(site.getFileName()).toEqual(__filename); | ||
expect(site.getColumnNumber()).toEqual(expect.any(Number)); | ||
expect(site.getLineNumber()).toEqual(expect.any(Number)); | ||
expect(fs.readFileSync).toHaveBeenCalledWith('mockedSourceMapFile', 'utf8'); | ||
}); | ||
|
||
test('reads source map file to determine line and column', () => { | ||
fs.readFileSync.mockImplementation(() => 'file data'); | ||
|
||
const sourceMapColumn = 1; | ||
const sourceMapLine = 2; | ||
SourceMap.SourceMapConsumer = class { | ||
originalPositionFor(params) { | ||
expect(params).toMatchObject({ | ||
column: expect.any(Number), | ||
line: expect.any(Number), | ||
}); | ||
|
||
return { | ||
column: sourceMapColumn, | ||
line: sourceMapLine, | ||
}; | ||
} | ||
}; | ||
|
||
const site = getCallsite(0, {[__filename]: 'mockedSourceMapFile'}); | ||
|
||
expect(site.getFileName()).toEqual(__filename); | ||
expect(site.getColumnNumber()).toEqual(sourceMapColumn); | ||
expect(site.getLineNumber()).toEqual(sourceMapLine); | ||
expect(fs.readFileSync).toHaveBeenCalledWith('mockedSourceMapFile', 'utf8'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/** | ||
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. @rickhanlonii would you mind adding a couple of unit tests to this? 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. Yeah, will do 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. | ||
* | ||
* @flow | ||
*/ | ||
|
||
import type {SourceMapRegistry} from 'types/SourceMaps'; | ||
|
||
import fs from 'graceful-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) => { | ||
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, | ||
}, | ||
}); | ||
}; | ||
|
||
export default (level: number, sourceMaps: ?SourceMapRegistry) => { | ||
const levelAfterThisCall = level + 1; | ||
const stack = callsites()[levelAfterThisCall]; | ||
const sourceMapFileName = sourceMaps && sourceMaps[stack.getFileName()]; | ||
|
||
if (sourceMapFileName) { | ||
try { | ||
const sourceMap = fs.readFileSync(sourceMapFileName, 'utf8'); | ||
addSourceMapConsumer(stack, new SourceMapConsumer(sourceMap)); | ||
} catch (e) { | ||
// ignore | ||
} | ||
} | ||
|
||
return stack; | ||
}; |
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