Move cryptography-based imports into fixture

[mgorny]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[webknjaz]

@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]

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]

@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]

@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]

Ah, makes sense then.

[mgorny]

Thank you!