Added information about capturing static inner classes, fixed javadoc warnings

[AkaZver]

Hello!

Originally I've found this issue in Spring Boot integration tests
How to reproduce: create some static inner class and try to capture its logs - it will write logs as expected but nothing will be captured
It happens because Class.getName() on static inner class will return something like org.example.untitled.Main$Inner but for correct capturing we need org.example.untitled.Main.Inner which can be returned by Class.getCanonicalName() (also may return null in some cases so I used Optional)

Also fixed some javadoc warnings from build log

Please review and give me some feedback)
Thx

[Hakky54]

Hi Vasiliy,

Nice catch and thank you for providing a PR with a fix for it. I like the implementation and the usage of the Optional.
I see you added a unit test for Log4J2 using Lombok annotations. Although this might be sufficient to test the functionality I am not quite sure how it will behave and work for the other use cases such as when using SLF4J, Java Util Logging etc. It would be nice to also cover those cases, what do you think?

I really appreciate that you fixed the build warnings regarding the javadoc, but what do you think of the resulting javadoc for those two methods?

[AkaZver]

Hi Vasiliy,

Nice catch and thank you for providing a PR with a fix for it. I like the implementation and the usage of the Optional.
I see you added a unit test for Log4J2 using Lombok annotations. Although this might be sufficient to test the functionality I am not quite sure how it will behave and work for the other use cases such as when using SLF4J, Java Util Logging etc. It would be nice to also cover those cases, what do you think?

I really appreciate that you fixed the build warnings regarding the javadoc, but what do you think of the resulting javadoc for those two methods?

It works fine with Log4j2 and Slf4j, but there are problems with JUL: Lombok @Log annotation adds logger with getName() call, also it's the usual way to create it with JUL as far as I remember.
If we can't programmatically fix this then I guess I should revert part with getName/getCanonicalName and write about static inner classes in readme?

Javadoc looks fine for me both in Idea and browser. Should I add something else?

[Hakky54]

Javadoc looks fine for me both in Idea and browser. Should I add something else?

Well I was considering to suppress the warning by adjusting the javadoc plugin instead of adding the return type within the documentation. But I think what you did is better as my approach would also suppress other issues if they would occur in the future. So lets keep your changes 😊

It works fine with Log4j2 and Slf4j, but there are problems with JUL: Lombok @log annotation adds logger with getName() call, also it's the usual way to create it with JUL as far as I remember. If we can't programmatically fix this then I guess I should revert part with getName/getCanonicalName and write about static inner classes in readme?

Aha, good that we have discovered this for JUL. I still like the feature of capturing the inner classes and having this support for SLF4J and Log4J2 based logging api/implementations. So I would suggest to have a new section within Known issues maybe called Capturing logs for inner classes and mention that it works for SLF4J and Log4J2 based classes but not for JUL. What do you think? So no need to revert your changes 🎉

And maybe it is also good to have a unit test for slf4j with inner class which passes. And one unit test for jul with inner classes which does not return logs. In that way we have covered the three use cases within the tests if thats not too much effort for you. What do you think, would it be valuable to add that?

[AkaZver]

Aha, good that we have discovered this for JUL. I still like the feature of capturing the inner classes and having this support for SLF4J and Log4J2 based logging api/implementations. So I would suggest to have a new section within Known issues maybe called Capturing logs for inner classes and mention that it works for SLF4J and Log4J2 based classes but not for JUL. What do you think? So no need to revert your changes 🎉

And maybe it is also good to have a unit test for slf4j with inner class which passes. And one unit test for jul with inner classes which does not return logs. In that way we have covered the three use cases within the tests if thats not too much effort for you. What do you think, would it be valuable to add that?

I think I was sleepy at time when I was testing it but situation is opposite)
SLF4J and JUL are ok, but Log4j2 is failing because of that LoggerContext.java#L98
So 2 loggers against 1 - I reverted change in LogCaptor class, added tests and updated readme
Please, take a look

[Hakky54]

Hmm, The code changes looks good to me and I want to try this locally so I am just merging this PR. I want to test with couple of different scenarios to discover the behaviour with different use cases. Thank you for this discovery and improvements on the library!

[Hakky54]

@all-contributors please add @AkaZver for design ideas research

[allcontributors]

@Hakky54

I've put up a pull request to add @AkaZver! 🎉

[Hakky54]

I have refactored your PR, hope you won't be mad as I changed most of the stuff. The unit test you have provided are very compact and advanced which I liked at the time of merging it, but I preferred to move them to separate classes and packages. The resulting code is unfortunately verbose but easier to maintain for me. I also discovered that static inner classes works with Log4J. Strange that Apache decided to change the behaviour for Log4J2. But I like the documentation you added, it clearly states how you should properly test those cases 👍🏼