feat: fail-fast of wait.LogStrategy

[frozenbonito]

What does this PR do?

This PR fixes the check that the container is not emitting new logs (please refer this comment).

logs variable contains all logs after container start, so it can never be an empty string. Therefore, comparisons with empty strings are meaningless.
Instead, it is verified that the length of the logs has not changed since the last check in this PR.

This fix broke Test_GetLogsFromFailedContainer, so I fixed it too.

Why is it important?

When the container stop, LogStrategy should fail-fast, but it doesn't.
This PR will fix it.

Related issues

• Closes [Bug]: Fail-fast of wait.LogStrategy doesn't work properly #1302

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	9102ab6
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/64ca1101c534970008c4f6f7
😎 Deploy Preview	https://deploy-preview-1304--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[mdelapenya]

Hi @frozenbonito thanks for opening the issue and taking the time to elaborate a PR 💪

I think it makes sense to check for logs length to verify if the log changed after each poll of the logs.

I'm labelling the PR as enhancement instead of bug, as the wait strategy indeed works and here we are just improving the fail-fast mechanism. wdyt?

[mdelapenya]

Taking a deeper look at the current code, I see that we are extracting the state of the container, but we are evaluating if there is an error too late 🤔. I've double checked all the usages of the checkTarget function and we are always evaluating the error it returns immediately.

We could probably fix that logic, keeping the bug nature, like this:

			checkErr := checkTarget(ctx, target)
+			if checkErr != nil {
+				return checkErr
+			}

			reader, err := target.Logs(ctx)
			if err != nil {
				time.Sleep(ws.PollInterval)
				continue
			}

			b, err := io.ReadAll(reader)
			if err != nil {
				time.Sleep(ws.PollInterval)
				continue
			}

			logs := string(b)
-			if logs == "" && checkErr != nil {
-				return checkErr
+ 			if logs == "" {
+				return fmt.Errorf("no logs found")
			} else if strings.Count(logs, ws.Log) >= ws.Occurrence {
				break LOOP
			} else {
				time.Sleep(ws.PollInterval)
				continue
			}

Thoughts?

[frozenbonito]

@mdelapenya Thank you for comments.

I think LogStrategy is special case.
This is because sometimes we want to wait for specific logs whether the container has exited or not.
Please see this my comment.

If we evaluate and return the error of checkTarget immediately, some uses shown by tests are broken.
The most obvious example is TestCMD.

testcontainers-go/docker_test.go

Lines 1203 to 1228 in f5a4a54

	func TestCMD(t *testing.T) {
	/*
	echo a unique statement to ensure that we
	can pass in a command to the ContainerRequest
	and it will be run when we run the container
	*/
	ctx := context.Background()
	req := ContainerRequest{
	Image: "docker.io/alpine",
	WaitingFor: wait.ForAll(
	wait.ForLog("command override!"),
	),
	Cmd: []string{"echo", "command override!"},
	}
	c, err := GenericContainer(ctx, GenericContainerRequest{
	ProviderType: providerType,
	ContainerRequest: req,
	Started: true,
	})
	require.NoError(t, err)
	terminateContainerOnEnd(t, ctx, c)
	}

[mdelapenya]

Ah this makes even more sense now, sorry for not recalling that very same conversation 🤦

Please see this #944 (comment).

Then this changes makes sense. Let me fix the issue with the current state of the CI, caused by the recent changes in Go + Moby (see #1394), as any CI job will fail because of that (unless we pin the Go versions)

[mdelapenya]

LGTM! Will merge this one right after fixing the errors with Go+Moby+Host header validation

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication