feat: expose functions for resource clean up in tests and examples #2738

stevenh · 2024-08-19T15:49:45Z

Ensure that all resources are cleaned up for tests and examples even if they fail.

This leverages new helpers in testcontainers:

TerminateContainer for examples
CleanupContainer and CleanupNetwork for tests

These are required ensuring that containers that are created but fail in later actions are returned alongside the error so that clean up can be performed.

Consistently clean up created networks using a new context to ensure that the removal gets run even if original context has timed out or been cancelled.

Use fmt.Print instead of log.Fatal to ensure that defers are run in all examples again ensuring that clean up is processed.

Call Stop from Terminate to ensure that child containers are shutdown correctly on clean up as the hard coded timeout using by ContainerRemove is too short to allow this to happen correctly.

Clean up of test logic replacing manual checks and asserts with require to make them more concise and hence easier to understand.

Quiet test output by either capturing or disabling output so it's easier to identify issues when tests are run in non verbose mode.

Clarify source of errors with wrapping and update tests to handle.

Ensure that port forwarding container is shutdown if an error occurs during setup so it isn't orphaned.

Shutdown the port forwarding container on both stop and terminate to prevent it being orphaned when the Stop is used.

Add missing error checks to tests.

Remove unused nolint directives and enable the nolintlint to catch any regressions.

netlify · 2024-08-19T15:50:02Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`e04735e`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/66e1d172d88c0c0008ebbd43
😎 Deploy Preview	https://deploy-preview-2738--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.

stevenh · 2024-08-20T08:54:33Z

@mdelapenya sorry about the size of this one, no easy way to get around it. A good amount of the files are just go.mod / go.sum changes, but will still be a long review I'm afraid.

mdelapenya

Excellent work, changes consistently applied across the codebase. Thanks for making reviews that easy 🙇

I added a few minor comments, but LGTM. I will basically accept the PR, just want some clarification on those points.

docker.go

docker_auth_test.go

docker_test.go

docs/features/common_functional_options.md

modules/pulsar/pulsar_test.go

modules/registry/examples_test.go

wait/host_port.go

mdelapenya · 2024-09-10T15:39:27Z

BTW, I've resolved the conflicts manually

mdelapenya · 2024-09-10T15:40:35Z

~~One last thing the matchFile function is affected by make lint. Could you include it in your changes?~~

Never mind: I updated it

mdelapenya · 2024-09-10T16:37:32Z

@stevenh there is one error in a testable example for the registry module. I think we messed up something in the fmt prints.

stevenh · 2024-09-10T21:34:08Z

@stevenh there is one error in a testable example for the registry module. I think we messed up something in the fmt prints.

Just ran the tests locally, no errors, will keep an eye on the CI.

go test -timeout 10m -coverprofile=/tmp/vscode-goDlE8R3/go-code-cover github.com/testcontainers/testcontainers-go/modules/registry -v -count=1

=== RUN   TestRegistry_unauthenticated
2024/09/10 22:32:28 github.com/testcontainers/testcontainers-go - Connected to docker:
  Server Version: 27.1.1
  API Version: 1.46
  Operating System: Docker Desktop
  Total Memory: 31947 MB

...

2024/09/10 22:32:41 🚫 Container terminated: e46301df078f
--- PASS: ExampleRun_pushImage (3.84s)
PASS
coverage: 79.7% of statements
ok  	github.com/testcontainers/testcontainers-go/modules/registry	12.630s	coverage: 79.7% of statements

stevenh · 2024-09-10T21:52:58Z

thanks for a very through review on such a big PR @mdelapenya.

I've made 90% of the changes, resolving the comments that match, there's a few questions outstanding, but getting close.

mdelapenya · 2024-09-11T10:34:02Z

@stevenh I'm marking this PR as an enhancement, as we are exposing new API functions for the cleanup, that are being used everywhere in the codebase. Therefore users of the project will be notified in the release notes about this new available feature. I changed the title in consequence.

docs/features/creating_container.md

Ensure that all resources are cleaned up for tests and examples even if they fail. This leverages new helpers in testcontainers: * TerminateContainer for examples * CleanupContainer and CleanupNetwork for tests These are required ensuring that containers that are created but fail in later actions are returned alongside the error so that clean up can be performed. Consistently clean up created networks using a new context to ensure that the removal gets run even if original context has timed out or been cancelled. Use fmt.Print instead of log.Fatal to ensure that defers are run in all examples again ensuring that clean up is processed. Call Stop from Terminate to ensure that child containers are shutdown correctly on clean up as the hard coded timeout using by ContainerRemove is too short to allow this to happen correctly. Clean up of test logic replacing manual checks and asserts with require to make them more concise and hence easier to understand. Quiet test output by either capturing or disabling output so it's easier to identify issues when tests are run in non verbose mode. Clarify source of errors with wrapping and update tests to handle. Ensure that port forwarding container is shutdown if an error occurs during setup so it isn't orphaned. Shutdown the port forwarding container on both stop and terminate to prevent it being orphaned when the Stop is used. Add missing error checks to tests. Remove unused nolint directives and enable the nolintlint to catch any regressions. Don't use container as a variable as its overused.

stevenh · 2024-09-11T17:22:59Z

@mdelapenya I think this should be good, just two open discussion points which I don't think are blockers.

mdelapenya

LGTM, great PR 🙇

stevenh requested a review from a team as a code owner August 19, 2024 15:49

stevenh marked this pull request as draft August 19, 2024 15:49

This was referenced Aug 19, 2024

[Bug]: various random test failures #2685

Open

fix(reaper): refactor to allow retries and fix races #2728

Draft

stevenh force-pushed the fix/resource-cleanup branch 8 times, most recently from 931b868 to 657e42c Compare August 20, 2024 08:50

stevenh marked this pull request as ready for review August 20, 2024 08:54

mdelapenya reviewed Sep 10, 2024

View reviewed changes

stevenh force-pushed the fix/resource-cleanup branch from 124cf30 to 670f7eb Compare September 10, 2024 21:31

stevenh force-pushed the fix/resource-cleanup branch from 670f7eb to c6e53c5 Compare September 10, 2024 21:44

stevenh force-pushed the fix/resource-cleanup branch from c6e53c5 to 1574d2b Compare September 10, 2024 22:10

mdelapenya added the chore Changes that do not impact the existing functionality label Sep 11, 2024

mdelapenya self-assigned this Sep 11, 2024

mdelapenya changed the title ~~fix: resource clean up for tests and examples~~ feat: expose functions for resource clean up in tests and examples Sep 11, 2024

mdelapenya added enhancement New feature or request and removed chore Changes that do not impact the existing functionality labels Sep 11, 2024

mdelapenya reviewed Sep 11, 2024

View reviewed changes

docs/features/creating_container.md Show resolved Hide resolved

stevenh force-pushed the fix/resource-cleanup branch from 1574d2b to e04735e Compare September 11, 2024 17:20

stevenh requested a review from mdelapenya September 11, 2024 17:23

mdelapenya approved these changes Sep 12, 2024

View reviewed changes

mdelapenya merged commit b60497e into testcontainers:main Sep 12, 2024
112 checks passed

stevenh deleted the fix/resource-cleanup branch September 12, 2024 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose functions for resource clean up in tests and examples #2738

feat: expose functions for resource clean up in tests and examples #2738

stevenh commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

stevenh commented Aug 20, 2024

mdelapenya left a comment

mdelapenya commented Sep 10, 2024

mdelapenya commented Sep 10, 2024 •

edited

Loading

mdelapenya commented Sep 10, 2024

stevenh commented Sep 10, 2024

stevenh commented Sep 10, 2024

mdelapenya commented Sep 11, 2024

stevenh commented Sep 11, 2024

mdelapenya left a comment

feat: expose functions for resource clean up in tests and examples #2738

feat: expose functions for resource clean up in tests and examples #2738

Conversation

stevenh commented Aug 19, 2024 • edited Loading

netlify bot commented Aug 19, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh commented Aug 20, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya commented Sep 10, 2024

mdelapenya commented Sep 10, 2024 • edited Loading

mdelapenya commented Sep 10, 2024

stevenh commented Sep 10, 2024

stevenh commented Sep 10, 2024

mdelapenya commented Sep 11, 2024

stevenh commented Sep 11, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

stevenh commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

mdelapenya commented Sep 10, 2024 •

edited

Loading