testcontainers / testcontainers-dotnet

A library to support tests with throwaway instances of Docker containers for all compatible .NET Standard versions.
https://dotnet.testcontainers.org
MIT License
3.73k stars 262 forks source link

feat: Extend the "wait until file exists" API to distinguish between the test host and container filesystem #1009

Closed maaex closed 10 months ago

maaex commented 11 months ago

What does this PR do?

The existing implementation of the UntilFileExists WaitStrategy seems to look for a file in the host's file system instead of the container's. This PR changes that so UntilFileExists looks for the file in the container's file system instead.

Why is it important?

Looking for a file on the host instead of the container seems like incorrect behavior.

netlify[bot] commented 11 months ago

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit 0d3f6cc6aba3ccfd4f62e4c3002d7500896c8bfe
Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6542140a474d0e00088acc07
Deploy Preview https://deploy-preview-1009--testcontainers-dotnet.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

maaex commented 11 months ago

Thank you for your pull request. This is some kind of on purpose, but I understand the confusion regarding the wait strategy. I would like to suggest the following in order to avoid breaking existing configurations:

  1. Enhance the UntilFileExists(string) documentation (interface) and make it more explicit that it checks for a file on the test host.
  2. Add an overloaded method that checks for the file inside the container.

We can also introduce two explicit APIs (wait strategies): One that checks on the test host and another that checks in the container, while deprecating the old one. My goal is to avoid introducing breaking changes without informing the developers.

Good point. Should I move the code into a UntilFileExistsInContainer(string) method (+ a related WaitStrategy class) maybe? That naming should make the distinction clearer between the two.

HofmeisterAn commented 11 months ago

Good point. Should I move the code into a UntilFileExistsInContainer(string) method (+ a related WaitStrategy class) maybe? That naming should make the distinction clearer between the two.

Yes, I think that would be a better and more explicit approach. I am currently thinking how we can enhance the wait strategy API in a broader sense, without the need for a complete overhaul. The wait strategy API has remained unchanged since its early days. Perhaps an approach similar to the HttpWaitStrategy, which allows more configurations, would be a more future-proof solution. We could consider implementing something like a FileSystemWaitStrategy, which accept a file path and a scope to determine whether it is a file within a container or a file on the test host.

maaex commented 10 months ago

I see that the HttpWaitStrategy has different usage pattern to the 'normal' wait strategies. Do we need to go there with this? Maybe something like Wait.ForUnixContainer().UntilFileExists("/foo/bar", FileSystem.Host) would suffice? And have the default scope value as Host to avoid breaking any existing functionality.

And do you have any suggestions for what to call the scope enum?

HofmeisterAn commented 10 months ago

Do we need to go there with this? Maybe something like

No, I think your suggestion is reasonable.

And do you have any suggestions for what to call the scope enum?

FileSystem.Host, FileSystem.Container sounds good 👍.

maaex commented 10 months ago

I have updated the code as per our discussion. I was unsure of where to put the FileSystem enum but ended up putting it under TestContainers.Configurations as I felt it was a more generic thing than WaitStrategies. What do you think?

HofmeisterAn commented 10 months ago

I took a brief look. It looks very good. Great first contribution, thanks. I won't be able to finalize the review today, but I can certainly do it tomorrow.