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

Coverage reported by Travis CI reflects reduced coverage from IE w/ skipped tests #4121

Closed
kfranqueiro opened this issue Nov 28, 2018 · 0 comments

Comments

@kfranqueiro
Copy link
Contributor

kfranqueiro commented Nov 28, 2018

It turns out that #4046 did have an effect on reported coverage, but only in the context of our CI. (For example, see before vs. after.)

For whatever reason, istanbul is now using the coverage reported for the IE test run, which has lower coverage due to ripple-related tests being skipped. It used to report the coverage numbers seen for Chrome / Firefox / iOS Safari, when we were running all CI unit tests through browsers on Sauce Labs.

I'd like to be able to say that it's reporting the coverage based on the last finished test, but I can't even conclude that, because IE always finished last, even previously when we were running all browsers on Sauce Labs.

There are multiple possible ways to work around this or attempt to resolve this:

  1. Reduce the required coverage from 95% to 94% to account for the dip on IE (which is a <1% difference)
  2. Figure out a way to not skip ripple-related tests in IE (in Dialog, Select, and Tab) so this becomes a non-issue (currently we're only able to test by detecting the ripple-upgraded class, which is never applied on IE)
  3. Split up our karma conf into 2 configurations, to run sauce labs before local, so that the report reflects Chrome/Firefox coverage (this would make unit tests take a minute longer because they can't be parallelized between local and SL)
  4. See whether upgrading to nyc (istanbul's new version) would pick up reports from multiple browsers better... but documentation seems confusing at best and migration documentation seems nonexistent, and I have no idea if it will even fix the issue
  5. Improve coverage elsewhere to get us further away from the failure threshold (or add comments to avoid instrumenting code that is infeasible to cover)

Option 1 is the quickest short-term fix if we encounter more problems with dipping below coverage in PRs (we hit it in #4055). Option 2 would probably be preferable to solve this. Option 5 is probably also something we should pursue, eventually.

@abhiomkar abhiomkar added this to the v0.43.0 milestone Nov 29, 2018
@kfranqueiro kfranqueiro modified the milestones: v0.43.0, v0.44.0 Dec 14, 2018
@kfranqueiro kfranqueiro removed this from the v0.44.0 milestone Feb 1, 2019
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

3 participants