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

Run coverage on travis #4605

Merged
merged 7 commits into from Oct 22, 2018
Merged

Run coverage on travis #4605

merged 7 commits into from Oct 22, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Sep 5, 2018

Our coverage is good, and I would love to maintain it like that. The coverage report is 0% on management commands (not sure if we care to have tests for that)

What change? Almost nothing, devs can still run normal tests, as usual. The only thing is that Travis will fail if the coverage goes down 79% (our current coverage).

We can increase the coverage later, but keeping it >=79% is a good first step.

If you want to check the coverage locally, you'll need to run tox like this tox -e py36,coverage

@stsewd stsewd requested a review from Sep 5, 2018
tox.ini Outdated
whitelist_externals = echo
commands =
py.test --disable-pytest-warnings \
--cov-report=term --cov-report=html --cov-config {toxinidir}/.coveragerc --cov=. {posargs}
coverage report --show-missing --fail-under=79
Copy link
Member Author

@stsewd stsewd Sep 5, 2018

Choose a reason for hiding this comment

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

79 is our current percent

Loading

Copy link
Member Author

@stsewd stsewd Sep 5, 2018

Choose a reason for hiding this comment

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

Well, looks like it is on 80% on travis, this is for the search tests I guess

Loading

Copy link
Member

@humitos humitos left a comment

I like this.

On the other hand, going so strict could bring problems, as usual. Maybe we can see how it goes and downgrade the limit later.

I think it will be better if we execute the coverage command as a separate job under travis. That way, we can immediately know if it fails because of the coverage or because of a test.

Also,

  • I think we can exclude the rtd_tests and tests/ and tests.py files so the output is not that verbose.

    • why there are test files saying that some lines are missing? aren't those tests being executed? why? are they marked as @Skip
  • analytics/vendor can be excluded also. It's not our own code

  • */management/commands can be excluded

  • settings can be excluded also

  • wsgi.py also

I love this PR, but I'd like to request these changes to make it more accurate.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 5, 2018

why there are test files saying that some lines are missing? aren't those tests being executed? why? are they marked as @Skip

Those aren't being executed at the moment but are unskiped in other PRs.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 5, 2018

I think it will be better if we execute the coverage command as a separate job under travis. That way, we can immediately know if it fails because of the coverage or because of a test.

Not sure about that, It will slow down the CI even more :/

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Sep 5, 2018

Also, we could easily see the tox report to see which command fail, tests or coverage.

Loading

tox.ini Outdated
whitelist_externals = echo
commands =
py.test --disable-pytest-warnings \
--cov-report=term --cov-report=html --cov-config {toxinidir}/.coveragerc --cov=. {posargs}
coverage report --show-missing --fail-under=76
Copy link
Member Author

@stsewd stsewd Sep 5, 2018

Choose a reason for hiding this comment

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

And with the search tests we are at 78%, should I increase the coverage?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 17, 2018

I'm not convinced this is the best path forward with coverage. I don't think we really want to fail tests on it, since that will rarely happen, but instead make the lack of coverage more visible.

I like what Sphinx does on their PR's -- sphinx-doc/sphinx#5549 (comment) -- I wonder if it would be more worthwhile to integrate something like that, so that we have better data around coverage.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 17, 2018

Yeah, integrating with codecov looks good, and it uses coverage so we can integrate that easily here. I'll update this PR to integrate with codecov

Loading

@humitos
Copy link
Member

@humitos humitos commented Oct 17, 2018

codecov is good, but it's too verbose. Is this comment added on every commit to the PR? If so, I think we will end up with many comments from this bot on PRs that will be impossible to follow.

I think we should tweak codecov to behave in a way that doesn't make hard to read all the comments of the PR. Maybe only the line

Merging #5549 into 1.8 will decrease coverage by <.01%.

with the link to the codecov.io page is enough.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 17, 2018

@humitos what I saw, it just post one comment and then updates that comment on each commit, and yeah, it's very verbose, should be a way of a more simple report.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 19, 2018

Here we go https://codecov.io/gh/rtfd/readthedocs.org/ the request for the bot is sent, I guess someone with admin access needs to accept the invitation 🤷‍♂️

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 19, 2018

People at home can still see the reports, with tox -e py36 and tox -e coverage or just tox -e py36,coverage

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 19, 2018

Also, I think we are good with the defaults for the configuration, for the comment we only have this

comment:
  layout: "header, diff"

https://docs.codecov.io/docs/codecov-yaml#section-default-yaml

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 19, 2018

I poked around on codecov but didn't see what confirmation I needed. Is it to install the bot on GH? Seems like that is optional, so I'm not 100% sure what's required.

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 19, 2018

Ah, found the request in email -- approved. 👍

Loading

Copy link
Member

@ericholscher ericholscher left a comment

I like this approach better. Haven't seen the output from the bot, but this looks like a good way to do it.

I'm a little worried that coverage will slow down our test runs, but we can cross that bridge if we hit it.

Loading

@codecov
Copy link

@codecov codecov bot commented Oct 19, 2018

Codecov Report

No coverage uploaded for pull request base (master@df85fef). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #4605   +/-   ##
========================================
  Coverage          ?   75.3%           
========================================
  Files             ?     158           
  Lines             ?   10045           
  Branches          ?    1266           
========================================
  Hits              ?    7564           
  Misses            ?    2145           
  Partials          ?     336

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 19, 2018

I retriggered the job, looks like we need to set up a less verbose comment for the bot p:

I'm a little worried that coverage will slow down our test runs

I didn't see any big difference locally, I think we are good.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented Oct 19, 2018

The bot is ready, @humitos are we ok with the bot's comment? If so, I'll merge this PR.

Loading

Copy link
Member

@humitos humitos left a comment

Looks good to me!

We can tweak the bot config if we found that it's still too verbose. For now, it looks good, though.

The header layout was removed from the comment but it's still there 😕

Loading

Copy link
Member

@humitos humitos left a comment

Changes look good.

The header layout was removed from the comment but it's still there, though.

We can tweak the configuration later if we find this too annoying.

Loading

Copy link
Member

@humitos humitos left a comment

Changes look good.

The header layout was removed from the comment but it's still there, though.

We can tweak the configuration later if we find this too annoying.

Loading

Copy link
Member

@humitos humitos left a comment

Changes look good.

The header layout was removed from the comment but it's still there, though.

We can tweak the configuration later if we find this too annoying.

Loading

Copy link
Member

@humitos humitos left a comment

Changes look good.

The header layout was removed from the comment but it's still there, though.

We can tweak the configuration later if we find this too annoying.

Loading

@humitos
Copy link
Member

@humitos humitos commented Oct 22, 2018

Haha! If it's not clear, I've trying to approve this PR --GitHub was working terrible :)

Loading

@stsewd stsewd merged commit a4291d1 into readthedocs:master Oct 22, 2018
3 checks passed
Loading
@stsewd stsewd deleted the run-coverage branch Oct 22, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 22, 2018

🎉

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants