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

Exclude devtools tests from coverage #1808

Merged
merged 3 commits into from
Aug 5, 2019
Merged

Conversation

marvinhagemeister
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jul 26, 2019

Coverage Status

Coverage increased (+0.8%) to 99.627% when pulling ec211c8 on devtools_tests_skip into 4a8e01d on master.

ua.indexOf('Edge') === -1 &&
ua.indexOf('Chrome') > -1 &&
ua.indexOf('Firefox') > -1
) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-statement is the only change here in this file. Everything else is just indentation. I tried using the this.skip-API like the mocha docs recommend, but it will still run some tests contrary to what the docs say.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you like exit or return or something for early exit instead of indenting everything?

Copy link
Member

@developit developit Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this works:

const supported = /Chrome|Firefox/.test(navigator.userAgent) && !/Edge/.test(navigator.userAgent);
const desc = supported ? describe : describe.skip;

desc('..', () => {
  ..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's clever! I like it 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ForsakenHarmony tried the early return, and it works too. I think I'll go with @developit 's approach though, it's more clear and prints that the devtools test have been skipped 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ForsakenHarmony I was wrong! It doesn't make any difference 😂

@marvinhagemeister
Copy link
Member Author

marvinhagemeister commented Jul 26, 2019

Hm I'm not sure if there is a way to tell coveralls to pull the coverage from one browser only.

EDIT: Ah shucks, it seems to be an open issue with karma-coverage. It will only collect coverage data from the last browser that ran :/ See karma-runner/karma-coverage#9 .

@developit
Copy link
Member

@marvinhagemeister we could probably have the last browser always be Chrome?

@marvinhagemeister
Copy link
Member Author

@developit I'm not sure how can ensure that it's the last one. Once Saucelabs takes over the order seems to be in their hand.

The devtools code uses newer ECMAScript features because the
devtools extension is only available for Firefox and Chrome. But
we didn't have such a distinction in our tests suite. That leads
to failing tests on IE11.

We tried running those tests only on FF and Chrome, but because
karma-coverage always collects coverage data from the latest
running browser, it would lead to coverage failures. The latest
browser is typically slowest and you may have guessed it already:
This will pretty much always be IE11.

The nearterm conclusion is to just disable coverage reporting
for the devtools code completely...
@marvinhagemeister marvinhagemeister changed the title Only run devtools tests in Firefox or Chrome Exclude devtools tests from coverage Aug 5, 2019
@marvinhagemeister
Copy link
Member Author

Tried a few other things, but I don't see any other way than to disable coverage reporting for the devtools code...

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.

5 participants