Skip to content

Avoid changing global logging state in tests#1522

Open
matt-graham wants to merge 5 commits into
masterfrom
mmg/fix-logging-reset-in-tests
Open

Avoid changing global logging state in tests#1522
matt-graham wants to merge 5 commits into
masterfrom
mmg/fix-logging-reset-in-tests

Conversation

@matt-graham

@matt-graham matt-graham commented Nov 20, 2024

Copy link
Copy Markdown
Collaborator

Resolves #1521

Removes tlo.logging.core.reset function (which was only used in tests in tests/test_logging.py) as this resets logging state to initial state on first import of tlo.logging.core which we are unlikely want to do in practice, as this overwrites any changes to 'global' (tlo.logging.core module level) logging state resulting as side effects of subsequent module imports - for example the very common pattern of logger = getLogger(__name__).

A new context manager restore_global_state is instead provided to serve the purpose of allowing testing changes to the global logging state without this persisting across tests. This records the initial global logging state variables on entry in to the context manager and restores these to their original values on exit from the context manager. This also involves performing some resetting of attributes of new loggers created in context managed code, but we do not explicitly remove the corresponding logger objects in the global state of the base logging library as this is difficult to do robustly as the base loggers store references to each other and use placeholders for loggers higher in hierarchy which haven't been initialised at the point a child logger is initialised.

To confine accesses to the logging global state to the tlo.logging.core module, this PR also moves the get_custom_levels function from the tlo.logging.helpers module to tlo.logging.core, swaps the previous usage of _logging.root.manager.loggerDict to find all initialised tlo.methods loggers to instead use the global _loggers dictionary (which should contain instances of tlo* loggers). There was also an instance of iteration over a set in this function previously which has been changed to iteration over a list to avoid the ordering of iterations changing in each interpreter instance (unclear if this had any effect but thought I'd fix it just to be safe...).

@mnjowe

mnjowe commented Jan 16, 2025

Copy link
Copy Markdown
Collaborator

@tamuri can we get this PR in? I'm sure its all complete

@tamuri tamuri self-assigned this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One test failing in test_utils.py when running pytest tests

3 participants