search

agilgur5 / physijs-webpack

PhysiJS port for bundlers with out-of-the-box support for Webpack and Browserify
Other
9 stars 3 forks source link

Add code coverage #6

Open agilgur5 opened 5 years ago

agilgur5 commented 5 years ago

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 commented 5 years ago

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 https://github.com/istanbuljs/nyc/issues/1130 to see if this is a bug or if someone could point me in the right direction

agilgur5 commented 5 years ago

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 commented 4 years ago

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)