nyc
nyc copied to clipboard
istanbuljs
chore(deps): bump foreground-child from ^2.0.0 to ^3.0.0
@nwalters512 do you need to run npm i Locally to update the lock file?
npm ERR! cipm can only install packages with an existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or later to generate it, then try again.
@adevick There's no package-lock.json checked into this repo. The last commit to this repo was https://github.com/istanbuljs/nyc/commit/ab7c53b2f340b458789a746dff2abd3e2e4790c3, which removed package-lock.json, and from what I can see, no CI run has completed successfully since then. It seems like CI wasn't tested at all before that commit landed on master.
@bcoe can you say more about why this was closed? Is there anything I can do to help land this change?
I've pinged @bcoe on X .. we're also experiencing #1535 (or likely a variant thereof).
@kraenhansen thanks for the contribution. It looks like a few tests are failing, do you see this locally?
@bcoe this is my contribution; I'm looking into the failure right now!
My bad sorry, I was half paying attention, and it was @kraenhansen who reached out to me.
I was able to fix one test failure cause, which was that foreground-child will overwrite process.exitCode; the correct way to specify a new exit code is to return one from the cleanup function. This was done in
The second failure relates to --show-process-tree, which now includes the "watchdog" process that foreground-child added in v3. I don't see an easy way to fix this; it will always be incompatible with snapshots because the process is invoked with a non-deterministic PID in its arguments. Any suggestions?
@nwalters512 two options:
- I'd be fine with skipping those tests.
- Alternatively, what I've done in the past is remove values from the string that's about to be snapshot with a string replace that vary.
(1) would seem to be the easiest, as https://github.com/istanbuljs/istanbul-lib-processinfo doesn't consistently order items in the tree, and the order actually changes the output of the lines we do want because of how the tree is rendered.
It's worth noting that this change will be visible to users, in the form of an extra line in the process tree like this:
├── node 88099
│ Unknown % Lines
We could conceivably catch that and filter it out by looking for something like this:
execArgv[0] === '-e' && execArgv[1].indexOf('foreground-child watchdog pid') !== -1
I'm not sure if that's worth the effort, or how brittle that would end up being in the long run if/when foreground-process changes. Might be worth checking with @/isaacs, who oddly enough both maintains foreground-process and requested the process tree functionality in the first place: https://github.com/istanbuljs/nyc/issues/158
@nwalters512 the process tree is pretty niche, I'm comfortable with commenting out these tests as long as the majority of tests work that exercise core behaviour.
@bcoe I haven't forgotten about this! I'm AFK on my honeymoon, I'll be able to get back to this on the week of May 6. If there's an urgent need to land this in the meantime, don't hesitate to make changes on my behalf.
@bcoe I was able to fix many of the broken tests today. There are now only four remaining: every test in test/processinfo.js. I'll leave a comment on a line in a second highlighting what I've been able to figure out so far. I'm at the limit of my understanding of what nyc is doing (all this processinfo stuff is a bit confusing); I'd appreciate your help in taking this over the finish line, even if it's just pointing me in the right direction.
@nwalters512 if you're pretty confident that it's the tests that are fragile and not the update to foreground-child, I'm comfortable with us skipping appropriate tests.
A couple nits:
- Rather than commenting out, you should be able to use
tap.skip. - I'd make the comments:
TODO: get this test working again with foreground-child 3.x.x or delete #[issue #].
There are still a healthy number of tests failing, is this expected based on "I have no idea what blorp is, what NYC_PROCESSINFO_EXTERNAL_ID is supposed to do", and we just need to comment out a couple more tests.
nyc hasn't bee released in 4 years, and the next release is going to be a major. I'm excited for us to be on newer versions of deps, even if there's potentially some bugs that need to be ironed out after.
@bcoe I updated the tests per your recommendation to use \.skip. The tests themselves now pass locally for me, but the test process on the whole errors out because the skipped tests left some lines uncovered and thus dropped the coverage below the required thresholds:
https://github.com/istanbuljs/nyc/blob/10daaccaf37ef9556ff15430cb08c6e8f6991618/nyc.config.js#L18-L21
I could try to add some tests to exercise and assert something about the code, or I could slap some /* istanbul ignore next */ on the relevant lines with links back to this PR; let me know what you'd prefer. Thanks for your help/advice!
I have a fix for getting coverage back to 100% (and not skipping any tests), but I'd need this first: https://github.com/tapjs/foreground-child/pull/59
fix is pretty much this:
// nyc.js:91
foregroundChild(childArgs, async (code, signal, processInfo) => {
let exitCode = process.exitCode || code
try {
// clean up foreground-child watchdog process info
const parentDir = path.resolve(nyc.tempDirectory())
const dir = path.resolve(nyc.tempDirectory(), 'processinfo')
const files = await nyc.coverageFiles(dir)
for (let i = 0; i < files.length; i++) {
const data = await nyc.coverageFileLoad(files[i], dir)
if (data.pid === processInfo.watchdog.pid) {
fs.unlinkSync(path.resolve(parentDir, files[i]))
fs.unlinkSync(path.resolve(dir, files[i]))
}
}
await nyc.writeProcessIndex()
@isaacs is a saint and reviewd/merged the PR, so I PR'd this PR with the changes: https://github.com/nwalters512/nyc/pull/1