Skip to content

Remove global/static certbot._internal.cli.set_by_cli.detector #9598

@bmw

Description

@bmw

I think one of the reasons that Certbot is hard to test is due to somewhat hacky management of state. See things like:

# Cleanup opened resources after a test. This is usually done through atexit handlers in
# Certbot, but during tests, atexit will not run registered functions before tearDown is
# called and instead will run them right before the entire test process exits.
# It is a problem on Windows, that does not accept to clean resources before closing them.
logging.shutdown()
# Remove logging handlers that have been closed so they won't be
# accidentally used in future tests.
logging.getLogger().handlers = []
util._release_locks() # pylint: disable=protected-access

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:

  1. Try setting detector on the config object we pass around everywhere.
  2. 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.
  3. Rewrite CLI parsing as described at Rewrite argument parsing #4493 which is also in some of our higher level planning docs.
  4. Some higher level refactoring between the options (1)/(2) and rewriting all of CLI parsing.
  5. 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!

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions