Add test verifying log4j2 updateLoggers is called #5822
Conversation
pativa
force-pushed
the
test-log4j-reconfigure
branch
from
9e154a6 to
d78df65
Compare
Signed-off-by: Patrik Ivarsson <[email protected]>
pativa
force-pushed
the
test-log4j-reconfigure
branch
from
d78df65 to
71a5d6b
Compare
|
Sure, it's updated to target |
pativa
changed the title
There was a problem hiding this comment.
I worry the test may be relying on internal details too much which could make it fragile to later changes, but I'm not sure how else to write the test. It would have caught removing updateLoggers before, so perhaps it's still worth adding and see if fragility becomes an issue.
| @@ -186,6 +187,27 @@ void asyncLogShouldNotBeDuplicated() throws IOException { | |||
| .until(() -> registry.get("log4j2.events").tags("level", "info").counter().count() == 3); | |||
| } | |||
|
|
|||
| // see https://github.com/micrometer-metrics/micrometer/pull/872 | |||
There was a problem hiding this comment.
Should we also add @Issue("#867")?
There was a problem hiding this comment.
We can, though I guess 867 is linked to from 872. Obviously out of scope for this PR, but since you reminded me, I wonder if we should get rid of that @Issue annotation entirely since we have no tooling that uses it and so it doesn't seem to add value over a comment without an annotation.
As a questioned whether the call to
updateLoggersis necessary in #5810 I made a quick test verifying that its being called so it can't accidentally be removed 🙂(made as a separate PR as this test is unrelated to the other change)