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.

Sign up for GitHub

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

Jump to bottom

Do not show logs for failed tests by default #124

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

Do not show logs for failed tests by default #124

Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3b4b441
Don't kill all processes when testing os.kill
lorenyu Feb 3, 2023
ae18998
Use cat instead of ls to avoid race condition
lorenyu Feb 3, 2023
3eec7e2
Isolate audit tests from rest of tests
lorenyu Feb 3, 2023
da712bc
Set db environment variables
lorenyu Feb 3, 2023
dd085c6
Add autouse fixture to load local env vars instead
lorenyu Feb 3, 2023
bdca593
Merge branch 'main' into lorenyu/testonhost
lorenyu Feb 3, 2023
2e3d39a
Remove obsolete comment
lorenyu Feb 3, 2023
9d3a8c3
Merge branch 'lorenyu/testonhost' of github.com:navapbc/template-appl…
lorenyu Feb 3, 2023
49abc63
Revert var name
lorenyu Feb 3, 2023
200d2ad
Turn off log capture for failed tests by default
lorenyu Feb 3, 2023
9b37c07
Update comment explaining log capture
lorenyu Feb 3, 2023
6edbc27
Merge branch 'main' into lorenyu/testlogcapture
lorenyu Feb 3, 2023
19b4eeb
Add link to docs
lorenyu Feb 3, 2023
103cff7
Merge branch 'lorenyu/testlogcapture' of github.com:navapbc/template-…
lorenyu Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,13 @@ db-migrate-heads: ## Show migrations marked as a head
# Testing
##################################################

# Turn off log capture for failed tests by default.
# To re-enable display of captured logs when tests fail, run `make test args="--show-capture=all"`
# Or to turn off capturing of logs (and have logs print to stdout in realtime), run `make test args="--capture=no"`
# or for short `make test args="-s"`
# See https://docs.pytest.org/en/latest/how-to/capture-stdout-stderr.html#how-to-capture-stdout-stderr-output
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
  ...


test-audit: ## Run audit logging tests
$(PY_RUN_CMD) pytest -m "audit" $(args)
Expand Down