CI: consolidate the build scripts and github actions tests workflow#1241
Conversation
caspervdw
left a comment
There was a problem hiding this comment.
Some minor nitpicks, for the rest there is one test failing on python 3.10, something with a deprecation warning on setting attributes. OK if you want to postpone solving that. Maybe good to decorate those with shapely20_todo so that we can start working with a green CI.
.github/workflows/tests.yml
Outdated
| run: | | ||
| python -c "from shapely import geos; print(geos.geos_version_string)" | ||
| python -m pytest --cov shapely --cov-report term-missing | ||
| pytest shapely/tests -cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} |
There was a problem hiding this comment.
| pytest shapely/tests -cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} | |
| pytest tests shapely/tests -cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} |
And drop the next line.
|
|
||
|
|
||
| # TODO(shapely-2.0) | ||
| @pytest.mark.xfail(strict=False, reason="Not yet implemented for Shapely 2.0") |
There was a problem hiding this comment.
Maybe use shapely20_todo here?
There was a problem hiding this comment.
Yeah, I initially did, but that one is currently a "strict" xfail, and this test also seemed to pass in some builds (it might depend on the GEOS version, in any case something to verify and clean-up in a follow-up).
I could also make the general marker less strict, but the strict version is also nice because then you get notified if something starts passing.
Co-authored-by: Casper van der Wel <caspervdw@gmail.com>
shapely/geometry/polygon.py
Outdated
| return [] | ||
| return InteriorRingSequence(self) | ||
|
|
||
| def __eq__(self, other): |
There was a problem hiding this comment.
This was an oversight in #1239 where I removed the other __eq__ methods
.github/workflows/tests.yml
Outdated
| run: | | ||
| python -c "from shapely import geos; print(geos.geos_version_string)" | ||
| python -m pytest --cov shapely --cov-report term-missing | ||
| pytest shapely/tests tests -cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} |
There was a problem hiding this comment.
| pytest shapely/tests tests -cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} | |
| pytest shapely/tests tests --cov shapely --cov-report term-missing ${{ matrix.extra_pytest_args }} |
maybe you can also include the coverage stuff in the “extra_pytest_args” so that the coverage report is only generated on a specific run?
There was a problem hiding this comment.
Good catch, thanks :)
maybe you can also include the coverage stuff in the “extra_pytest_args” so that the coverage report is only generated on a specific run?
I think it can actually be useful to run coverage on multiple builds (and the coverage report should then get combined), to run cover code that is version specific as well (although most of the GEOS-version specific code lives in C, and that's not covered by this anyway)
Co-authored-by: Casper van der Wel <caspervdw@gmail.com>
|
There is one issue with coverage also reporting test coverage on the tests in shapely/tests themselves. According to the docs , pytest-cov needs this in the |
|
Btw I am mostly not using coverage in my projects, because I think it gives a false sense of confidence. So I am actually 👍 on dropping it all together |
|
Yeah, I personally also don't use coverage that much (it's mostly because it was present, that I kept it). I think as long as you are aware of the limitations and don't use it as indication that "all is good", it can have some use (if something is not covered, that's still useful information) |
|
Test workflows are all passing here, so going to merge this so have proper CI running again. |
|
Good! Are you planning to look into the wheelbuild workflows? I could do that maybe in the coming days. |
Didn't plan yet, so feel free to do so! (I think it should be only a minor edit, for example at least updating the test invocation to run shapely tests, but maybe not much more, since the GEOS build scripts are already updated in this PR) |
xref #962
This PR consolidates the github actions workflow for tests + the build scripts required for that, and gets the tests "passing":
TODO(shapely-2.0)comment), to get the tests CI green here (we can fix those in targeted follow-ups, but for those follow-ups a green CI will be useful)