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

Integrating coverage reports #1054

Closed
mikeal opened this Issue Oct 16, 2017 · 32 comments

Comments

Projects
None yet
9 participants
@mikeal
Copy link

mikeal commented Oct 16, 2017

After some recent PR's we've got some test coverage working w/ browserify-istanbul & puppeteer.

There's quite a bit of hacks that we had to pull to get it working and it still doesn't properly. There's also a bug, that looks like it's from the puppeteer side, when running tests with tap --coverage.

not ok Page Error: Error: ReferenceError: cov_vnbsudnht is not defined at <anonymous>:1:12

And then a big traceback that completely miss-reports the line that must have generated it.

I'm not an expert on how coverage is instrumented for istanbul/nyc/tap so I'm a little out of my depth at this point, but I figured ya'll had thought a bit about coverage and how it might be best integrated.

/cc @bcoe @izs @alexindigo

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Oct 16, 2017

@mikeal This surfaced a while back: #763 (comment)

Not sure how we can help here though.

@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Oct 16, 2017

@aslushnikov, @mikeal and I have been hammering out an approach that works well with Istanbul and puppeteer ... I wonder if we could figure out a way to make Istanbul not step on the toes of puppeteer, while also documenting somewhere how to get test coverage?

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Oct 16, 2017

@bcoe do you care about coverage of the browser-run code? E.g. given the following script, would you care about coverage of the evaluated function?

await page.evaluate(() => {
  alert('foo');
});
@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Oct 17, 2017

If I'm understanding how things work; I don't think we care about the coverage of alert('foo'); -- the code that we're collecting coverage for will have been pre-instrumented by the user running tests (using a tool like webpack or browserify); It might be enough to place istanbul ignore next at a few key places in the puppeteer codebase ...

This might also be of interest, and coincidently got submitted today:

istanbuljs/istanbuljs#108

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

So, the following code.

async () => {
  same(clean(document.querySelector('test-two render').textContent), '2')
}

When .toString() with coverage enabled is:

async()=>{cov_vnbsudnht.f[0]++;cov_vnbsudnht.s[3]++;same(clean(document.querySelector('test-two render').textContent),'2');}

Is there a way to dynamically pull this object from the Node.js scope and represent it in headless Chrome?

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

To clarify, the reason we are getting errors in evaluate() is because this code is injected in to all lines in Node.js when coverage is enabled.

When this is then run in puppeteer this object doesn't exist.

We should be able to fix this if we can:

  • detect when coverage is enabled
  • look for references to coverage objects in evaluate strings prior to evaluation.
  • crate a representation of the object in the browser that proxies these calls to the same object on the Node.js side.
@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

@bcoe I don't think that we want to ignore these lines. These are lines of the test file, if they end up being dead code that never runs we actually want that in the report.

@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Oct 17, 2017

@mikeal in general the test file should be skipped for instrumentation; I'm a bit confused as to why the lines of the assertion would be instrumented for test. You can see the ignore logic here.

As long as the source-code being passed to puppeteer for execution has been instrumented in full, it will have a coverage tracking object populated in it -- I'm a bit confused how the partially covered snippets of code are making their way into puppeteer (I was equally curious when I read through #763).

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Oct 17, 2017

Is there a way to dynamically pull this object from the Node.js scope and represent it in headless Chrome?

@mikeal You can create harness using the page.exposeFunction.

Something along the lines:

// Provide window.countCoverage function in browser context
await page.exposeFunction('countCoverage', coverageProperty => {
  cov_vnbsudnht[coverageProperty]++; // very rough; there should be array index as well
});
// Add harness to browser context that marshals coverage calls to node
await page.evaluate(covid => {
  window['cov_' + covid] = new Proxy({}, {
   // ... proxy property access over to node using the window.countCoverage
}, covid);
@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

@bcoe by default the test file lines are included in coverage. In all my other projects I leave this on because it has actually found a lot of bad tests with dead code in them that we thought was checking for something.

As long as the source-code being passed to puppeteer for execution has been instrumented in full, it will have a coverage tracking object populated in it -- I'm a bit confused how the partially covered snippets of code are making their way into puppeteer (I was equally curious when I read through #763).

So, check out this line of code https://github.com/mikeal/gza/blob/coverage/tests/test-browser.js#L165

The function passed to evaluate is not executed in Node.js, it is .toString()'d and then eval'd in the page's context in puppeteer. Unless we skip coverage of the file in question, that function will be instrumented, and that partial instrumentation will fail when eval'd in puppeteer :)

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

@bcoe is there only one cov_${id} object in the entire process? Is there any way to detect the id(s) of these in the Node.js process?

@alexindigo

This comment has been minimized.

Copy link

alexindigo commented Oct 17, 2017

Ha, now I remember hitting similar issue with Phantom :) But somehow it slipped my mind this time :)

@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Oct 17, 2017

@mikeal there will only be one cov_${id} in the entire project, post instrumentation it will have been inserted at the very top of the test file -- I wonder if you could potentially kick this along with the code that's being serialized ... maybe wrap the default behavior?

edit: if this can be pulled into some modules that work for the generic case, I'd love to add a section on https://istanbul.js.org/.

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

@bcoe if there's only one of these with the same name for the entire process then we just need to know its name and find a globally accessible reference to it.

With that we could implement this proxying behavior in puppeteer and dynamically insert the proxy object when globals.__coverage___ exists :)

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

@alexindigo

This comment has been minimized.

Copy link

alexindigo commented Oct 17, 2017

Here it's just passed with the arguments
https://github.com/alexindigo/obake/blob/master/lib/phantom.js
although it's somewhat limited use case.

@alexindigo

This comment has been minimized.

Copy link

alexindigo commented Oct 17, 2017

Does __coverage__ object has any metadata in puppeter?

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

I figured out what the cv_${id} objects are and where to find them :)

There's an object per file and the key can be derived from the filename. The object itself is actually on the global __coverage__ object but that object has keys of the entire filename so you just need to re-hash it the same way the coverage system does it.

let cv_objects = {}
Object.keys(__coverage__).forEach(filename => {
  let hash = createHash('sha1')
  hash.update(filename)
  let key = parseInt(hash.digest('hex').substr(0, 12), 16).toString(36)
  cv_objects[key] = __coverage__[filename]
})
@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

I think that with this I can start working on a patch tonight/tomorrow that can detect coverage and make puppeteer's evaluate function "just work" :)

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 17, 2017

Alright, PR is sent #1067

Could use some help fixing Typescript globals errors.

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Oct 31, 2017

Summarizing, there are two issues here:

  1. enabling istanbul to work with pptr - important!
  2. collecting coverage inside browser context - less critical, orthogonal to [1] and should be handled separately. For the record, there's a gotwarlost/istanbul#254 that might come handy.

Let's scope this issue to cover [1].

I see a few options to enable istanbul with pptr projects:

  1. Fix gotwarlost/istanbul#310 and gotwarlost/istanbul#674 - istanbul spoiling function's toString() method. It might be that this could be fixed by injecting the following small code snippet in the beginning of istanbul-preprocessed code:
(() => {
  const originalToString = Function.prototype.toString;
  Function.prototype.toString = () => {
    const code = originalToString.call(this);
    return code.replace(/\bcov_[^;]+/g, '');
  }
})();

@mikeal mentioned this wouldn't work, though I don't understand why. Having this as an istanbul config option seems pretty safe.

  1. Adding cov_XX && prefix to every istanbul coverage trace. This will make the cov_XX.s[3]++ trace to become cov_XX && cov_XX.s[3]++, and might also help to address gotwarlost/istanbul#119

  2. Recommend clients to use istanbul's code ignore comments to guard pptr's evaluations.

@bcoe @gotwarlost @mikeal thoughts?

@mikeal

This comment has been minimized.

Copy link
Author

mikeal commented Oct 31, 2017

@aslushnikov

I don't think this is a good path. This actually breaks the coverage we have working in cappadonna and will effectively miss the .execute(code) that runs in the browser.

I don't see why an istanbul config option would be useful. This breaks coverage detection in many of the blocks you would configure it for, it's better to just opt those files out of coverage if you're concerned about this.

We actually have something that "just works" if you use a higher level tool https://github.com/mikeal/cappadonna/blob/master/index.js#L95

I know @bcoe is working on integrating with v8's coverage utilities but those are still quite far behind and, as far as I understand it, don't include the level of branch coverage we already have working in istanbul.

If that gets to the point we can rely on it then we would just want to change instanbul's function .toString() by default.

@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Nov 1, 2017

@aslushnikov 👋

I did something similar to what you're proposing, to preserve function names (Istanbul's instrumentation at one point broke Function.name). I'm glad we put in the work to rewrite code in such a way that the name was preserved, but it was a hassle and took quite a few iterations to get right... I think that preserving the original toString() would be significantly more complex and I think would also lead to a significant performance hit when running instrumented code.

What I would rather see would be that we work with the V8 team to close the delta on V8 coverage vs. Istanbul coverage.

Newer versions of V8 do support block level coverage, but I think there are still some missing features (I was playing last night, and for instance conditional assignment seemed to not be covered).

I suggest we loop in folks like @schuay, and see if we can't get V8 coverage in Node.js over the finish line, vs. adding any more complexity to Istanbul's parser -- which I'd honestly love to retire.

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Nov 1, 2017

V8 JS coverage sounds great to me, it's way nicer then what istanbul is currently doing.

Newer versions of V8 do support block level coverage, but I think there are still some missing features (I was playing last night, and for instance conditional assignment seemed to not be covered).

@bcoe: do you have a list of missing parts? cc @ak239 who lives and breathes V8.

@bcoe

This comment has been minimized.

Copy link
Contributor

bcoe commented Nov 1, 2017

@aslushnikov I've been working from this testing repo where I intend to document the deltas I find between Chrome's coverage functionality, and the instrumentation folks have become accustomed to with Istanbul.

Here are a few related issues:

discussion on v8 issue tracker, regarding adding deatiled coverage to Node.js.
discussion on Node.js project, regarding adding better v8 coverage support

@ak239 I would definitely be curious to know your thoughts around v8's built in coverage; here's an example of where I think things are lacking (maybe I'm just not doing things properly?):

code to instrument:

const foo = true || (9 + 10)

instrumenter output:

{
  "result": [
    {
      "scriptId": "56",
      "url": "file://test/fixtures/b.js",
      "functions": [
        {
          "functionName": "",
          "ranges": [
            {
              "startOffset": 0,
              "endOffset": 29,
              "count": 1
            }
          ],
          "isBlockCoverage": true
        }
      ]
    }
  ]
}

My expectation would be something that indicates that the block (9 + 10) has not been executed... In Istanbul there's the construct branchMap which tracks specific branches that have been executed. I think my expectation was that Chrome's detailed coverage would provide something close to this level of granularity.

@ak239

This comment has been minimized.

Copy link
Contributor

ak239 commented Nov 1, 2017

I left comments about teardown and tearup events. As far as I know precise coverage supports block - level coverage and true || (9 + 10) are not separate blocks.
@schuay, please take a look.

@schuay

This comment has been minimized.

Copy link

schuay commented Nov 2, 2017

V8's block coverage doesn't support short-circuiting boolean operators yet, see: https://crbug.com/v8/6660 and the list of known limitations here. If this is important to you please let us know.

cc @hashseed

@hashseed

This comment has been minimized.

Copy link

hashseed commented Nov 2, 2017

We'd love to help with fixing any issues the community feels are important. We have long-term plans to no longer require reload when block-based coverage is enabled, also to solve the issue in Node.js where reload is not possible. That however will require some more time.

@aslushnikov

This comment has been minimized.

Copy link
Contributor

aslushnikov commented Jan 10, 2018

CSS and JS coverage landed in pptr a few days ago: page.coverage

Please give it a try!

@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Aug 5, 2018

@aslushnikov I don't think page.coverage() solves the issue of wanting to run nyc on the NodeJS side without breaking Puppeteer's page.evaluate()

@c094728

This comment has been minimized.

Copy link

c094728 commented Aug 23, 2018

Has there been any progress on this issue? I have this in my code to allow coverage report to run.

    /* due to this issue https://github.com/GoogleChrome/puppeteer/issues/1054,   
     * the istanbul (coverage reporter) will cause error inside page.evaluate throwing exception. 
     * the following "ignore" comment allows istanbul to succeed */
    /* istanbul ignore next */
    var hrefs = await page.evaluate(() => {
        const anchors = document.querySelectorAll('a');
        return [].map.call(anchors, a => a.href);
    });
@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Aug 23, 2018

@c094728 I think that is the proper approach, which I have taken too. You can also put the comment right infront of the arrow function to make sure really only the function is ignored, not the page.evaluate() call itself:

https://github.com/felixfbecker/semantic-release-firefox/blob/78fb91f59ed2730536fbd986ab081281a9f019be/src/publish.ts#L106-L111

I don't think Puppeteer can or should do something here - the function must be prevented from being instrumented because it is serialised and sent to another context, but there is no way istanbul/nyc could know that, and no way for Puppeteer to tell istanbul/nyc. The developer is the one who should annotate the function, and they can do so with istanbul ignore comments, which works fine.

HaNdTriX added a commit to HaNdTriX/next.js that referenced this issue Nov 1, 2018

HaNdTriX added a commit to HaNdTriX/next.js that referenced this issue Nov 1, 2018

HaNdTriX added a commit to HaNdTriX/next.js that referenced this issue Nov 1, 2018

HaNdTriX added a commit to HaNdTriX/next.js that referenced this issue Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment