TST: add configuration for no-network tests #2371
Conversation
.github/workflows/ci-testing.yml
Outdated
| @@ -13,10 +13,16 @@ jobs: | |||
| os: [ubuntu-latest, macos-latest, windows-latest] | |||
| python-version: [3.9, '3.10', '3.11', '3.12'] | |||
| shapely-dev: [false] | |||
| no-network: [false] | |||
There was a problem hiding this comment.
The double negative is sometimes confusing in the conditionals. What about "use-network" and invert the logic, or "disable-network" instead?
| no-network: [false] | |
| disable-network: [false] |
There was a problem hiding this comment.
I like "use-network" as it puts the more complicate if statement in the special case step.
.github/workflows/ci-testing.yml
Outdated
| include: | ||
| - os: ubuntu-latest | ||
| python-version: '3.11' | ||
| shapely-dev: true | ||
| no-network: false |
There was a problem hiding this comment.
Do these pick up the defaults from above, or do we have to manually add them to each option?
| no-network: false |
There was a problem hiding this comment.
They do pick up the default. The only difference was that without adding these explicitly both were marked in the test output as "ubuntu-latest, 3.11, false". So having it like this made it easier for me to find the one I was looking for. Future us cannot be expected to remember what the order of these booleans mean though, so will revert that.
There was a problem hiding this comment.
I'm also wondering if we still need the shapely-dev one, as it looks like it was added in the run up to the Shapely 2.0 release to make sure we were ready for that: #2080
There was a problem hiding this comment.
Yeah, that is a good point, it might make sense to keep it though and turn it into a monthly pre-release run to get all of Shapely+Numpy+Matplotlib nightlies instead and just run with everyone's updates once every so often rather than with everyone's PRs.
Maybe leave it as a follow-up PR cleaning some of this other stuff up so it doesn't blow up the scope of this one.
There was a problem hiding this comment.
Actually it turns out these flags default to false, so I do need use-network: true in shapely-dev.
| @@ -82,6 +89,14 @@ jobs: | |||
| --mpl-results-path="cartopy_test_output-${{ matrix.os }}-${{ matrix.python-version }}-${{ matrix.shapely-dev }}" \ | |||
| --pyargs cartopy ${EXTRA_TEST_ARGS} | |||
|
|
|||
| - name: No Network Tests | |||
There was a problem hiding this comment.
Maybe add a quick comment like "The test suite should pass without network access" so we no why we are adding this?
|
I think I've addressed the review comments on this one. Deliberately breaking some tests still gives consistent failures. |
Rationale
Following on from #2368, this PR adds a new test configuration using pytest-socket to ensure that any tests that need to download something have been marked with either the
natural_earthornetworkmarker.I tried using the existing
Testingstep and put the relevant args in the $EXTRA_TEST_ARGS variable following the pattern used in theCoverage packagesstep, but couldn't get it to work: it worked as expected if I set the environment variable to-m networkbut when I tried either-m "not network"or-m 'not network'zero tests were discovered, so I guess it's something to do with the quote marks? My experience with github actions is negligible so I may be missing something obvious.To demonstrate how the failures will look, I made a branch off this branch and added commit rcomer@21f7944. The CI from that is here.
Implications