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

Prevent all loggers from printing to stdout #9

Closed
angelsanzn opened this issue Jun 1, 2021 · 8 comments
Closed

Prevent all loggers from printing to stdout #9

angelsanzn opened this issue Jun 1, 2021 · 8 comments
Labels
enhancement New feature or request no-activity

Comments

@angelsanzn
Copy link

angelsanzn commented Jun 1, 2021

Is your feature request related to a problem? Please describe.
When LogCaptor is launched, it makes all loggers log to the most verbose level, and they print to stdout. This is making my tests slow by an order of magnitude (from less than 1s to around 5s), because some libraries I invoke in the tests log very heavily.

More so, in some tests where the input data is very large and it happens to gets logged by an underlying library, the tests time out because the printing to stdout takes very long. I could work around this using .disableLogs, but that couples my test to a bunch of internal class names of the underlying library.

Describe the solution you'd like
I would like to have a way to configure LogCaptor to prevent all loggers from printing to stdout. This would not change how logging levels are currently handled by LogCaptor (as opposed to what .disableLogs does). The logged messages and objects would still be kept in memory in order to later assert against them with the current API.

This could even be the default behavior, although that might be a breaking change for some people.

In case it is not the default behaviour, it could be configured with an environment variable. What do you think?

Describe alternatives you've considered
An alternative is to manually disable all printing to stdout altogether. However, this would wipe out all other strings printed to stdout which are not logs (e.g. messages from the test runner or other testing libraries).

Another alternative is to add one .disableLogs call per verbose logging class somewhere before the test starts running, but that's fragile as it couples the tests to the logging implementation of each verbose logging library.

@Hakky54
Copy link
Owner

Hakky54 commented Jun 1, 2021

Hi Ángel,

Thank you for creating this issue and asking for this kind of feature of disabled log output to the console without losing the capability of capturing it. I will work on an implementation to have this feature included with the next release. I will update you when it is ready.

@Hakky54 Hakky54 added the enhancement New feature or request label Jun 1, 2021
@Hakky54 Hakky54 linked a pull request Jun 1, 2021 that will close this issue
@Hakky54
Copy link
Owner

Hakky54 commented Jun 1, 2021

Initial implementation is ready and it works. Please have a look. You can also fork the repo and test it locally and see if this solution is working for your use case. Looking forward to get your feedback

@angelsanzn
Copy link
Author

angelsanzn commented Jun 2, 2021

Hi Hakan,

Many thanks for the very quick reply! Let me take a closer look, as the implementation looks nice but I just tested it and I encountered two problems:

  1. I can still see all logs from all the loggers I didn't explicitly disable. What's more, when I replace the workaround described above with a call to LogCaptor.forRoot().disableConsoleOutput();, I see all logs and my tests choke the same way they did before I applied the workaround. So, detaching the console appender doesn't seem to be having any effect, at least when done at the same site as the workaround (first statement of the test).
  2. I'm getting an OutOfMemoryError in the second or third test from my suite.

@angelsanzn
Copy link
Author

angelsanzn commented Jun 2, 2021

Aside from that, I have a couple of comments about the solution:

  1. I only really need to disable console output for all loggers at the same time. So I would only use this with LogCaptor.forRoot().disableConsoleOutput();. It's not necessary to be able to disable it selectively for some loggers.
  2. Why not make this behavior the default? Do you think there will be a use case for showing logs on the console? What one wants is precisely to test logs, so printing them could even incentivize people to not test them.
  3. If you still want to make this behavior optional, why not allow it to be configured globally with an environment variable? This would reduce the burden on the user of the library, because they would not need to sprinkle their code with calls to .disableConsoleOutput. Just setting an env var in the test run configuration would be enough. Moreover, I believe that the first problem I just mentioned has to do with the timing when the call to .disableConsoleOutput is made. Even placing it in a static initializer block of my test class was too late, as my test run still prints to console both logs which I believe are logged before the static initializer block and logs which are clearly logged after the static initializer block.

@Hakky54
Copy link
Owner

Hakky54 commented Jun 2, 2021

Really great to get feedback on this, and strange that it is not working although it was working for my unit tests. There is an alternative which will definitely work without the need of calling additional methods on the LogCaptor or having environment variables. Can you add the following file logback.xml to your test resources and retest it:

<configuration>
    <statusListener class="ch.qos.logback.core.status.NopStatusListener" />
</configuration>

@Hakky54
Copy link
Owner

Hakky54 commented Jun 5, 2021

Hi @angelsanzn any updates from your side?

@angelsanzn
Copy link
Author

Thanks for the logback config suggestion. That one definitely stops all printing of logs. However, my tests are still somehow choking, not with an OOM but apparently getting blocked by something and then timing out. I have to investigate a bit more and maybe put together a minimal repro. I'll also try the custom VM argument you introduced support for and will get back to you soon. Thanks!

@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the no-activity label Jul 14, 2021
@stale stale bot closed this as completed Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request no-activity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants