You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
One of the specific pieces of this is certbot._internal.cli.set_by_cli.detector which bit us recently at #9567. We use this variable to cache the result of the fairly computationally expensive process of determining which CLI options were set by the user (since the user may have explicitly set things to their default value).
The variable being stored this way results in a lot of testing code creating mocks or manually (re)setting it (git grep set_by_cli certbot/tests | grep -e detector -e mock | wc -l prints 56 as of writing this). I think we should remove this unusual global/static variable in favor of a more locally scoped approach that automatically cleans up after itself and then delete the pytest fixture added in the recent PR.
I think there are many ways we can potentially do this:
Try setting detector on the config object we pass around everywhere.
If we mainly call set_by_cli in a loop in a couple particular places in our code, maybe we can change the API to compute this once per set of calls by storing the value of detector in a local variable somewhere.
Rewrite CLI parsing as described at Rewrite argument parsing #4493 which is also in some of our higher level planning docs.
Some higher level refactoring between the options (1)/(2) and rewriting all of CLI parsing.
Your idea?
I'm throwing this into our current milestone for now, because I personally feel like it's a good project that'd help with the testing cleanup work I started recently after getting complaints from many of the Certbot devs. If other Certbot devs disagree with this, please change it and/or let me know!
I think one of the reasons that Certbot is hard to test is due to somewhat hacky management of state. See things like:
certbot/certbot/certbot/tests/util.py
Lines 380 to 388 in a42cffc
One of the specific pieces of this is
certbot._internal.cli.set_by_cli.detectorwhich bit us recently at #9567. We use this variable to cache the result of the fairly computationally expensive process of determining which CLI options were set by the user (since the user may have explicitly set things to their default value).The variable being stored this way results in a lot of testing code creating mocks or manually (re)setting it (
git grep set_by_cli certbot/tests | grep -e detector -e mock | wc -lprints56as of writing this). I think we should remove this unusual global/static variable in favor of a more locally scoped approach that automatically cleans up after itself and then delete thepytestfixture added in the recent PR.I think there are many ways we can potentially do this:
detectoron theconfigobject we pass around everywhere.set_by_cliin a loop in a couple particular places in our code, maybe we can change the API to compute this once per set of calls by storing the value ofdetectorin a local variable somewhere.I'm throwing this into our current milestone for now, because I personally feel like it's a good project that'd help with the testing cleanup work I started recently after getting complaints from many of the Certbot devs. If other Certbot devs disagree with this, please change it and/or let me know!