Skip to content

Simplify cleanup logic in integration tests#156

Merged
patrickhener merged 1 commit intopatrickhener:mainfrom
alexandear-org:refactor/integration
Apr 27, 2026
Merged

Simplify cleanup logic in integration tests#156
patrickhener merged 1 commit intopatrickhener:mainfrom
alexandear-org:refactor/integration

Conversation

@alexandear
Copy link
Copy Markdown
Contributor

Description

Refactor integration tests with t.Cleanup.

Copilot AI review requested due to automatic review settings April 27, 2026 12:47
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors integration tests to rely on t.Cleanup for container teardown, reducing repetitive per-test cleanup logic.

Changes:

  • Updated spawnTestContainer to register container cleanup via t.Cleanup and return only the mapped port.
  • Simplified integration tests by removing explicit cleanup calls and error handling around container startup.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
integration/integration_test.go Switches tests to use the simplified spawnTestContainer API (port-only) and removes explicit cleanup/error checks.
integration/functions.go Refactors spawnTestContainer to handle failures internally and register container teardown via t.Cleanup.
Comments suppressed due to low confidence (1)

integration/functions.go:29

  • spawnTestContainer now performs assertions via require.*, but it isn't marked as a test helper. Consider calling t.Helper() at the start so any failures (e.g., file opens, container startup) are reported at the test callsite instead of inside this helper, consistent with other helpers like smbConnect.
func spawnTestContainer(t *testing.T, config string, webdav bool, smb bool) nat.Port {
	// Make sure the host-side coverage drop dir exists and is writable
	// by the container's non-root uid (1000). 0o777 is fine for an
	// ephemeral test artifact directory.
	require.NoError(t, os.MkdirAll(coverageDir, 0o777))
	require.NoError(t, os.Chmod(coverageDir, 0o777))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread integration/functions.go
Comment on lines +167 to +169
t.Cleanup(func() {
cleanupContainer(t, goshsServer)
})
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawnTestContainer opens several host files (config/certs/keys) and passes them as Readers to testcontainers.ContainerFile, but they are never closed. Now that this function already registers a t.Cleanup, consider also closing those opened files (or defer-closing them right after opening) to avoid leaking file descriptors across the integration test suite.

Copilot uses AI. Check for mistakes.
@patrickhener patrickhener merged commit 637f162 into patrickhener:main Apr 27, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants