Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly reset log level when temporarily changing it #6327

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Aug 23, 2024

Fixes tests that were failing on my cylc remove branch due to a change in order

Context

Running these 2 tests in this order would guarantee test_Timer would fail.

pytest -n 0 \
  tests/integration/scripts/test_show.py::test_workflow_meta_query \
  tests/unit/test_timer.py::test_Timer

This is due to the way we were temporarily changing the Cylc logger level during Scheduler.start()

@contextmanager
def patch_log_level(logger: logging.Logger, level: int = logging.INFO):
"""Temporarily patch the logging level of a logger if the specified level
is less severe than the current level.
Defaults to INFO.
"""
orig_level = logger.getEffectiveLevel()
if level < orig_level:
logger.setLevel(level)
yield
logger.setLevel(orig_level)

Logger.getEffectiveLevel() will return the root logger level if Logger.level is NOTSET.

In pytest the root logger level is WARNING, so in the tests, there were cases where the Cylc logger was being incorrectly reset to WARNING instead of NOTSET

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • Tests are included
  • No changelog entry needed as not affecting users
  • No docs needed
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

`Logger.getEffectiveLevel()` will return the root logger level if `Logger.level` is `NOTSET`.
In pytest the root logger level is `WARNING`, so in the tests, there were cases where the Cylc logger was being incorrectly reset to `WARNING` instead of `NOTSET`
@MetRonnie MetRonnie added small could be better Not exactly a bug, but not ideal. labels Aug 23, 2024
@MetRonnie MetRonnie added this to the 8.3.4 milestone Aug 23, 2024
Comment on lines -93 to -94
if logger.getEffectiveLevel != logging.INFO:
logger.setLevel(logging.INFO)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointless comparison of <function logger.getEffectiveLevel> != <int>

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - one question about whether both tests are necessary should not stand in the way of merger.

@oliver-sanders
Copy link
Member

merger-ing

@oliver-sanders oliver-sanders merged commit 6926aac into cylc:8.3.x Aug 29, 2024
27 checks passed
@MetRonnie MetRonnie deleted the logging branch August 29, 2024 10:57
@MetRonnie MetRonnie mentioned this pull request Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal. small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants