uvicorn icon indicating copy to clipboard operation
uvicorn copied to clipboard

Fix duplicate logs when use logging.basicConfig (#1285)

Open guyskk opened this issue 4 years ago • 7 comments

Fix https://github.com/encode/uvicorn/issues/1285

guyskk avatar Dec 09 '21 02:12 guyskk

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

euri10 avatar Dec 10 '21 09:12 euri10

@euri10 I see, will check on it.

guyskk avatar Dec 10 '21 10:12 guyskk

Hi @euri10, the failures is because pytest caplog not able to capture propagate=False logger, see also: https://github.com/pytest-dev/pytest/issues/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.

guyskk avatar Dec 11 '21 10:12 guyskk

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

euri10 avatar Dec 11 '21 10:12 euri10

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

guyskk avatar Dec 11 '21 10:12 guyskk

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

euri10 avatar Jul 06 '22 12:07 euri10

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 avatar Jul 07 '22 05:07 Kludex