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

Do not show logs for failed tests by default #124

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

lorenyu
Copy link
Contributor

@lorenyu lorenyu commented Feb 3, 2023

Ticket

Changes

  • Turn off showing of captured logs by default in failed tests

Context for reviewers

Logs can be useful but are generally noisy to include by default for failed tests so this PR turns it off by default.
Turning them on can be done with make test args="--show-capture=all"

Testing

Example now by default

image

@lorenyu lorenyu requested a review from chouinar February 3, 2023 20:58
test: ## Run all tests except for audit logging tests
$(PY_RUN_CMD) pytest -m "not audit" $(args)
$(PY_RUN_CMD) pytest -m "not audit" --show-capture=no $(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have a quick sync chat on this. While I do want to keep my setup working, I don't want to dictate the implementation so much so for just myself.

Ideally the default would be roughly the same verbosity as it was before the changes to avoid drowing out the test output, but configurable enough for when you do want the full logs.

I did like your idea about a separate .env file for an individual developer, but what would be the best way to make it clear that exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure happy to chat about it. I'll look for some time.

If I understand correctly, before the changes either the console handler we defined would be attached to the root logger, or the caplog handler would be attached (if the developer included the logging_fix fixture which turned off the console handler), but not both.

the console handler verbosity can be controlled via LOG_LEVEL, which could either be in local.env or a separate test.env file if we go that route. the caplog verbosity can be controlled by caplog.setLevel(.

also by default, neither the logs that get printed to stdout stream from console handler nor the logs captured by caplog get shown to the user unless a test fails, and at that point both sets of logs get printed out.

so maybe we can chat about what the ideal would be in each / all of these scenarios, and how it could potentially be flexible to individual developer preferences.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've got an idea, not sure the best way to configure it, but rather than messing with log levels, what about the stream handler itself?

Right now, we configure the stream handler in logging/config.py as console_handler = logging.StreamHandler(sys.stdout), but what if that was overridable in some way?

If we make a real simple stream ourselves:

class NullStream(StringIO):
    def write(self, text:str) -> None:
        pass

and then do logging.StreamHandler(NullStream()) - we get no logs.

I'm not sure how we would want to configure this though. I think this might be best handled by the user-specific env file we've thought about, unless you've got another idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable. But it seems like it has the same effect as changing the log level while requiring more code?

I'm imagining it'd be the difference in local.env of log_handler=console vs log_level=CRITICAL or log_level=WARNING

Do you think the question boils down to how we can have user-specific env files? If so, I think we can rename local.env to .env, remove it from source control, provide an example.env, and add something along the lines of cp local.env .env if .env doesn't already exist to make setup-local. i think we can do that using Makefile's built in method of detecting whether a target already exists i.e.

.env:
  cp example.env .env

setup-local: .env
  ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants