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

Handle logging.disable() in caplog.set_level and caplog.at_level #8711

Closed
alexlambson opened this issue May 28, 2021 · 6 comments · Fixed by #8758 · May be fixed by alexlambson/pytest#1
Closed

Handle logging.disable() in caplog.set_level and caplog.at_level #8711

alexlambson opened this issue May 28, 2021 · 6 comments · Fixed by #8758 · May be fixed by alexlambson/pytest#1
Labels
plugin: logging related to the logging builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@alexlambson
Copy link
Contributor

alexlambson commented May 28, 2021

What's the problem this feature will solve?

Currently, caplog will not catch logs if logging.disable(LEVEL) was called somewhere, in my case it's called in my django
settings for test runs.

Describe the solution you'd like

caplog.at_level and caplog.set_level should both take a new arg called force_enable that tells caplog to set the disable setting to one level below whatever level was requested to caplog.
Or it should force enable the logging level if rootLogger.manager.disable >= level so that logging will always
be enabled for the level a test is trying to capture.

This will allow caplog to capture logs at the desired level even if they were disabled.

e.g.

# settings/testing.py
# disables all logs
import logging
logging.disable(logging.CRITICAL)


# test_file.py
import logging


def test_stuff(caplog):
    caplog.set_level(logging.WARNING)
    logging.error("Sierra 117")
    assert len(caplog.records) == 1
    assert "117" in caplog.text

The test will currently fail because caplog doesn't override logging.disable which is a separate setting from log level.

My proposal is to add a call to logging.disable(max(level-10, 0)) inside both set_level and at_level. The -10 means disable everything below the requested level for caplog. We set a minimum of 0 because that's the lowest log level.

This can be reset by adding an attribute to keep track of the initial disabled level, similar to the one added to fix #7133.

Alternative Solutions

This is my current workaround in one of my tests

with caplog.at_level(logging.WARNING):
    # logging.disable needs to be called because caplog doesn't handle disabled logging.
    logging.disable(logging.WARNING)
    # inside this request a `logging.error()` is called.
    response = ah_client.get("/api/redacted/")
    # re-disable all logs.
    logging.disable(logging.CRITICAL)

Additional context

I have an example PR that I can transition to a full PR if the team decides they want this behavior
alexlambson#1

alexlambson pushed a commit to alexlambson/pytest that referenced this issue May 28, 2021
…ytest-dev#8711

Forces requested `caplog` logging levels to be enabled if they were disabled via `logging.disable()`

`[attr-defined]` mypy error ignored in `logging.py` because there were existing errors with the imports
and `loggin.Logger.manager` is an attr set at runtime. Since it's in the standard lib I can't really fix that.

Ignored an attr-defined error in `src/_pytest/config/__init__.py` because the re-export is necessary.

Issue: pytest-dev#8711
@Zac-HD Zac-HD added plugin: logging related to the logging builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature labels May 31, 2021
@alexlambson
Copy link
Contributor Author

Should I open a pull request and you guys can decide if you want this behavior?

@RonnyPfannschmidt
Copy link
Member

hmm, at first glance this seems like good to have feature

CC @nicoddemus

@alexlambson
Copy link
Contributor Author

If we're worried about magic, I could switch it to being an optional arg so people have to explicitly ask for this.

@bluetech
Copy link
Member

@alexlambson I suggest you submit the PR -- since you've already prepared it, it's always nicer to discuss with working code.

@alexlambson
Copy link
Contributor Author

Opened #8758.

@alexlambson
Copy link
Contributor Author

Just bumping / following up on this because it's popped up for me again in a project.

alexlambson pushed a commit to alexlambson/pytest that referenced this issue Nov 6, 2021
…ytest-dev#8711

Forces requested `caplog` logging levels to be enabled if they were disabled via `logging.disable()`

`[attr-defined]` mypy error ignored in `logging.py` because there were existing errors with the imports
and `loggin.Logger.manager` is an attr set at runtime. Since it's in the standard lib I can't really fix that.

Ignored an attr-defined error in `src/_pytest/config/__init__.py` because the re-export is necessary.

Issue: pytest-dev#8711
alexlambson added a commit to alexlambson/pytest that referenced this issue Nov 6, 2021
- Resolves issues from review provided by https://github.com/andrewdotn

- Fixed spelling errors

- Improved logic in `force_enable_logging`

- Make a fixture to un-disable logging after a test so that logging will be un-disabled even if the test fails due to an assertion error.
The fixture is in `test_fixture.py` because we can't `import logging` in `conftest.py` because there is a module called `logging` in the same folder.

- Mypy implicit import error. I can't commit without this.

Issue: pytest-dev#8711
PR: pytest-dev#8758
alexlambson added a commit to alexlambson/pytest that referenced this issue Nov 11, 2021
- Adds test coverage for `force_enable_logging` with a string `level`. Fixes [code coverage check](https://github.com/pytest-dev/pytest/pull/8758/checks?check_run_id=4123920877)

Issue: pytest-dev#8711
PR: pytest-dev#8758
alexlambson added a commit to alexlambson/pytest that referenced this issue Nov 11, 2021
- Adds test coverage for `force_enable_logging` with a string `level`. Fixes [code coverage check](https://github.com/pytest-dev/pytest/pull/8758/checks?check_run_id=4123920877)

Issue: pytest-dev#8711
PR: pytest-dev#8758
alexlambson pushed a commit to alexlambson/pytest that referenced this issue May 18, 2023
…ytest-dev#8711

Forces requested `caplog` logging levels to be enabled if they were disabled via `logging.disable()`

`[attr-defined]` mypy error ignored in `logging.py` because there were existing errors with the imports
and `loggin.Logger.manager` is an attr set at runtime. Since it's in the standard lib I can't really fix that.

Ignored an attr-defined error in `src/_pytest/config/__init__.py` because the re-export is necessary.

Issue: pytest-dev#8711
alexlambson added a commit to alexlambson/pytest that referenced this issue May 18, 2023
- Resolves issues from review provided by https://github.com/andrewdotn

- Fixed spelling errors

- Improved logic in `force_enable_logging`

- Make a fixture to un-disable logging after a test so that logging will be un-disabled even if the test fails due to an assertion error.
The fixture is in `test_fixture.py` because we can't `import logging` in `conftest.py` because there is a module called `logging` in the same folder.

- Mypy implicit import error. I can't commit without this.

Issue: pytest-dev#8711
PR: pytest-dev#8758
alexlambson added a commit to alexlambson/pytest that referenced this issue May 18, 2023
- Adds test coverage for `force_enable_logging` with a string `level`. Fixes [code coverage check](https://github.com/pytest-dev/pytest/pull/8758/checks?check_run_id=4123920877)

Issue: pytest-dev#8711
PR: pytest-dev#8758
alexlambson added a commit to alexlambson/pytest that referenced this issue May 18, 2023
- Address review ad rebase to latest from main
- Make `force_enable_logging` private.

Issue: pytest-dev#8711
PR: pytest-dev#8758
nicoddemus pushed a commit that referenced this issue May 18, 2023
…8758)

Forces requested `caplog` logging levels to be enabled if they were disabled via `logging.disable()`

`[attr-defined]` mypy error ignored in `logging.py` because there were existing errors with the imports
and `loggin.Logger.manager` is an attr set at runtime. Since it's in the standard lib I can't really fix that.

Ignored an attr-defined error in `src/_pytest/config/__init__.py` because the re-export is necessary.

Fixes #8711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: logging related to the logging builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
4 participants