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: use run.include, remove --ignore-errors, send TOXENV as name to codecov #4841

Merged
merged 5 commits into from
Feb 27, 2019

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 26, 2019

This appears to improve performance - ~4s with tox -e py37-coverage -- testing/test_collection.py.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

With tox -e py37-coverage:

  • master (9dcd6f2): 232.63s user 94.48s system 87% cpu 6:13.17 total
  • this PR: 219.65s user 91.22s system 86% cpu 5:57.47 total

The .coverage file itself is also a bit smaller (old: 298K, new: 220K).

@blueyed blueyed changed the title [WIP] coverage: use source=. and report.include coverage: use source=. and report.include Feb 27, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

Re timing: currently again at "234.82s user 92.08s system 88% cpu 6:11.41 total", 223K.
With (".coveragerc: fix/tighten paths").

This appears to improve performance - ~4s with `tox -e py37-coverage --
testing/test_collection.py`.
This should not be necessary (anymore).
@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

@pytest-dev pytest-dev deleted a comment from codecov bot Feb 27, 2019
@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #4841 into master will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4841      +/-   ##
==========================================
- Coverage   94.44%   94.32%   -0.13%     
==========================================
  Files         113      113              
  Lines       25163    25163              
  Branches     2498     2498              
==========================================
- Hits        23765    23734      -31     
- Misses       1066     1095      +29     
- Partials      332      334       +2
Flag Coverage Δ
#linux 91.72% <ø> (-0.13%) ⬇️
#windows 93.46% <ø> (-0.18%) ⬇️
Impacted Files Coverage Δ
src/_pytest/helpconfig.py 85.58% <0%> (-11.72%) ⬇️
src/_pytest/capture.py 89.81% <0%> (-3.85%) ⬇️
src/_pytest/debugging.py 57.23% <0%> (-3.15%) ⬇️
src/_pytest/config/__init__.py 92.16% <0%> (-1.64%) ⬇️
src/_pytest/warnings.py 87.65% <0%> (-1.24%) ⬇️
src/_pytest/assertion/rewrite.py 95.31% <0%> (-0.17%) ⬇️
src/_pytest/terminal.py 91.74% <0%> (+2.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1254c4...ee95d66. Read the comment docs.

@blueyed blueyed changed the title coverage: use source=. and report.include coverage: use run.include, remove --ignore-errors, send TOXENV as name to codecov Feb 27, 2019
@blueyed blueyed changed the title coverage: use run.include, remove --ignore-errors, send TOXENV as name to codecov WIP: coverage: use run.include, remove --ignore-errors, send TOXENV as name to codecov Feb 27, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

src/pytest went missing: https://codecov.io/gh/pytest-dev/pytest/tree/ecddf69524e7917bc9b87d18c7d178ffa8a12b02

@nicoddemus
btw: have you changed the required checks? At least e.g. the WIP should be in there to prevent accidental merging. I would also add Travis, Azure, and codecov/patch.

@nicoddemus
Copy link
Member

btw: have you changed the required checks? At least e.g. the WIP should be in there to prevent accidental merging. I would also add Travis, Azure, and codecov/patch.

I did not change anything, it has never been enabled actually. The only check we have in place is to require reviewer approval.

I will enable WIP; I will hold on enabling the others because we have merged PRs that had one or more checks failing because the change was unrelated to that PR (for example a docs PR to master and master itself is broken for other reasons). On those cases having to ask the original developer to rebase after the issue is fixed is kind of a pain.

@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

On those cases having to ask the original developer to rebase after the issue is fixed is kind of a pain.

Admins (we) could always override it (this is also a setting).

@blueyed blueyed changed the title WIP: coverage: use run.include, remove --ignore-errors, send TOXENV as name to codecov coverage: use run.include, remove --ignore-errors, send TOXENV as name to codecov Feb 27, 2019
@blueyed blueyed merged commit e711a6c into pytest-dev:master Feb 27, 2019
@blueyed
Copy link
Contributor Author

blueyed commented Feb 27, 2019

Merging, https://codecov.io/gh/pytest-dev/pytest/tree/ee95d666f8c875a7b4d46b2f4e0ce08cb05a0007 says +1.15%.

@blueyed blueyed deleted the coverage-source branch February 27, 2019 12:16
@nicoddemus
Copy link
Member

Admins (we) could always override it (this is also a setting).

Yep, but @RonnyPfannschmidt has asked us to disable that rule even for admins. I'm OK with letting admins override that rule, the rule is mostly to prevent mistakes, but we are already well versed with the workflow.

@blueyed
Copy link
Contributor Author

blueyed commented Mar 1, 2019

Admins (we) could always override it (this is also a setting).

Yep, but @RonnyPfannschmidt has asked us to disable that rule even for
admins. I'm OK with letting admins override that rule, the rule is mostly to
prevent mistakes, but we are already well versed with the workflow.

@RonnyPfannschmidt
Are you still wanting this?

I think it is better to e.g. clearly have an indicator if Travis/Azure fails,
but allow us to merge nonetheless.

I agree that it would be better to still have the "Require pull request reviews
before merging" enforced though.
It is a pity that it is not possible to only override status checks currently.

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.

2 participants