ember-cli / rfcs

Archive of RFCs for changes to ember-cli (for current RFC repo see https://github.com/emberjs/rfcs)
45 stars 54 forks source link

Add lcov endpoint RFC #57

Closed stefanpenner closed 7 years ago

rwjblue commented 8 years ago

How does this compare to ember-cli-code-coverage? Seems like most of this works out of the box for apps (with addon support following along soon).

stefanpenner commented 8 years ago

How does this compare to ember-cli-code-coverage? Seems like most of this works out of the box for apps (with addon support following along soon).

Does it support parallel tests? Also, I believe it munges code as part of build time, rather then at runtime. Which means running coverage requires re-building (or special build) rather then attaching to an existing app. Which I am still uncertain about. But would love to hear more.

@kategengler please confirm or deny ^

kategengler commented 8 years ago

It doesn't currently support parallel tests but could since Istanbul does support combining coverage results.

It does only instrument code at build time, rather than runtime. That made it much easier to only instrument relevant code and use Istanbul in a recommended way (they suggest only using client-side instrumentation in demos). Not sure what you mean about attaching to an existing app.

It works roughly like this:

  1. COVERAGE=true ember test -s (or ember test or ember s + going to /tests)
  2. App files are instrumented as part of the build
  3. Tests run and hit an endpoint with the coverage results, write the report to disk and put coverage #s in the test runner

Currently the last report written wins, which is part of what we'd have to change to support parallel tests.

stefanpenner commented 8 years ago

Not sure what you mean about attaching to an existing app.

we run tests against prod builds of our apps, our app.js stays identical between test runs and prod.

stefanpenner commented 8 years ago

Currently the last report written wins, which is part of what we'd have to change to support parallel tests.

So it seems the above proposal would also work for you add-on. Perfect

kategengler commented 8 years ago

Gotcha. Yeah, ember-cli-code-coverage wouldn't work in that case.

I think I could make parallel tests work without any hackery now even, but I have no objection to standardization.

stefanpenner commented 8 years ago

Gotcha. Yeah, ember-cli-code-coverage wouldn't work in that case.

ya its not for everyone.

I think I could make parallel tests work without any hackery now even, but I have no objection to standardization.

Ya, you could (during one test run, merge all posted files) I would like to standardize it though. Does your setup just post lcov, or what format/contents are posted.

sglanzer-deprecated commented 8 years ago

Looks fine to me at a glance, just a few notes:

How would other reporter formats (e.g. HTML, JSON) be handled? Writing other formats to the endpoint or a post-processing hook (assuming that's a thing)

I'm planning on deprecating blanket in favor of ember-cli-code-coverage so if that project handles writing from parallel execution then we're covered there

HTML reports/integration - do we have any intention on using the middleware to host coverage reports (a la test run reports) or would that be left to addons to implement and integrate with the testing frameworks (qunit, mocha)?

@jschilli @sandersky anything to add?

stefanpenner commented 8 years ago

I'm planning on deprecating blanket in favor of ember-cli-code-coverage so if that project handles writing from parallel execution then we're covered there

some of us will still require runtime instrumentation, so we will need to make sure ember-cli-code-coverage also supports that (not just build-time instrumentation) .

stefanpenner commented 8 years ago

HTML reports/integration - do we have any intention on using the middleware to host coverage reports (a la test run reports) or would that be left to addons to implement and integrate with the testing frameworks (qunit, mocha)?

future work likely

sglanzer-deprecated commented 8 years ago

some of us will still require runtime instrumentation

Fair point, we'll make sure to distinguish the use cases for the two projects until that functionality can be absorbed. I'm still going to promote the "core" project as the standard though since that's where the majority of the effort is going

stefanpenner commented 8 years ago

@sglanzer istanbul supports both, although encourages the server-side transpile step. I suspect we can make it configurable.

If we are merging, should we both adding this to ember-cli / testem, or do we add it to our addons?

sglanzer-deprecated commented 8 years ago

If we are merging, should we both adding this to ember-cli / testem, or do we add it to our addons?

Can you clarify this? I'm not sure what you're referring to

jschilli commented 8 years ago

totally support the effort to institutionalize coverage capture.

The biggest issue we had (or one of) with ember-cli-blanket is the client side transpile was divorced from the original source by at least two steps - module loading and the additional coverage annotation (which was not source map aware)

A whole series of hacks emerged to support coveralls and other solutions to map coverage back to original source files.

I also think it's pretty important to support instrumentation for ember test (which is afaik being proposed) b/c for our largest products doing multiple ci build/test runs is prohibitively expensive.

I'm far from an expert on the LCOV format but my experience suggests that translating LCOV to almost anything is possible.

I'd be interested if anyone has a use case that isn't covered in LCOV

sglanzer-deprecated commented 8 years ago

my experience suggests that translating LCOV to almost anything is possible

I would assume this as well, which is why I was thinking a post-processing hook would be very valuable

rwjblue commented 8 years ago

I agree with @sglanzer, I would like to see this RFC add an additional addon hook to allow post processing the coverage information. I'm happy with it being something generic like postTest (similar conceptually to postBuild), or something more tailored to coverage collection.

stefanpenner commented 8 years ago

@rwjblue I'm not sure why this RFC requires that, seems orthogonal or supplemental. But maybe you can share more so I can accommodate

fotinakis commented 8 years ago

Another thing this RFC might need to address: common libraries used in Ember tests like ember-cli-mirage, pretender directly, or mockjax, block new network requests by default, so a new testem middleware endpoint like /_testem/lcov might be silently blocked depending on what other addons are in the app.

ember-cli-blanket currently has this problem as it uses a /write-blanket-coverage testem endpoint: https://github.com/sglanzer/ember-cli-blanket#reporters-and-testing-mocks My library ember-percy also has this problem with its /_percy/snapshot endpoint, with basically the same documented workaround: https://percy.io/docs/clients/javascript/ember

The workaround is usually a hacky version of "add these things to your pretender config" (which unfortunately differ based on if you're using ember-cli-mirage or pretender directly, and what version of jQuery, and need to ensure it's above a this.namespace declaration, etc). It's interesting that testing-specific middleware is becoming a more and more common pattern in Ember tests.

stefanpenner commented 7 years ago

I think this is still a good idea, but doesn't seem like we have time/bandwidth. We can always revist

trentmwillis commented 7 years ago

Agreed. One of the primary use cases (e.g., code coverage with parallelized tests) has already found a decent, alternative solution anyway.