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

Fix coverage with sourcemaps inlined via Transform API. #895

Merged
merged 2 commits into from Nov 21, 2018

Conversation

@Invader444
Copy link
Contributor

Invader444 commented Nov 18, 2018

This also fixes issue #886 "--coverage-all fails to find any files on Windows", which still has issues after #888.

I have also fixed a few tests that relied on path strings to work with Windows paths.

@Invader444 Invader444 force-pushed the secretcowlevel:master branch 3 times, most recently from 29f35d7 to 86b76d6 Nov 18, 2018
Fix issue #886 "--coverage-all fails to find any files on Windows".
Fix tests that rely on path strings on Windows.
@Invader444 Invader444 force-pushed the secretcowlevel:master branch from 86b76d6 to 9db5dee Nov 18, 2018
@geek geek self-assigned this Nov 18, 2018
@geek geek added the bug label Nov 18, 2018
@geek geek added this to the 18.0.1 milestone Nov 18, 2018
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented on lib/coverage.js in 9db5dee Nov 18, 2018

Without the above change, this test results in zero files being covered. But if we only change this line, the fileCache breaks due to mixed path separators!

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented on lib/coverage.js in 9db5dee Nov 18, 2018

The above change is inspired from code in the SourecMapSupport package. If we used their API to hook require it would be cleaner and more upgrade tolerant, but I felt it may introduce bugs as well... so I opted to adding a little more code to maintain instead.

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented on 9db5dee Nov 18, 2018

Even with these changes, two tests still fail on Windows! CLI multiple reporter tests continue to fail when Hoek.clone is called on the options object, which tries to assign to 'bytesWritten' on a Pipe object. I believe however that that is a bug in Hoek, and so haven't addressed it here.

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 18, 2018

It would be awesome if you added builds for Windows to Travis :D

sourceIndex--;
}

if (sourceIndex >= 0) {

This comment has been minimized.

Copy link
@geek

geek Nov 19, 2018

Member

Can you add a comment to explain more of the purpose of line 550-570

This comment has been minimized.

Copy link
@Invader444

Invader444 Nov 19, 2018

Author Contributor

The line I removed, const transformedFile = content.replace(/\/\/\#(.*)\r?\n?$/, '');, used to remove inlined sourcemaps from the data.source object. SourceMapSupport.retrieveSourceMap(ret.filename) will only find inlined sourcemaps if they are present on disk. In combination with the Transform API, inlined sourcemaps are never found.
This code will look at the data.source object to find the inlined sourcemap from the specified Transform if the source disk file has no sourceMapURL.

This comment has been minimized.

Copy link
@Invader444

Invader444 Nov 19, 2018

Author Contributor

The test I added covers this case by:

  1. Ensuring the source file has no sourceMapURL on disk
  2. Injecting a sourceMapURL with inline source map data via the Transform API
  3. Verifying that the original line numbers are present and accurate in the coverage analysis

This comment has been minimized.

Copy link
@Invader444

Invader444 Nov 19, 2018

Author Contributor

I suppose the question becomes, should sourceMapURL's passed within the content to coverage be preferred over sourceMapURL's found in the file's disk contents? With this patch, a sourceMapURL present on disk would trump the one in the content.

I chose to do this mainly because it is closer to current behavior that people may be expecting; if someone is currently using lab Transform API and generating inline sourcemaps, but haven't complained, they may also somehow have sourceMapURLs in their disk content that they prefer to use? (Sounds unlikely... but still...)

This comment has been minimized.

Copy link
@geek

geek Nov 21, 2018

Member

Thank you for the explanation. I'll merge and verify that I don't encounter any breakage. I'm considering this a bug fix unless you think otherwise. Thanks again!

@geek

This comment has been minimized.

Copy link
Member

geek commented Nov 19, 2018

@Invader444 thanks for the PR. I'd support a windows build from travis as well once we can narrow down if the failure is a hoek bug or not.

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 19, 2018

For what it's worth, here is the output of npm run test with my patch in place:

C:\Users\Jon\workspace\git\Doomtrooper\lab>npm run test
npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/
npm WARN npm npm does not support Node.js v10.0.0
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm Supported releases of Node.js are the latest release of 4, 6, 7, 8, 9.
npm WARN npm You can find the latest version at https://nodejs.org/

> lab@18.0.0 test C:\Users\Jon\workspace\git\Doomtrooper\lab
> node ./bin/_lab -fL -t 100 -m 3000



  ..........x.......................................
  .........................x........................
  ..................................................
  ..................

  .

1 tests complete
Test duration: 1 ms
No global variable leaks detected

................................
  ..................................................
  ..................................................
  ................................................

Test script errors:

ENOENT: no such file or directory, unlink 'C:\Users\Jon\AppData\Local\Temp\1542597363994-12768-03aa23a2e1178061'
  Error: ENOENT: no such file or directory, unlink 'C:\Users\Jon\AppData\Local\Temp\1542597363994-12768-03aa23a2e1178061'

There were 1 test script error(s).

Failed tests:

  11) CLI runs tests with multiple reporters:

      actual expected

      "TypeError: Cannot assign to read only property 'bytesWritten' of object '#<Pipe>'\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:29)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at Object.exports.clone(C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at options.reporter.forEach (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:20:38)\n    at Array.forEach (<anonymous>)\n    at new module.exports.internals.Reporter (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:18:22)\n    at Object.exports.generate (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\index.js:73:22)\n    at Object.exports.report (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\runner.js:67:32)\n    at Object.exports.run (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\cli.js:65:19)\n    at C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:13:66\n    at Object.<anonymous> (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:20:7)\n    at Module._compile (internal/modules/cjs/loader.js:678:30)\n    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)\n    at Module.load (internal/modules/cjs/loader.js:589:32)\n    at tryModuleLoad (internal/modules/cjs/loader.js:528:12)\n    at Function.Module._load (internal/modules/cjs/loader.js:520:3)\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\n    at startup (internal/bootstrap/node.js:228:19)\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\n"""

      Expected 'TypeError: Cannot assign to read only property \'bytesWritten\' of object \'#<Pipe>\'\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:29)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at options.reporter.forEach (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:20:38)\n    at Array.forEach (<anonymous>)\n    at new module.exports.internals.Reporter (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:18:22)\n    at Object.exports.generate (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\index.js:73:22)\n    at Object.exports.report (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\runner.js:67:32)\n    at Object.exports.run (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\cli.js:65:19)\n    at C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:13:66\n    at Object.<anonymous> (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:20:7)\n    at Module._compile (internal/modules/cjs/loader.js:678:30)\n    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)\n    at Module.load (internal/modules/cjs/loader.js:589:32)\n    at tryModuleLoad (internal/modules/cjs/loader.js:528:12)\n    at Function.Module._load (internal/modules/cjs/loader.js:520:3)\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\n    at startup (internal/bootstrap/node.js:228:19)\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\n' to equal specified value: ''

      at C:\Users\Jon\workspace\git\Doomtrooper\lab\test\cli.js:129:39

  76) CLI when using multiple reporters does not display error message when there is more than one "only" accross multiple files:

      Expected 'TypeError: Cannot assign to read only property \'bytesWritten\' of object \'#<Pipe>\'\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:29)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at Object.exports.clone (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\node_modules\\hoek\\lib\\index.js:79:39)\n    at options.reporter.forEach (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:20:38)\n    atArray.forEach (<anonymous>)\n    at new module.exports.internals.Reporter (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\multiple.js:18:22)\n    at Object.exports.generate (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\reporters\\index.js:73:22)\n    at Object.exports.report (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\runner.js:67:32)\n    at Object.exports.run (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\test_runner\\cli.js:65:19)\n    at C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:13:66\n    at Object.<anonymous> (C:\\Users\\Jon\\workspace\\git\\Doomtrooper\\lab\\bin\\_lab:20:7)\n    at Module._compile (internal/modules/cjs/loader.js:678:30)\n    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)\n    at Module.load (internal/modules/cjs/loader.js:589:32)\n    at tryModuleLoad(internal/modules/cjs/loader.js:528:12)\n    at Function.Module._load (internal/modules/cjs/loader.js:520:3)\n    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)\n    at startup (internal/bootstrap/node.js:228:19)\n    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)\n' to include '14 skipped'

      at C:\Users\Jon\workspace\git\Doomtrooper\lab\test\cli.js:610:46


2 of 348 tests failed
Test duration: 45169 ms
No global variable leaks detected
Coverage: 100.00%
Linting results: No issues

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! lab@18.0.0 test: `node ./bin/_lab -fL -t 100 -m 3000`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the lab@18.0.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Jon\AppData\Roaming\npm-cache\_logs\2018-11-19T03_16_18_499Z-debug.log
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 19, 2018

hapijs/hoek#228 points to Pipe objects needing a special case, maybe...

@Invader444 Invader444 force-pushed the secretcowlevel:master branch from b5c4194 to 52c4658 Nov 20, 2018
@Invader444 Invader444 force-pushed the secretcowlevel:master branch from 52c4658 to c877a18 Nov 20, 2018
@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 20, 2018

With the better implementation of deep clone in lodash, my Windows tests also fully pass on this branch.

@geek
geek approved these changes Nov 21, 2018
@geek geek merged commit da9c89d into hapijs:master Nov 21, 2018
2 checks passed
2 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Nov 24, 2018

There is indeed a Hoek.clone() bug. I'm looking into it: hapijs/hoek#278

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Nov 24, 2018

The Hoek issue should be fixed.

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 24, 2018

Ill switch back and check my tests later today :)

@Invader444

This comment has been minimized.

Copy link
Contributor Author

Invader444 commented Nov 24, 2018

I can confirm the Hoek.clone fixes in hapijs/hoek#278 also fix the lab tests on Windows. See #898

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.