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

Disable coverage on PyPy #3112

Merged
merged 5 commits into from
Feb 18, 2022
Merged

Conversation

abravalheri
Copy link
Contributor

Currently, CI jobs running on PyPy are (painfully) slow.
This seems to be a well know side effect of enabling coverage.

Summary of changes

The change proposed here is to disable coverage on PyPy for both Windows and MacOS (which seem to be very slow).

Coverage for PyPy on Ubuntu is preserved because, while being slower than CPython, it is still OK-ish.
Hopefully tests running on PyPy/Ubuntu will include most of PyPy's specific corner cases.

Pull Request Checklist

Currently, CI jobs running on PyPy are (painfully) slow.
This seems to be a well know side effect of enabling coverage.

The change proposed here is to disable coverage on PyPy for both Windows
and MacOS (which seem to be very slow).

Coverage for PyPy on Ubuntu is preserved because, while
being slower than CPython, it is still OK-ish.
Hopefully tests running on PyPy/Ubuntu will include most of
PyPy's specific corner cases.
@jaraco
Copy link
Member

jaraco commented Feb 15, 2022

Oh interesting. I thought this had been done previously (for all of PyPy). Perhaps that got lost.

@jaraco
Copy link
Member

jaraco commented Feb 15, 2022

Currently, pytest-black and pytest-mypy are disabled for local development and CI in the setup.cfg. Perhaps it would make sense to exclude the dependency there as well, for consistency and to save PyPy users the same pain.

Instead of disabling coverage in the GitHub workflow
it might be easier to just not install it in the test environment.
@abravalheri abravalheri marked this pull request as ready for review February 15, 2022 23:51
@abravalheri
Copy link
Contributor Author

abravalheri commented Feb 15, 2022

I updated the PR to use a conditional test dependency on setup.cfg instead of tackling the problem in the GitHub workflow file.

Thank you very much for the tip!

(Let's see if it really improves things)

@jaraco
Copy link
Member

jaraco commented Feb 16, 2022

Oh, shoot. That didn't work - because not having the plugin makes the invocation invalid. You may be able to move the --cov-report xml to pyproject.toml such that it's only present when the plugin is, but that will make the xml option used unconditionally. I'm not sure what that option does - if it's used for something or if it's just some option that someone wanted, so I'm unsure if it's desirable to have outside CI.

@abravalheri
Copy link
Contributor Author

I'm not sure what that option does - if it's used for something or if it's just some option that someone wanted, so I'm unsure if it's desirable to have outside CI.

I suspect the original intention was to enable the codecov service (it needs the XML file). I don't see it being used outside of the CI.

I think an extra coverage xml command can be added to produce the required xml file... I will try that.

@blink1073
Copy link
Contributor

blink1073 commented Feb 16, 2022

I'd try disabling the xdist plugin on the the pypy windows build (-p no:xdist). I'd also suggest using pytest-timeout with --timeout=1000 --timeout_method=thread to handle individual tests that hang. It will give a traceback with the spot it was at when timing out.

@abravalheri
Copy link
Contributor Author

I'd try disabling the xdist plugin on the the pypy windows build (-p no:xdist). I'd also suggest using pytest-timeout with --timeout=1000 --timeout_method=thread to handle individual tests that hang. It will give a traceback with the spot it was at when timing out.

Yeah pypy+windows is consistently annoying 😂
I thought I got rid of this problem in #3099, but apparently there might be another a BrokenProcessPool exception even before the timeout is reached...
I think I will try to handle this other error first in the test directly before resorting to the plugin. Editing the workflow file to change specific job always feels a bit like wresting GitHub.

@abravalheri
Copy link
Contributor Author

Ok, I activated codecov again to see if it is compatible and it seems to be OK.
That difference in coverage is around 12% because the comparison is done with the last coverage upload done to codecov (more than 200 commits ago).

According to the codecov team there is not really a way to get around this, except if we make the job only informational (in that case it will always show up as "green").

@blink1073
Copy link
Contributor

I think it is a losing battle to try and run coverage on pypy: we used conditional invocations here

@abravalheri
Copy link
Contributor Author

Thanks for the example @blink1073, this PR is effectively disabling coverage on PyPY :)

Codecov is trying to compare the coverage on the other jobs (CPython) with an old report they have from more than 6 months ago, which causes the failure :(

@blink1073
Copy link
Contributor

Ah, here is how it was disabled previously using conftest.py: https://github.com/pypa/setuptools/pull/2254/files

@blink1073
Copy link
Contributor

That approach was reverted in f01d271

@abravalheri
Copy link
Contributor Author

I see! Thank you very much @blink1073 for investigating this.

So f01d271 did implement more or less the same approach of this PR, but that change seems to have been reverted last time jaraco/skeleton was collapsed.

@blink1073
Copy link
Contributor

I am trying without xdist on my fork, but we've passed with xdist in this repo before.

Another thing we often have to do on windows is avoid capture with -s

@blink1073
Copy link
Contributor

Yeah, still failing with no xdist: setuptools\tests\test_build_ext.py ....F [ 27%].

Trying with no capture now.

@blink1073
Copy link
Contributor

No luck: setuptools\tests\test_build_ext.py ....F [ 27%]

@blink1073
Copy link
Contributor

Ha, and now they pass here as well. I think if we merge this, and then follow up by removing the informational part, we should be good.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jaraco jaraco merged commit 1181440 into pypa:main Feb 18, 2022
@abravalheri abravalheri deleted the disable-coverage-pypy branch February 18, 2022 00:33
abravalheri added a commit to abravalheri/setuptools that referenced this pull request Feb 18, 2022
Currently the test suite is problematic on PyPy.
There is some advice about disabling `xdist` in
pypa#3112 (comment)
to try helping with this issue.

This is just an experiment to see how disabling `xdist` would affect the
total testing time.
@abravalheri abravalheri mentioned this pull request Feb 18, 2022
2 tasks
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.

None yet

3 participants