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

[Enhancement]: Clarify documentation for IAbstractBuilder.WithLogger that it is for logs produced by the builder, not the container #1215

Closed
carlin-q-scott opened this issue Jul 17, 2024 · 4 comments · Fixed by #1251
Labels
enhancement New feature or request

Comments

@carlin-q-scott
Copy link

Problem

The IAbstractBuilder.WithLogger method documentation states:

Sets the logger.

What is it setting the logger for? Since I'm building a container I'd assume it's for the container, not the builder.

Solution

Clarify that this is the logger for the builder and that WithOutputConsumer is used to collect logs from the container.

Benefit

Less confusion for developers using this library.

Alternatives

.

Would you like to help contributing this enhancement?

No

@carlin-q-scott carlin-q-scott added the enhancement New feature or request label Jul 17, 2024
@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Jul 20, 2024

What is it setting the logger for? Since I'm building a container I'd assume it's for the container, not the builder.

Improving the documentation is always a good idea. After reading this issue and thinking about the various parts where log messages are produced, it is indeed probably not as obvious as I initially thought. I would like to provide some context, and then we can consider how to improve the documentation on this matter:

  • The builds' API WithLogger(ILogger) expects an ILogger implementation. This API call takes care of forwarding the Testcontainers library log messages to the appropriate logger, such as:
    [testcontainers.org 00:00:00.13] Connected to Docker:
      Host: npipe://./pipe/docker_engine
      Server Version: 26.1.1
      Kernel Version: 5.15.146.1-microsoft-standard-WSL2
      API Version: 1.45
      Operating System: Docker Desktop
      Total Memory: 15.51 GB
    [testcontainers.org 00:00:00.18] Docker network fee5640b5186 created
    [testcontainers.org 00:00:00.29] Docker container 599192691e46 created
    [testcontainers.org 00:00:00.32] Start Docker container 599192691e46
    
  • The WithOutputConsumer(IOutputConsumer) method exists only for the ContainerBuilder API and allows the user to forward container log messages (stdout, stderr) to a stream. Basically, it continuously forwards the container stream to the desired output consumer.
  • The IContainer interface additionally provides a method called GetLogsAsync(DateTime since, DateTime until, bool follow, CancellationToken cancellationToken) which returns the stdout/stderr messages for a specific period of time. If you do not provide a time period, you will receive all log messages from the beginning until now. Usually, I would use this method to gather the container log messages in case a test fails.

Edit:

While updating the documentation, we should also update this section. It is no longer up to date.

@carlin-q-scott
Copy link
Author

Thank you @HofmeisterAn for thoroughly thinking through the issue. I'm also using the GetLogsAsync method to collect logs after test failure, but the other developer on my team prefers WithOutputConsumer. I don't care which one is recommended in a <remark>, but perhaps both can be?

@HofmeisterAn
Copy link
Collaborator

I don't care which one is recommended in a <remark>, but perhaps both can be?

I don't understand that. What do you mean? Regarding the example you mentioned, I prefer the GetLogsAsync method because it's much simpler. Setting up the output consumer and handling the cleanup of the stream, etc., requires more boilerplate code.

@carlin-q-scott
Copy link
Author

@HofmeisterAn 🤷 I don't get it either, but I asked him to explain himself on this issue. I agree with you that GetLogsAsync is easier to work with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants