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.

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

Added tear down for log captor, small enhancements #12

Merged
merged 1 commit into from Jun 4, 2021

Conversation

AkaZver
Copy link
Contributor

@AkaZver AkaZver commented Jun 3, 2021

Hello!

I discovered that if you attach captor for class it will never be detached and could potentially affect other tests, because all appenders are still visible between tests.
So I decided to add a method to safely detach captor by direct call (maybe in @AfterAll) or via try-with-resources.

Btw, all attached appenders had no name and you couldn't identify which one was attached by library.
Also I forgot to rename variable with MDC)

Thank you.

@Hakky54
Copy link
Owner

Hakky54 commented Jun 3, 2021

Hi @AkaZver

Thank you and well discovered! I like the idea and also the implementation.
I dropped a small comment.

@Hakky54
Copy link
Owner

Hakky54 commented Jun 3, 2021

Also I forgot to rename variable with MDC

I see this is still present within the unit tests. Would it be possible for you to also change it there to diagnosticContext?

@AkaZver
Copy link
Contributor Author

AkaZver commented Jun 3, 2021

Also I forgot to rename variable with MDC

I see this is still present within the unit tests. Would it be possible for you to also change it there to diagnosticContext?

Fixed

@Hakky54
Copy link
Owner

Hakky54 commented Jun 3, 2021

By the way, what is your opinion regarding clearing the listappender within the close method. I am not quite sure if we also should add that...

I have the feeling that if we clear it there it will be picked up earlier by the garbage collector if needed. The downside is that the logs are not accessible anymore when the close method is called, but would that be an issue?

What is your opinion regarding this enhancement?

@AkaZver
Copy link
Contributor Author

AkaZver commented Jun 3, 2021

By the way, what is your opinion regarding clearing the listappender within the close method. I am not quite sure if we also should add that...

I have the feeling that if we clear it there it will be picked up earlier by the garbage collector if needed. The downside is that the logs are not accessible anymore when the close method is called, but would that be an issue?

What is your opinion regarding this enhancement?

If you use static captor for multiple tests then @AfterEach with clearLogs() + @AfterAll with close() should do the trick.

If you prefer variable creation per test method using try-with-resources you won't have access to appender outside of the scope anyway, because captor will be already destroyed with all its content (or am I missing something?).

@Hakky54
Copy link
Owner

Hakky54 commented Jun 3, 2021

If you use static captor for multiple tests then @AfterEach with clearLogs() + @afterall with close() should do the trick.
If you prefer variable creation per test method using try-with-resources you won't have access to appender outside of the scope anyway, because captor will be already destroyed with all its content (or am I missing something?).

Yes sounds good. No need to adjust something at that level

@Hakky54
Copy link
Owner

Hakky54 commented Jun 4, 2021

Thank you Vasiliy for this amazing PR! It looks good to me, let's merge it 🎉

@Hakky54 Hakky54 merged commit d4a80b2 into Hakky54:master Jun 4, 2021
@Hakky54
Copy link
Owner

Hakky54 commented Jun 4, 2021

Your changes are now available within version 2.6.1 I just published a new version

@AkaZver AkaZver deleted the feature/add-tear-down branch June 9, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants