Skip to content
This repository was archived by the owner on Dec 8, 2024. It is now read-only.

instrumentation for multi-process code #199

Open
jsdevel opened this issue May 3, 2014 · 13 comments
Open

instrumentation for multi-process code #199

jsdevel opened this issue May 3, 2014 · 13 comments

Comments

@jsdevel
Copy link

jsdevel commented May 3, 2014

I have a project that'll end up being a CLI to start a daemon. The daemon will start an express app, and will allow REST interaction to start clusters. The clusters will all call fork().

I can take the unit test approach and use istanbul cover with no problems; however, I see more value in being able to test my application from a very high level. So I've created a simple bash script that will interact with the CLI and exercise the application in that fashion. I'd really like to handle code coverage in this way if possible.

At first I attempted to instrument all my files, and require the covered entry module from the CLI script based on the existence of an env var. Unfortunately, the instrumented code doesn't do any sort of reporting on it's own, so my baseline coverage files never get updated.

Punchline: It would be really nice if there was an option with istanbul instrument that would embed a reporter in the instrumented files. This would allow the baseline-coverage.json files to get updated across processes. I've been browsing the issues that are currently open related to multi proc / clustered code (#147, #127, #115), and I believe the proposed solution herein would solve them more elegantly.

I currently work at GoDaddy, and I know several of the engineers / architects would prefer to take this approach to testing where possible. /cc @asilvas @tracker1 @azweb76

@parautenbach
Copy link

+1

I have a similar situation.

@Raynos
Copy link

Raynos commented Aug 13, 2014

I have inlined this function into my lib folder for this use case (using child_process.spawn instead of cluster.fork)

function spawnChild(file, args, opts, callback) {
    /*jshint camelcase: false */
    var isIstanbul = process.env.running_under_istanbul;

    var cmd;
    // istanbul can't actually cover processes that crash.
    // so there is little point as it doesn't add much coverage
    // in the future it will https://github.com/gotwarlost/istanbul/issues/127

    if (isIstanbul) {
        cmd = istanbul + ' cover ' + file + ' --report cobertura' +
            ' --print none' +
            ' --root ' + process.cwd() +
            ' --dir ' + coverageFolder + '/multiple' +
            count + ' -- ' + args;
    } else {
        cmd = 'node ' + file + ' ' + args;
    }

    count++;
    return exec(cmd, opts, beSilent);

    function beSilent(err, stdout, stderr) {
        if (stderr) {
            stderr = stderr.replace('No coverage information ' +
                'was collected, exit without writing coverage ' +
                'information\n', '');
        }

        callback(err, stdout, stderr);
    }
}

I think we can create a similar fix for cluster.fork

We can start by creating an istanbul-child-process module to support this use case.

We can then consider getting it upstreamed in istanbul itself.

@jonathanong
Copy link

+1 would really like support for this!

@Raynos
Copy link

Raynos commented Aug 22, 2014

Let's write a istanbul-child_process module that solves this problem (including cluster.fork use cases).

Once we have gone through a couple of minor versions of that we can then upstream that entire feature into istanbul.

@jonathanong
Copy link

i'll test it

@TimothyGu
Copy link

Just want to +1 this. In jadejs/jade, we are testing the jade CLI app with childProcess.spawn() and .exec(). It's a pity that istanbul is unable to pick that up.

@mysterycommand
Copy link

What's the latest on this? I see a merged PR, but the issue is still open?

@fengmk2
Copy link

fengmk2 commented Aug 29, 2015

Is there a way to merge all coverage.json, coverage-*.json into one html report?

@fengmk2
Copy link

fengmk2 commented Aug 29, 2015

Wow, I found out, just run istanbul report, it will merge all coverage-*.json!

ingomueller-net referenced this issue in ingomueller-net/oms-core Nov 28, 2015
If we start it through server.js, then everything that runs in
the new process will not be tracked by the code coverage tool.
szilveszter9 added a commit to szilveszter9/istanbul that referenced this issue Apr 22, 2016
@cdaringe
Copy link

@Raynos, are your thoughts that istanbul-child_process would essentially modify child_process s.t. all calls, such as cp.spawn(cmd, [ args... ]), would first be intercepted by istanbul-child_process & have the coverage bits injected, then run the modified child process?

it sounds straightforward enough. however, how do the child coverage stats get aggregated with the parent coverage stats?

@Raynos
Copy link

Raynos commented Apr 27, 2016

@cdaringe https://github.com/bcoe/nyc

This has already been implemented.

@cdaringe
Copy link

@Raynos waaaat

should this issue be closed then, or does istanbul folks want to integrate said feature into this lib?

szilveszter9 added a commit to szilveszter9/istanbul that referenced this issue May 18, 2016
…e/issues/gotwarlost#199 and deepsweet/istanbul-instrumenter-loader/issues/gotwarlost#16
szilveszter9 added a commit to szilveszter9/istanbul that referenced this issue May 18, 2016
…e/issues/gotwarlost#199 and deepsweet/istanbul-instrumenter-loader/issues/gotwarlost#16
@silkentrance
Copy link

for people having problems with collecting coverage data from different processes, please have a look at the tests over at https://github.com/raszi/node-tmp

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

No branches or pull requests

9 participants