davidmarkclements / 0x

🔥 single-command flamegraph profiling 🔥
MIT License
3.23k stars 106 forks source link

tick-to-tree fails with an empty buffer when `tap` measures coverage on Node 8 #188

Closed AlanSl closed 2 years ago

AlanSl commented 6 years ago

To replicate, on Node 8 (tested on 8.11 and 8.12):

cd 0x

TAP_SNAPSHOT=1 tap test/ticks-to-tree.test.js
# test passes

TAP_SNAPSHOT=1 tap --lines 1 test/ticks-to-tree.test.js
# test fails due to stackChild.stdout being an empty buffer, failing the assert in ticks-to-tree.js 

The same thing happens if a dependent of 0x runs tests that call 0x's ticks-to-tree in a tap test called using any coverage measure.

Obviously this doesn't affect users' usage of 0x. It's just is a problem for 3rd party applications that use 0x and use tap with coverage checks in their CI.

mcollina commented 6 years ago

That's what happening:

Error [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension: /Users/matteo/.node-spawn-wrap-98539-8640f35b202e/node
    at Loader.exports.resolve [as resolver] (internal/loader/ModuleRequest.js:135:13)
    at Loader.resolve (internal/loader/Loader.js:82:40)
    at Loader.getModuleJob (internal/loader/Loader.js:112:40)
    at Loader.import (internal/loader/Loader.js:139:28)
    at module.js:466:29
    at Function.Module._load (module.js:467:7)
    at Function.Module.runMain (module.js:694:10)
    at startup (bootstrap_node.js:204:16)
    at bootstrap_node.js:625:3
mcollina commented 6 years ago

Unfortunately this looks like hard to fix, as it's a bug in spawn-wrap and it uses private core process.bindings that have likely changed between Node.js versions.

AlanSl commented 6 years ago

Maybe it's best then if 3rd parties using 0x simply do their test coverage checks in Node 10 or Node 6, and not Node 8.

Not sure if it's worth including a warning like this (maybe it should be general to spawn wrap rather than specific to tap):

if (process.env._TAP_COVERAGE_ && process.version.match(/^v8\./)) {
  console.warn('Tap coverage checks in Node 8 cause 0x to fail. Please use Node 6 or Node 10, and not Node 8, when calculating test coverage')
}
mcollina commented 6 years ago

IMHO spawn-wrap needs a fix for this.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] commented 2 years ago

This issue was closed because it has been stalled for 5 days with no activity.