Set propagate to False on "uvicorn" logger

[guyskk]

Fix #1285

[euri10]

this seems to break quite a lots of our tests @guyskk

[guyskk]

@euri10 I see, will check on it.

[guyskk]

Hi @euri10, the failures is because pytest caplog not able to capture propagate=False logger, see also: pytest-dev/pytest#3697

And the caplog_for_logger helper also not work on test config cases, because when create Config object, Config.configure_logging will remove caplog.handler.

A simple work around is set propagate=True before execute tests. Otherwise we need refactor Config class to make it friendly to unit tests.

[euri10]

And the caplog_for_logger helper also not work on test config cases, because when create Config object, Config.configure_logging will remove caplog.handler.

mm this is where I'm not too sure, I remeber that pytest issue and I think the fixture was written just for that, and if you look at the way we use it, we do Config first then only we use the fixture so I dont think that explanation is correct, but I may be off

[guyskk]

Yes, the caplog_for_logger works when test logs after create Config object, but it can not capture logs inside Config.__init__.
eg: test_should_warn_on_invalid_reload_configuration, test_reload_dir_is_set and test_non_existant_reload_dir_is_not_set

[euri10]

with some tweaks in the explanation and a rebase that solves conflicts it's ready to be merged for me

[euri10]

thanks a lot @guyskk
@Kludex since you tagged this on the 0.20.0 release I'll let you merge it, or maybe we can point it to this PR release but I'm not sure it exists yet

[Kludex]

It does. Changed the milestone.

The milestones are mainly so I can organize myself. It's a self promise that I'm going to look for the current milestone before the release. :)

[Kludex]

But then... all our users needs to do this if capturing the logs on their setup, right? 🤔

Should we mention this somewhere, or it's fine?

[guyskk]

all our users needs to do this if capturing the logs on their setup, right?

I think if users want to config uvicorn's logs, they should provide full logging config, no needs to set propagate. https://www.uvicorn.org/settings/#logging

And if users only want to config application logs, call logging.basicConfig works as expected, uvicorn's logs will not affected.

[Kludex]

I mean, if people want to capture those logs on their test, for some reason (?) they need to do the same thing.

[euri10]

But then... all our users needs to do this if capturing the logs on their setup, right? thinking

Should we mention this somewhere, or it's fine?

It introduces a change only for the users who in their tests, use a Config object and are looking for logs if I get it correctly, logging is complex so I'll admit I'm not 100% sure.

I'd be tempted to say it's a rather slim part of people in that case but I may be proven wrong, but there's no harm in saying something like:

Should you use the caplog pytest fixture in your test suite to capture uvicorn logs, you might want to use (the line above) if you use a Config object, please see (test line that explains it)

[euri10]

thoughts on this @guyskk ?

[Kludex]

(I just wanted to point this out, I don't have any strong opinion about this, jfyk)

[guyskk]

I mean, if people want to capture those logs on their test, for some reason (?) they need to do the same thing.

Yes. Maybe mention it in changelog? if those user's tests broken(rare usage), they can easily know why.

[euri10]

ok let's add a note then !

[Kludex]

@guyskk Is this "propagate": False still needed?

[Kludex]

It is.