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

Cover Browser Tests (and all Node versions) #2886

Closed
wants to merge 5 commits into from

Conversation

ScottFreeCode
Copy link
Contributor

Fixes #2883.

Depends on #2868.

@ScottFreeCode
Copy link
Contributor Author

Most of the commits in this should disappear after #2868 is merged.

@ScottFreeCode
Copy link
Contributor Author

We keep getting Sauce 20,000ms disconnects on Edge, IE7 and Safari... Not sure if that's actually related to any of these changes or not.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 85.978% when pulling a066109 on multi-coverage into 7647e18 on master.

@ScottFreeCode
Copy link
Contributor Author

ScottFreeCode commented Jun 16, 2017

Relative coverage percentage has gone down because I made the browser coverage include browser-only files that the Node coverage was ignoring -- so there's more source being counted in the first place now. The current counts are "2097 of 2439 relevant lines covered (85.98%)" and "824 of 1038 branches covered (79.38%)" whereas, if you click on "PR base - master" you see "1988 of 2249 relevant lines covered (88.39%)" and "774 of 955 branches covered (81.05%)" -- so that's actually an increase of 109 covered lines and 50 covered branches (it just happens there are even more newly counted lines/branches that were never covered). Furthermore, if you look through the list of changes on the PR coverage page, none of the files had decreases in lines or branches covered.

The browser files just haven't had coverage to begin with; but we can measure and start correcting that now!

@ScottFreeCode
Copy link
Contributor Author

Thinking of rebasing onto master to:

  • get rid of extraneous commits from the CI-fixes branch in the history (instead just add the coverage-related changes against master now that master has the CI fixes merged in), thus ensuring that when this is merged it doesn't do anything weird to the overall commit history
  • correct the bug in the couple of bugged commits rather than having a separate commit for bug-correction
  • move the timeout adjustment to before the browser coverage

The latter two would allow both the browser coverage commits to each be tested on Travis and show valid results, demonstrating any changes in behavior from the first of the two browser coverage commits to the second, which may tell us something about the Sauce 20,000ms disconnect problem.

I will probably create a copy of this branch beforehand so we can bring it back if I goof up the rebase in any way.

@coveralls
Copy link

coveralls commented Jun 17, 2017

Coverage Status

Coverage decreased (-2.9%) to 85.486% when pulling 2aa9c6a on multi-coverage into 50fc47d on master.

@ScottFreeCode
Copy link
Contributor Author

Ok, so, this is exactly what I wanted to be able to see:

  • The commit before adding coverage to Karma has those Sauce 20,000ms disconnect errors on Edge, IE7 and Safari.
  • It only has them on the Travis PR build and not the Travis Push build.

That means, among other things:

  • It doesn't seem to be the addition of coverage that's at fault. Yay!
  • Something's different between this code in and of itself and this code pulled onto master? Despite the fact that this was just rebased onto master?? How is that possible?!

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 85.978% when pulling 2aa9c6a on multi-coverage into 50fc47d on master.

@ScottFreeCode
Copy link
Contributor Author

Why is Snyk flagging this PR? I click on "details" and I get a page showing the vulnerable dependencies we had before this PR. (And I double-checked, they're not also dependencies of the dependencies added in this PR.) Furthermore, the only dependency changes in this PR are to dev dependencies...

@coveralls
Copy link

coveralls commented Jun 18, 2017

Coverage Status

Coverage decreased (-1.2%) to 87.167% when pulling 58e4b5d on multi-coverage into 1f270cd on master.

@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@mochajs mochajs deleted a comment from coveralls Jun 18, 2017
@ScottFreeCode ScottFreeCode changed the title Cover Browser Tests Cover Browser Tests (and all Node versions) Aug 1, 2017
…eralls typically uses the Job ID, not the Build ID, so presumably it is designed to handle multiple jobs in the same build?
200 ms timeout was already causing flakes even before the coverage additions anyway; testing suggests that specific tests won't pass with instrumentation without increased time.
@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage decreased (-1.8%) to 87.208% when pulling d3aab5f on multi-coverage into 075bd51 on master.

@ScottFreeCode
Copy link
Contributor Author

Rebased onto master to get various fixes, but it seems the flakiness is still prevalent with coverage instrumentation (even though past commits demonstrated that it happened on this branch before adding instrumentation), and apparently Phantom is now even slower?

@ScottFreeCode ScottFreeCode added the area: repository tooling concerning ease of contribution label Aug 25, 2017
@ScottFreeCode
Copy link
Contributor Author

See #2890 concerning the test failures. Adding that to my follow-up list.

@boneskull
Copy link
Contributor

I'd like to rebase this and test it again.

@boneskull boneskull closed this Apr 8, 2018
@juergba juergba deleted the multi-coverage branch March 2, 2019 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants