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

Add a network pytest mark for tests that use the network #1669

Merged
merged 5 commits into from
Aug 13, 2021

Conversation

antlarr
Copy link
Contributor

@antlarr antlarr commented Jun 7, 2021

Sometimes it's useful to have the tests that use the network
marked so they can be skipped easily when we know the network
is not available.

This is useful for example on SUSE and openSUSE's build servers.
When building the httpx packages (actually, any package in the
distribution) the network is disabled so we can assure
reproducible builds (among other benefits). With this mark, it's
easier to skip tests that can not succeed.

Sometimes it's useful to have the tests that use the network
marked so they can be skipped easily when we know the network
is not available.

This is useful for example on SUSE and openSUSE's build servers.
When building the httpx packages (actually, any package in the
distribution) the network is disabled so we can assure
reproducible builds (among other benefits). With this mark, it's
easier to skip tests that can not succeed.
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

I'm personally okay with this, though perhaps let's make sure this doesn't just appear as a "utility marker", since you'd be relying on it for building on SUSE?

setup.cfg Outdated Show resolved Hide resolved
j178 and others added 2 commits June 16, 2021 14:01
Co-authored-by: Florimond Manca <15911462+florimondmanca@users.noreply.github.com>
@antlarr
Copy link
Contributor Author

antlarr commented Jun 16, 2021

I'm personally okay with this, though perhaps let's make sure this doesn't just appear as a "utility marker", since you'd be relying on it for building on SUSE?

That's right, we'll use it for building the httpx SUSE (and openSUSE) packages

@tomchristie
Copy link
Member

tomchristie commented Jun 17, 2021

Seems fair. I'd been keen on us switching these cases out to non-network-requiring tests at some point, tho not a massive priority right now.

@antlarr Could you comment here showing how to disable running the network tests once we've got this marker? Does it require a particular envvar to be set in order to skip them? Where is this marker (and making use of it) documented?

Once we've got that info against this history, then I'm super happy for this to go in, yup.

@s-t-e-v-e-n-k
Copy link

https://docs.pytest.org/en/6.2.x/example/markers.html is the upstream documentation for custom markers like this.

@antlarr
Copy link
Contributor Author

antlarr commented Jun 18, 2021

Seems fair. I'd been keen on us switching these cases out to non-network-requiring tests at some point, tho not a massive priority right now.

@antlarr Could you comment here showing how to disable running the network tests once we've got this marker?
Does it require a particular envvar to be set in order to skip them?

Sure. We just use it in our build service by running the tests like this:

%pytest -k 'not network'

Where %pytest is a rpm macro that basically runs pytest with the given arguments for every python flavor (currently 3.6, 3.8 and 3.9) in order to test the rpm packages that are being built for each flavor.

You can see this at the python-httpx spec file in openSUSE build service in line 84.

Where is this marker (and making use of it) documented?

@s-t-e-v-e-n-k already linked the pytest documentation for markers. I also added a comment at 5f7ee8e to document what that particular marker is used for, just in case some other test is added in the future that makes use of the network.

Once we've got that info against this history, then I'm super happy for this to go in, yup.

Cool

@florimondmanca
Copy link
Member

florimondmanca commented Jun 24, 2021

@tomchristie With a small tweak to scripts/test (passing "$@" to the coverage command, I believe) we could also do this:

scripts/test -m "not network"

(We used to have this, but it got knocked out last year by #1353.)

@florimondmanca florimondmanca added the tooling Changes to our CI/CD, tests setup, etc. label Jun 24, 2021
@tomchristie
Copy link
Member

@florimondmanca Gotcha thanks. We can potentially follow up with that, but either ways let's get this in...

@tomchristie tomchristie merged commit 77193b2 into encode:master Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Changes to our CI/CD, tests setup, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants