Add code coverage to pdf.js

[jezhou]

Fixes #8632.

This is a first pass at generating test coverage reports for PDF.js. It seemed straightforward enough if I used Istanbul's nyc tool to wrap the current test command.

npm run coverage # runs `npx nyc -- npm test`

Doing that seems to generate some rudimentary coverage in my terminal. Here's what it looks like:

I ended up not diving too deep into the gulpfile since using nyc in this way seemed to be a more declarative way of generating the coverage, but I'm open to changing it to exclusively containing it in the gulpfile if there's a nit here that I'm not seeing?

Also, I'm pretty sure I'm missing maybe a few other test commands that need coverage, but I rather get feedback on everything first since this is my first time using Istanbul.

[Rob--W]

Have you verified that your way of running tests is compatible with all tests that we run (see #8632)?
If the CLI approach works with our tests, then I don't have anything against skipping the gulp part.

Does the current version of the PR generate output that can be consumed by coveralls?

[jezhou]

@Rob--W I've only verified with the gulp npm-test command, which seems to be somewhat of a proxy to the unittestcli command. I can try the others tonight.

One question I had is that should the coverage command trigger all of the test runs mentioned in #8632 and generate a single, large report? Or do you want each set of tests to generate their own reports?

Does the current version of the PR generate output that can be consumed by coveralls?

I'm not sure - I would think in it's current state no, but it might just be a matter of configuration. Is there a specific format coveralls is expecting / is there an example I can look at?

[jezhou]

Also, after reading #8632 (comment), I've realized that most of the instrumentation I have is coming from the build folder, which means I'm instrumenting the already babel-transpiled version, right?

Based on the comment, it sounds like the coverage should ideally come from src and not build?

Let me know if there are other folders that need to be included for coverage

[jezhou]

One last question: How do I run the last two tests you mention in #8632? Namely examples/node/pdf2svg.js and the browser extensions. Do I just need to run the former in node, and just check if the web extension compiles?

[jezhou]

@Rob--W I have a better implementation that I'm working on cleaning up. I'll re-tag you for a review when it's ready. Do you think you can answer these questions when you have the chance? The others I figured out through experimenting and reading through the last PR attempt at this.

#11580 (comment) 👈 the questions

[Rob--W]

One question I had is that should the coverage command trigger all of the test runs mentioned in #8632 and generate a single, large report? Or do you want each set of tests to generate their own reports?

In continuous integration, the coverage report should ideally be the combination of unittestcli+unittest+browsertest, as this allows us to see at once which code is dead or untested. If not feasible, it is fine to have separate reports per test type.

For local development and experimentation, it would be nice to get the coverage report from a single run of specific test(s). This allows one to easily check which part of the code is used when a specific PDF file is loaded.

Does the current version of the PR generate output that can be consumed by coveralls?

I'm not sure - I would think in it's current state no, but it might just be a matter of configuration. Is there a specific format coveralls is expecting / is there an example I can look at?

Based on the comment, it sounds like the coverage should ideally come from src and not build?

Ideally src/ should be covered. But if all else fails, build/ is better than no results at all.

One last question: How do I run the last two tests you mention in #8632? Namely examples/node/pdf2svg.js and the browser extensions. Do I just need to run the former in node, and just check if the web extension compiles?

This is not a test, but a demo, so just run it. My main motivation for getting coverage reports for this is to easily check which part of the code is activated when I run it for a specific file. This does not need to be integrated in continuous integration, just documenting how to run it should be sufficient.

[jezhou]

@Rob--W I think I've gotten as far as I could without help; I can get the three tests unittest unittestcli and browsertest working locally, but I can't get unittest or browsertest to run on Travis. Maybe because it needs a browser? I'm not sure how to run those tests in the CI environment.

This is the failing build: https://travis-ci.org/mozilla/pdf.js/builds/652327117?utm_source=github_status&utm_medium=notification

Coveralls seems to fail as well, maybe because some config needs to be set up first. Can you guide me on how to do that? I get this error as of right now:

Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

If it helps, here's the "high level" report coverage from my computer. Can you cross check it to see if the coverage "feels right"?

=============================== Coverage summary ===============================
Statements   : 61.5% ( 20472/33287 )
Branches     : 51.14% ( 5334/10430 )
Functions    : 66.93% ( 1591/2377 )
Lines        : 61.68% ( 20053/32510 )
================================================================================

[Rob--W]

browser tests are not automatically run by Travis, but on demand via botio, see https://github.com/mozilla/pdf.js/wiki/Bots and https://github.com/mozilla/botio-files-pdfjs

These days Travis does support the use of actual browsers, see e.g https://docs.travis-ci.com/user/chrome and https://docs.travis-ci.com/user/firefox (and https://docs.travis-ci.com/user/gui-and-headless-browsers/ ). But it would be a complete project on its own to move from on-demand botio tests to running the lot of browser tests on Travis' infrastructure. I think that it's very worthwhile to explore, but it is independent of this bug.

To resolve the original issue, we need to be able to:

1. Generate coverage data when running our tests.
2. Combine them (e.g. unittestcli + browsertests).
3. Generate (HTML) reports.

• and when merged: upload the data from step 2 to coveralls so that coveralls can generate the report on our behalf

The integration with coveralls is the last step (and probably the easiest), and can be done when the first parts of the implementation are done. ( It has been a while since I've set up coveralls on a new repository, but I believe that you could test coveralls integration on your own fork, via https://github.com/marketplace/coveralls )

The browser tests are run in a web browser, and I don't see special logic in the current version of this PR to transfer the coverage data from the browser to the test runner. This means that the report that you're getting is most likely not including the data from the browser tests.

I suggest that you read the conversation starting from #8632 (reference) , and maybe even take a look at the (closed) PR (8632) to get some insights in the task and the previous attempt to resolve it.

[Snuffleupagus]

Closing for now, since there's not been any updates for well over a year and as-is this patch cannot land; issue #8632 remains open to track this.