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

chore(deps): bump foreground-child from ^2.0.0 to ^3.0.0 #1546

Merged
merged 10 commits into from
Sep 13, 2024

Conversation

nwalters512
Copy link
Contributor

This should resolve #1535.

@adevick
Copy link

adevick commented Mar 12, 2024

@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.

@nwalters512
Copy link
Contributor Author

@adevick There's no package-lock.json checked into this repo. The last commit to this repo was ab7c53b, 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 bcoe closed this Apr 10, 2024
@nwalters512
Copy link
Contributor Author

@bcoe can you say more about why this was closed? Is there anything I can do to help land this change?

@kraenhansen
Copy link

I've pinged @bcoe on X .. we're also experiencing #1535 (or likely a variant thereof).

@bcoe
Copy link
Member

bcoe commented Apr 19, 2024

@kraenhansen thanks for the contribution. It looks like a few tests are failing, do you see this locally?

@nwalters512
Copy link
Contributor Author

@bcoe this is my contribution; I'm looking into the failure right now!

@bcoe
Copy link
Member

bcoe commented Apr 19, 2024

@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.

@nwalters512
Copy link
Contributor Author

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?

@bcoe
Copy link
Member

bcoe commented Apr 19, 2024

@nwalters512 two options:

  1. I'd be fine with skipping those tests.
  2. 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.

@nwalters512
Copy link
Contributor Author

(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: #158

@bcoe
Copy link
Member

bcoe commented Apr 20, 2024

@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
Copy link
Member

bcoe commented Apr 30, 2024

@nwalters512 friendly nudge on this one 😄

@nwalters512
Copy link
Contributor Author

@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
Copy link
Member

bcoe commented Apr 30, 2024

@nwalters512

I'm AFK on my honeymoon

Congrats!

@nwalters512
Copy link
Contributor Author

@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.

Comment on lines 196 to 199
// Disabled; see https://github.com/istanbuljs/nyc/pull/1546
// t.test('--show-process-tree displays a tree of spawned processes', t => testSuccess(t, {
// args: ['--show-process-tree', process.execPath, 'selfspawn-fibonacci.js', '5']
// }))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As desired, this test is now commented out. I left a link back to this PR for any future spelunkers.

Comment on lines 35 to 36
// The subprocess is failing with the following error:
// " External ID blorp used by multiple processes"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to determine this by changing the preceding lines to the following:

-  proc.stderr.resume()
-  proc.stdout.resume()
+  proc.stderr.pipe(process.stderr)
+  proc.stdout.pipe(process.stdout)

This let me see the actual error message from the subprocess. This error actually comes from istanbul-lib-processinfo: https://github.com/istanbuljs/istanbul-lib-processinfo/blob/b9cd52cf6eb140cf3bff24ba5df34d5812845d66/index.js#L250

I have no idea what blorp is, what NYC_PROCESSINFO_EXTERNAL_ID is supposed to do, or why it would end up being used by multiple processes (though this probably has something to do with the watchdog process that foreground-child creates).

test/temp-dir.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes here were necessary to account for the new watchdog process from foreground-child.

@bcoe
Copy link
Member

bcoe commented May 16, 2024

@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.

@nwalters512
Copy link
Contributor Author

@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:

nyc/nyc.config.js

Lines 18 to 21 in 10daacc

branches: 100,
functions: 100,
lines: 100,
statements: 100

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!

@frattaro
Copy link

I have a fix for getting coverage back to 100% (and not skipping any tests), but I'd need this first: tapjs/foreground-child#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()

@frattaro
Copy link

frattaro commented Aug 7, 2024

@isaacs is a saint and reviewd/merged the PR, so I PR'd this PR with the changes: nwalters512#1

@MINDoSOFT
Copy link

Looking forward for this MR being merged !

test/temp-dir.js Outdated Show resolved Hide resolved
@frattaro
Copy link

@bcoe any chance you could follow up on this? 😸

@bcoe bcoe merged commit af74d1e into istanbuljs:main Sep 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants