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: add support for subprocess spawning #11

Merged
merged 7 commits into from
Jan 1, 2018
Merged

chore: add support for subprocess spawning #11

merged 7 commits into from
Jan 1, 2018

Conversation

bcoe
Copy link
Owner

@bcoe bcoe commented Dec 18, 2017

using the spawn-wrap library, this pull request adds support for tracking coverage in applications that spawn subprocesses (this is important in unit testing since many test frameworks immediately spawn a subprocess for executing tests).

I was shocked when I pulled this feature together by the fact that everything just worked

This raises a couple mysteries:

Mystery 1

I'm able to output coverage information in the process.on('exit') handler:

    // hook process.exit() and common exit signals, e.g., SIGTERM,
    // and output coverage report when these occur.
    onExit(() => {
      session.post('Profiler.takePreciseCoverage', (err, res) => {
        if (err) console.warn(err.message)
        else {
          try {
            const result = filterResult(res.result)
            writeIstanbulFormatCoverage(result)
          } catch (err) {
            console.warn(err.message)
          }
        }
      })
    }, {alwaysLast: true})

I was pleasantly surprised to find out that I'm able to call Profiler.takePreciseCoverage from within an exit handler; this is a bit shocking to me because you're generally not able to perform async operations within this context (was a workaround for this introduced, in relation to the inspector?).

Mystery 2

block-level test coverage seems to be working on the canary build of Node.js:

screen shot 2017-12-17 at 4 24 45 pm

☝️ I'm ecstatic that this is working; but shouldn't it not be working since I'm not spawning with --inspect-brk? I was under the impression once the Node process starts we're not able to enable block level coverage.


@hashseed, @TimothyGu @schuay @eugeneo, perhaps you can provide some insight into these mysteries?

If things are working as expected, it would seem that we're at the point where we can use v8's built-in test coverage in Node.js pretty effectively 🎉

@bcoe bcoe requested a review from TimothyGu December 18, 2017 00:34
@schuay
Copy link

schuay commented Dec 18, 2017

☝️ I'm ecstatic that this is working; but shouldn't it not be working since I'm not spawning with --inspect-brk? I was under the impression once the Node process starts we're not able to enable block level coverage.

I'm not sure what the specific timing is in your case. As long as block coverage is enabled prior to a function being parsed, we can provide block-granularity data. This could also be due to things like lazy parsing of non top-level functions. Awesome that it works here :)

@bcoe bcoe merged commit f14508e into master Jan 1, 2018
@bcoe bcoe deleted the spawn-wrap branch January 1, 2018 22:59
Copy link
Contributor

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Sorry about the late review. I seem to have forgotten about this 😐 Nonetheless, I’ve attached two comments.

try {
// bootstrap the inspector before kicking
// off the user's code.
inspector.open(0, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t necessary. The in-process Inspector implementation works without launching the WebSocket server, which is what inspector.open does.

return isAbsolute(url) &&
exclude.shouldInstrument(url) &&
url !== __filename
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This filtering mechanism generally doesn’t work very well for Windows, but I don’t really have any easy way to fix that. I’ve been wanting to publish a module that does this conversion the same way Node.js core does it, and I’ll report back when I do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants