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

Move cryptography-based imports into fixture #10686

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Nov 26, 2021

Move the imports that require cryptography into the cert_factory
fixture. This makes it possible to skip these tests on systems where
cryptography cannot be installed due to its dependency on Rust.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 26, 2021

Hiya @mgorny!

Thanks for filing this PR. Could you clarify why you think this should be accepted upstream?


@pypa/pip-committers What do y'all think that we should do with this PR?

In case you're not up to speed, this is basically the fallout of cryptography adding an unconditional build-time dependency on Rust. https://lwn.net/Articles/845535/ is a worthwhile read on this topic (although I'd avoid reading the comments) and, if you somehow have a lot of free time at hand, there's also pyca/cryptography#5771. Basically, the situation here is some of the fringe platforms that Gentoo supports can not run the Rust compiler and, IIUC, @mgorny is trying to run pip's test suite on one of these platforms and being unable to do so.

Personally, I think we should reject this PR, following the rationale that weird architectures weren't supported to begin with. I don't want to be involved in (or bothered about) the maintainance/support story for fringe platforms. It's upto the redistributors to figure out how to get things working on fringe platforms, and if things break (eg: like this), they get to keep both the pieces. I wanna double check though, that other @pypa/pip-committers don't feel differently.

@uranusjr
Copy link
Member

I’d be OK if this is done with a flag (e.g. pytest -m 'not ssl'), and skip by test kind (instead of by fixture availability, as done here). We only use cryptography for SSL-related functionalities, which is technically not a required Python module, so it makes sense to be able to test pip without SSL support.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 26, 2021

Fair enough. If someone wants to do this as part of improving our test suite to be run on Python compiled without the _ssl module, I'm OK with this. I'm personally not interested in reviewing PRs for that though.

There's three places that we have an unconditional import ssl in tests/.

@pfmoore
Copy link
Member

pfmoore commented Nov 26, 2021

On a general note, I do not want to see pip getting sucked into that particular flamewar. So what I'm saying here would apply regardless of the whole rust issue - if cryptography explicitly checked architecture in setup.py and refused to build on these architectures, I'd be saying the same thing.

As a pure Python package, pip runs anywhere that Python runs, so I'm not entirely convinced by the "it's not a supported platform" argument. However, we only test on mainstream platforms and architectures, so the test suite doesn't need to run everywhere that pip runs. And that's fine, in my view. The test suite is not intended to run everywhere (nor do we offer support for it, in the sense that we offer support for pip itself). After all, for a long time, the pip test suite didn't all work on Windows, and that didn't mean we didn't support Windows... The test suite is something to help the pip developers produce reliable software, it's not a service we provide to our users.

I don't think our test suite should automatically skip tests if cryptography is unavailable. The features being tested are core pip features, and should be tested. "I don't have the full test environment available" should cause the test suite to fail, not just skip some tests. I'm fine with a pytest mark being added, so that the user can opt into skipping the affected tests, but IMO it has to be opt-in. (And we wouldn't guarantee that we'd ensure the mark was applied to any new tests that may get added - people can make mistakes, and see my comment above about the test suite not being "supported" in that sense).

Beyond this, I'm not personally interested in the architectures affected here, so like @pradyunsg I won't be getting involved in reviewing or merging PRs.

@pfmoore
Copy link
Member

pfmoore commented Nov 26, 2021

Fair enough. If someone wants to do this as part of improving our test suite to be run on Python compiled without the _ssl module, I'm OK with this.

Cryptography is used to generate certs for testing SSL features, as far as I can see. It's not related to SSL support as such, and I don't think this should be tied to the more general question of availability of SSL. (Whether ssl is available is independent of whether cryptography is...)

Maybe if someone wants to not test pip's SSL features, they won't need cryptography either, so maybe if we had a "don't test SSL" option, we wouldn't need a "don't use cryptography" option as well. But maybe someone might want to test the bits of ssl they can test while not using locally generated certs? 🤷

@mgorny
Copy link
Contributor Author

mgorny commented Nov 26, 2021

For a start, pip already has some optional test dependencies and I don't really see why cryptography should be special here, independently of context. In the end, this doesn't add any real maintenance burden to the developers.

If you don't believe that the tests should be skipped when the dependency is not available, would you at least accept a patch moving the import inside the fixture, in order to make it possible to --deselect the specific tests?

@uranusjr
Copy link
Member

Maybe if someone wants to not test pip's SSL features, they won't need cryptography either, so maybe if we had a "don't test SSL" option, we wouldn't need a "don't use cryptography" option as well.

This is what I meant; since SSL is optional functionality, it is fine to offer an option to test this “pip without SSL functionalities” setup, and cryptography becoming optional test dependency is incidental.

In the end, this doesn't add any real maintenance burden to the developers.

This is not true, unfortunately. By making the dependency optional and automatically skips tests when it’s missing, we lose the guarantee that features covered by those tests are always tested, because those tests stop being always run. I listed the two requirements for this to be accepted above for this exact reason; this test-skipping logic must be opt-in, and have a clear scope on what tests are going to be skipped, so behaviour of the test suite continues to be predictable and useful.

@mgorny mgorny changed the title Skip cryptography-based tests when it is unavailable Move cryptography-based imports into fixture Nov 26, 2021
@mgorny
Copy link
Contributor Author

mgorny commented Nov 26, 2021

Ok, I have changed the patch not to skip the test but merely move the import. This should not affect normal testing while still making it possible to skip the tests via command-line without having to patch the sources.

Move the imports that require cryptography into the cert_factory
fixture.  This makes it possible to deselect these tests on systems
where cryptography cannot be installed.
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

@mgorny could you replace the import instructions with calls to pytest.importorskip() with an explicit reason specified? This way, the comments won't be just in code but visible in the test output as explicit skips.

@webknjaz
Copy link
Member

This is not true, unfortunately. By making the dependency optional and automatically skips tests when it’s missing, we lose the guarantee that features covered by those tests are always tested, because those tests stop being always run.

I'd argue that if codecov/coveragepy is set up properly and measured the test coverage, it should spot the problem: https://nedbatchelder.com/blog/201908/dont_omit_tests_from_coverage.html.

@mgorny
Copy link
Contributor Author

mgorny commented Nov 29, 2021

@mgorny could you replace the import instructions with calls to pytest.importorskip() with an explicit reason specified? This way, the comments won't be just in code but visible in the test output as explicit skips.

I can do that but that's roughly what I originally had and @pradyunsg asked me the exact opposite, so you can imagine how confused I'm right now.

@pfmoore
Copy link
Member

pfmoore commented Nov 29, 2021

@webknjaz We explicitly don't want the tests to work (even with a skip message) if cryptography is unavailable. If the user explicitly opts out, that's fine, but by default all the tests should run.

Making coverage a mandatory CI check (which is how I assume you expect coverage would spot the issue) is a whole other issue, and much harder (a lot of our tests run pip in a subprocess, collecting coverage data from such a subprocess is non-trivial). If anyone wants to tackle coverage checking, it should be a separate issue/PR.

@webknjaz
Copy link
Member

Ah, makes sense then.

@pradyunsg pradyunsg merged commit 8d4fb05 into pypa:main Dec 3, 2021
@mgorny
Copy link
Contributor Author

mgorny commented Dec 3, 2021

Thank you!

@mgorny mgorny deleted the missing-cryptography branch December 3, 2021 13:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants