Conversation
|
If we are going to rely on certbot/certbot/tests/main_test.py Lines 2472 to 2473 in 5fdee74 Personally I am still in the habit of occasionally running tests directly via |
|
Ooo nice catch! I agree we should do something here. Do you mind if we do it in another PR though so we can get tests on Also, do you have a preference whether we just delete those lines so |
|
The pytest documentation does seem to offer it as a legitimate option: https://docs.pytest.org/en/7.1.x/how-to/usage.html#calling-pytest-from-python-code, but I'd be fine with deleting it too. Either way we get rid of the half-way state of "it runs but things are randomly broken". |
|
Cool. I'll play with it. I saw those docs too, but noticed that they don't pass EDIT: It's mimicking unittest's behavior that seems a bit unconventional to me. |
I now believe tests started failing after #9355 was merged because the annoying and sneaky global/static variable
certbot._internal.cli.set_by_cli.detectorwas being reset as part of _call at the beginning of each test and was not (also) being reset after each test. The tests introduced in that PR was not the only or first use of this pattern inmain_test.py. I think this pattern meant that depending on the exact test order (which I believe will vary by machine/run a bit due to our use of--numprocesses autoto spread tests across processes), the value ofcertbot._internal.cli.set_by_cli.detectormay still have been set to something when other tests ran affecting their result.In particular, building off my explanation at the beginning of #9564,
set_by_cli('authenticator')was returningTrueclaiming the value was set to Apache, however, Apache wasn't actually requested on the command line in the failing test. I believe this caused this code to leaveconfig.authenticatorunset which then caused us to blow up later because we didn't know what plugin to use.This PR fixes this by making use of a global pytest fixture that resets
cli.set_by_cli.detectortoNonebetween every test. I initially just put this inmain_test.pyas its the only place we currently do this in our tests, but I personally think it's useful to do it more globally to avoid hitting this in the future in places like the unit tests for theclimodule or other modules that use it such asclientorrenewal.This PR introduces the first import of
pytestin our shipped code although I think/hope there will be many to come and I don't think it's a problem because the Certbot test extras depend on pytest, we claimed this dependency years ago, and instruct packagers to run our tests using pytest.The last thing I want to say in my wall of text is I think us hitting this problem is another good reason we should prioritize cleaning up our testing and/or CLI parsing.