[Enhancement]: Add convenient features to wait strategies

[HofmeisterAn]

Problem

The wait strategies in Testcontainers for .NET would benefit from refactoring. Currently, the wait strategies lack several convenient features, such as individual retry counts, timeouts, frequencies, etc.

Solution

-

Benefit

-

Alternatives

-

Would you like to help contributing this enhancement?

Yes

[jlevier]

@HofmeisterAn , are you thinking something like this? https://github.com/jlevier/dotnet-testcontainers/commit/a702c92ce2d16f45b58e265c42c8b2a4ada7720f This approach is granular in that each wait strategy could have its own distinct set of options. Not sure if that is desired or if applying a single set of options to the chained wait strategies makes more sense.
Pardon the incomplete solution - just wanted to make sure we are on the same page before going further. Thanks!

[HofmeisterAn]

Yes, something like that. I do not have a particular implementation in mind. Ideally, it should not break any existing configurations and allow us to configure each wait strategy individual. I will take a look at your proposal during the weekend.

[jlevier]

👋 @HofmeisterAn don't mean to bother but was wondering if you had time to review? Please let me know, thanks.

[HofmeisterAn]

You're not bothering me at all. Thanks for reminding me, in fact, I almost forgot to reply. I've looked at it a few days ago. I think having options to configure the wait strategies is a good idea. However, we should allow configuring each wait strategy individually. Without taking a closer look, it's difficult to say which changes we exactly need, but I think having a proper base class for each wait strategy could be a good start, similar to the WaitStrategyOptions. This would allow us to configure an individual wait strategy. Note that the example below is just an illustration of configuring the wait strategy individually. The API might look different from the final result.

Wait.ForUnixContainer()
  .AddCustomWaitStrategy(new UntilMessageIsLogged(string.Empty).WithBackoffStrategy(...))
  .AddCustomWaitStrategy(new UntilMessageIsLogged(string.Empty).WithBackoffStrategy(...))

It may be necessary to extend IWaitUntil, which I am trying to avoid. Rather than introducing any breaking changes, I would prefer to offer a smooth transition to a better API, if needed.

[jlevier]

I see what you mean about allowing configuration on each wait strategy individually. I totally agree with that level of granularity. Let me look into this further. Thanks.

[jlevier]

Ok, @HofmeisterAn here is the take 2 attempt. https://github.com/jlevier/dotnet-testcontainers/commit/fbe994d1fdd05cbb9f628e84f87a74f065b4d3ac Hopefully this captures the level of granularity we're looking for while also leaving the existing API in tact. Although I would prefer the API to be exposed via the chained approach, I hesitated to modify IWaitForContainerOS for a couple of reasons: 1) I wasn't sure if we wanted this on AddCustomWaitStrategy and 2) it seemed that these new With methods could be confusingly misused on ForUnixContainer() without specifying an actual wait strategy first. Maybe that is actually desired? I could see it going both ways so will defer to you for direction, please.

[HofmeisterAn]

Thank you for your update on the proposal.

Although I would prefer the API to be exposed via the chained approach, I hesitated to modify IWaitForContainerOS for a couple of reasons: [...]

Indeed, exposing the configurations to the IWaitForContainerOS builder pattern may be confusing. I will look again into and and see if I come up with a proper design.

I have reviewed your proposal and have created a follow-up proposal that includes a dedicated type for receiving the wait strategy implementation and additional features such as the backoff strategy. Although I am not entirely satisfied with the names used, they can be revised later. I am curious what do you think about it: compare/feature/propose-wait-strategy-changes.

As IWaitForContainerOS is an internal class, we may consider replacing IEnumerable<IWaitUntil> Build() with IEnumerable<WaitStrategyOption> Build() and passing the wrapped type to DockerContainer.

What are your thoughts on this approach?

[jlevier]

@HofmeisterAn, thank you for your feedback and sorry it has taken me so long to get back to you on this.

I think I know what you are saying but might need some clarification. Would we intend to actually call this https://github.com/testcontainers/testcontainers-dotnet/compare/feature/propose-wait-strategy-changes#diff-ca8e5f81c5dc6550efd7888bc6dd66184966a85b244da3d2b0dcc9a235db6530R7 something like BackOffWaitStrategyOption? Since it is implementing both IWaitUntil, IWaitStrategyOption, I get the impression that it would be preferable for all existing wait strategies to also implement IWaitStrategyOption in order to always have the default options (Retries/Interval/Timeout) available for chaining. That would eliminate the need to pass them in as an overloaded parameter like we both have here: https://github.com/testcontainers/testcontainers-dotnet/compare/feature/propose-wait-strategy-changes#diff-fe60b6e945c153654f03486224ffe5d028020a42a3c0c9d9814c813abb592d9fR8. This is how I am interpreting your mention of a "dedicated type" but maybe I am off base.

[HofmeisterAn]

I get the impression that it would be preferable for all existing wait strategies to also implement IWaitStrategyOption in order to always have the default options (Retries/Interval/Timeout) available for chaining.

No worries. Indeed, that is true. However, wouldn't it break current configurations? I'm trying to avoid interfering with existing wait strategies and find something that enables us to proceed with the suggested changes once we introduce a new major version.