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

Add code coverage #6

Open
agilgur5 opened this issue Jun 13, 2019 · 3 comments
Open

Add code coverage #6

agilgur5 opened this issue Jun 13, 2019 · 3 comments

Comments

@agilgur5
Copy link
Owner

agilgur5 commented Jun 13, 2019

Now that #5 adds a test harness, runner, and some tests, it should be simple to just add some code coverage. It'll have to be customized a bit to ignore mostly vendored code, i.e. physi.js, physijs_worker.js, and ammo.js (though there's not much code left after that 😅 ). Here's how to exclude/include with nyc.

I just started looking into it and realized a pretty big problem -- the tests import the bundled code, not the source code, so without some likely quite hacky source mapping, it's probably not possible to actually get coverage numbers for source code.
But the non-vendored code basically doesn't branch and is quite tiny, so nbd either way in all honesty.

@agilgur5
Copy link
Owner Author

agilgur5 commented Jun 18, 2019

Started working on this because we have tests so code coverage really makes sense to have too. It turns out nyc does have support for source mapped files, but after a large amount of trial-and-error on the coverage branch, I couldn't get everything working. I got webpack partially working, with some incorrect exclusions, and could not get browserify working correctly 😕 .

There isn't much documentation on source map support for nyc, especially when not using babel. Filed an issue at istanbuljs/nyc#1130 to see if this is a bug or if someone could point me in the right direction

@agilgur5
Copy link
Owner Author

agilgur5 commented Jul 30, 2019

Added a coverage-jest branch as well just to test if jest runs into the same problems with its built-in coverage reporting, and indeed, yes it does.

browserify.spec.js actually hits an infinite loop according to jest (maybe that's why it was just 0s in the nyc branch coverage?). The error looks like:

 FAIL  test/browserify.spec.js
  ● Test suite failed to run

    Infinite cycle detected

      at TraversalContext.maybeQueue (node_modules/@babel/traverse/lib/context.js:61:13)
      at NodePath._containerInsert (node_modules/@babel/traverse/lib/path/modification.js:83:15)
      at NodePath._containerInsertBefore (node_modules/@babel/traverse/lib/path/modification.js:91:15)
      at NodePath.insertBefore (node_modules/@babel/traverse/lib/path/modification.js:51:17)
      at VisitState.insertCounter (node_modules/istanbul-lib-instrument/dist/visitor.js:183:12)
      at VisitState.insertStatementCounter (node_modules/istanbul-lib-instrument/dist/visitor.js:213:10)
      at VisitState.coverStatement (node_modules/istanbul-lib-instrument/dist/visitor.js:334:8)
      at enter.forEach.e (node_modules/istanbul-lib-instrument/dist/visitor.js:319:9)
          at Array.forEach (<anonymous>)
      at VisitState.wrappedEntry (node_modules/istanbul-lib-instrument/dist/visitor.js:318:11)

webpack.spec.js similarly doesn't ignore correctly, but with nyc it at least ignored more.

First made me think the problem would be in istanbul core since it occurs in both, but the output is different in both so maybe not? idk, this is probably gonna go unresolved for some time.

I also realized users of this library trying to test their own code are going to run into similar issues with workers etc welp welp 😕 😞

agilgur5 added a commit that referenced this issue Nov 26, 2019
- also output source map files for nyc to use
  - (deps): add exorcist to browserify for this

- FIXME: currently bugging out:
  - browserify is not reading source maps or build folder at all?
  - webpack is reading source maps, but isn't excluding physi.js files
agilgur5 added a commit that referenced this issue Nov 26, 2019
- jest's coverage reporting only knows how to use inline source maps,
  so make them output as inline instead of external .map files
  - can remove exorcist now since inline source maps are built-in
    to browserify

FIXME: jest --coverage on browserify.spec.js runs into an infinite loop
- likely bc it's relying on istanbul-lib-instrument and running it on
  ammo and failing spectacularly -- need it to ignore ammo code

FIXME: webpack passes like with nyc but ignores nothing... :/
@agilgur5
Copy link
Owner Author

agilgur5 commented Nov 27, 2019

Ok so I spent far far far far too long trying to figure this out and still failed to get it to work correctly... The source map ignore stuff I'm pretty sure is a bug in some part of istanbul, though there isn't too much docs on the topic of coverage with source maps (or how to ignore, though I looked through some --exclude-after-remap=false issues, those didn't help)

That being said, I think I narrowed down the issues:

  1. I believe that the no files output with nyc and ava and the jest --coverage hitting an infinite cycle are probably the same issue. This issue is almost certainly caused by istanbul-lib-instrument/@babel/traverse trying to parse some ammo code and failing spectacularly. When I instrument the code myself and ignore ammo, like I did with the cypress-coverage branch, it stops having this issue and runs fine.
    Depending on where I use it, it either hits the infinite cycle errors, takes long but passes with no coverage info, or starts getting gc errors and eventually aborts itself.
    While I think jest should handle custom instrumentation as well to handle for these types of issues, the browserify output was also giving lots of issues in general that did not happen with the webpack output (like browserifying a browserify bundle caused it to use the requires defined externally and try to require actual files instead of the bundled code). It's possible I might be able to add some transform to browserify so it outputs the ammo code differently / more like webpack so these issues don't happen (it would probably still need source maps though).

  2. Bundling plus istanbul coverage (with or without source maps) seems to have issues and there seems to be no way to ignore some files. Like I can't ignore the webpack bootstrap or universalModuleDefinition. Trying to ignore it with like webpack.bundle.js:/webpack etc just doesn't work.
    Trying to ignore, say, test/utils/ inside of the bundle (webpack.bundle.js:/Physijs/test/utils/) doesn't work either, neither with trying to ignore the bundled "file" nor the source file. It similarly does not work when source maps are included (which basically just change it to say physijs-webpack/test/utils/ in the output instead).
    If it's manually instrumented, then ignoring seems to work correctly. But not if it's bundled and source-mapped, only if it's bundled and manually instrumented in the bundle (it also seemed like source-mapping it when it was manually instrumented made no difference to the output)

  3. The cypress-coverage, once I got it to work with custom instrumentation, didn't show any usage of physijs_worker.js. I think the worker just wasn't instrumented or something (I also got coverage results saying the browserify-worker-stub exported function was never run), so it doesn't appear in the coverage report. physijs_worker.js probably appeared when using jsdom-worker because the worker is just run in the same thread vs. Electron and Cypress run a real Web Worker.
    I'm not really sure how to instrument and run the worker (this may also need changes to @cypress/code-coverage potentially) so that it shows the worker coverage, but even if we figured that out, we might still run into the other 2 issues (although I seemed to be able to successfully ignore the physijs directory once I actually got all the custom instrumentation nyc config to work right)

agilgur5 added a commit that referenced this issue Nov 27, 2019
- instrument the code with istanbul and output usage into .nyc_output
- add nyc to create coverage reports with the data in .nyc_output

- output sourcemaps with browserify and webpack
  - FIXME: this doesn't seem to change anything????
agilgur5 added a commit that referenced this issue Nov 27, 2019
- instrument the code with istanbul and output usage into .nyc_output
- add nyc to create coverage reports with the data in .nyc_output

- output sourcemaps with browserify and webpack
  - FIXME: this doesn't seem to change anything????

FIXME: physijs_worker.js doesn't appear in coverage reports at all
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant