Add comprehensive test suite for file persistence and change tracking by grouville · Pull Request #65 · dagger/container-use

grouville · 2025-06-13T00:36:47Z

Summary

Comprehensive test suite to ensure reliable file persistence, change tracking, and environment isolation in container-use.

Key Improvements

Core Functionality Tests

Work Persistence: Files and changes persist across container restarts
Change Tracking: Automatic capture and auditing of all modifications
File Handling: Graceful handling of Python caches, binaries, and large files
Environment Isolation: Safe parallel operations with proper isolation
Configuration: Reliable persistence of base images and setup commands

Test Coverage

Git operations (error handling, worktrees, empty directories)
Selective staging for Python cache and binary files
Configuration persistence and isolation verification
Behavior-driven tests focused on user experience
Test helpers for isolated environment setup

Implementation Notes

Modified git.go to support test isolation via CONTAINER_USE_CONFIG_DIR
Documented known limitations with skipped tests
Added to GitHub workflows for CI/CD

Status

Ready for review

UPDATE 06/25/2025

Rewrote the entire test suite, following the new paradigm. The idea is to have integration tests that mimick what a user will do to end up in a bug / weird situation

Already ready for review

Introduced the Repository: basePath:

type Repository struct {
	userRepoPath string
	forkRepoPath string
	basePath     string // defaults to ~/.config/container-use if empty
}

instead of the CONTAINER_USE_CONFIG_DIR to isolate tests and run them in parallel

The WithRepository is a helper function that spins up Dagger -- a repo with some configurable setup function and returns a userActions, which has access to the same function as the agent would. The idea is for this abstraction to also be able to mimick the CLI calls in the near future to declaratively express any user scenario

TODO:

do not copy the mcp handler
cleanup a bit

cwlbraa · 2025-06-13T16:38:15Z

@@ -0,0 +1,210 @@
+package environment


the unit tests will conflict with #59, but not in a way that's hard to fix i don't think, just gotta move some shit around

aluzzardi

Thank you @grouville, much appreciated! Some minor comments

aluzzardi · 2025-06-13T22:41:58Z

@@ -0,0 +1,729 @@
+package environment


nit: MAYBE we should move these into their own package, e.g. environment/integration or something, keep the unit tests top-level. MAYBE. Thoughts?

imo for small projects the thing he's doing with -short is quite nice and provides a similar test grouping UX. i'd like to try it out and see if there are other drawbacks that aren't yet obvious to me... the one i can see from here is it might be too easy to share helpers that turn unit tests into int tests.

nit: MAYBE we should move these into their own package, e.g. environment/integration or something, keep the unit tests top-level. MAYBE. Thoughts?

Regarding the integration tests: I’ve kept them in the environment package because they need access to the private functions to thoroughly verify the implementation. Moving them to a separate package (like environment_test or environment/integration) would limit testing to the public API only, reducing coverage (or requiring to turn more of those functions public. What would be your recommended approach (I don't mind) ?

cwlbraa

lgtm, just need to decide whether this or #62 goes in first... conventional wisdom says tests go in first, but idk cc @aluzzardi

aluzzardi

LGTM!

Left a few minor comments.

Major comment here: I think the distinction between unit and integration tests is very narrow. IMHO we should just put everything as unit, e.g. environment/environment_test.go and repository/repository_test.go?

To me integration tests would be like ... testing cu with claude for instance.

In any case -- all of this can be discussed in follow ups, IMHO this is good to go aside from the dirname comment which we need to discuss

aluzzardi · 2025-06-25T21:26:12Z

+
+// getRepoPath returns the path for storing repository data
+func (r *Repository) getRepoPath() string {
+	return r.getBasePath() + "/repos"


nit: use filepath.Join

aluzzardi · 2025-06-25T21:26:38Z

+
+// getWorktreePath returns the path for storing worktrees
+func (r *Repository) getWorktreePath() string {
+	return r.getBasePath() + "/worktrees"


filepath.Join

aluzzardi · 2025-06-25T21:27:29Z

-		if strings.Contains(err.Error(), "not a git repository") {
+		// Check for exit code 128 which means not a git repository
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 {


For later as a follow-up: maybe we should add this logic to RunGitCommand directly, catch all git commands that are not run from a git repository?

Although I think it's impossible since everything starts with Open()

aluzzardi · 2025-06-25T21:34:40Z

+		if errors.As(err, &exitErr) && exitErr.ExitCode() == 2 {
+			// Exit code 2 means the remote doesn't exist
+			// Use just the base name for temporary directories to avoid deeply nested paths
+			return homedir.Expand(filepath.Join(r.getRepoPath(), filepath.Base(repo)))


So actually that was done on purpose -- we used to use the directory name, but that failed if multiple repositories had the same name on different directories (e.g. ~/work/foo and ~/Documents/foo).

Was there an issue this is fixing, or was it for aesthetics?

aluzzardi · 2025-06-25T21:39:38Z

+}
+
+// getBasePath returns the base path for container-use data, using the default if not set
+func (r *Repository) getBasePath() string {


nit: This logic is already handled by Open() (e.g. setting cuGlobalConfigPath when calling Open()).

I think it would be simpler to avoid duplication of this logic, always assume r.basePath is set, and get rid of getBasePath altogether

aluzzardi · 2025-06-25T21:44:11Z

@@ -0,0 +1,128 @@
+package integration


Shouldn't these go into repository/repository_test.go? We're testing Open over there and the rest here here

cwlbraa · 2025-06-25T21:51:21Z

To me integration tests would be like ... testing cu with claude for instance.

i'd prolly call tests involving claude e2e, but if we're integrating anything here it's probably the MCP server against repository+env

…al behaviors users rely on: * Persistence of work across sessions (files and changes remain intact through restarts). * Automatic change tracking and audit trails for effective debugging. * Graceful handling of problematic files, including Python cache, binary files, and large files. * Isolation of multiple environments to support safe parallel operations. * Reliable persistence of environment configuration (base images, setup commands). Detailed testing includes: * Git operations, specifically handling command errors, worktree paths, and empty directories. * Selective file staging to manage Python cache and binary files appropriately. * Verification of configuration persistence and environment isolation. Adopted a behavior-driven testing approach focusing on user-experienced behaviors rather than internal implementation details. Modified git.go to support test isolation via the `CONTAINER_USE_CONFIG_DIR` environment variable. Introduced test helpers to streamline the creation of isolated test environments. Documented known limitations clearly with skipped tests for currently unresolved issues related to Python cache, binary directories, and environment variables. Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

make it work Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

Updated the test to use commands that developers actually run: - Creating build directories (common in CI/CD) - Writing to build output logs (typical build process) - Creating coverage directories (test runners like pytest) These are more representative of real-world scenarios where commands produce no git-trackable changes but should still be audited. Related to issue dagger#82

The test now: - Simulates Python cache directories without needing Python installed - Verifies that development continues normally with __pycache__ present - Serves as a regression test for Python workflows - Renamed from PythonCache to PythonDevelopmentWorkflow for clarity The original bug appears to have been fixed - __pycache__ directories are properly ignored by git and don't interfere with operations.

Concurrency was properly implemented -- added a test showing that in a sequencial environment, every git operation is sequential and working Removed the impossible concurrency test Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

Signed-off-by: home <guillaume@dagger.io>

Parallelise the tests -- rebased on top of the new function schema to open and get repos TODO: cleanup a bit the code / but it's working ! Signed-off-by: home <guillaume@dagger.io>

Signed-off-by: home <guillaume@dagger.io>

grouville requested review from aluzzardi, cwlbraa and tiborvass June 13, 2025 00:36

grouville marked this pull request as ready for review June 13, 2025 00:44

cwlbraa reviewed Jun 13, 2025

View reviewed changes

Comment thread environment/test_helpers.go Outdated

cwlbraa reviewed Jun 13, 2025

View reviewed changes

Comment thread environment/README.md Outdated

grouville force-pushed the test-suite branch 2 times, most recently from dd281c8 to 88bc059 Compare June 13, 2025 22:35

aluzzardi reviewed Jun 13, 2025

View reviewed changes

grouville force-pushed the test-suite branch 2 times, most recently from 0a223b7 to 67a1630 Compare June 16, 2025 00:34

grouville requested review from aluzzardi and cwlbraa June 16, 2025 18:05

grouville force-pushed the test-suite branch 2 times, most recently from 4038593 to 6f4ec13 Compare June 16, 2025 18:33

cwlbraa reviewed Jun 16, 2025

View reviewed changes

Comment thread CONTRIBUTING.md

cwlbraa reviewed Jun 16, 2025

View reviewed changes

Comment thread .github/workflows/cu.yml Outdated

cwlbraa mentioned this pull request Jun 16, 2025

environment management overhaul #62

Merged

grouville force-pushed the test-suite branch from 6f4ec13 to 25bb201 Compare June 17, 2025 08:02

grouville requested a review from cwlbraa June 17, 2025 08:16

grouville changed the title ~~test: Ensure reliable preservation of user work and robust handling of edge cases~~ Add comprehensive test suite for file persistence and change tracking Jun 17, 2025

cwlbraa approved these changes Jun 17, 2025

View reviewed changes

grouville force-pushed the test-suite branch from a7352ce to 2b37923 Compare June 17, 2025 21:45

grouville mentioned this pull request Jun 17, 2025

fix: add NoCache option to host directory uploads #96

Closed

3 tasks

grouville force-pushed the test-suite branch 2 times, most recently from 1606741 to 1c023af Compare June 25, 2025 05:06

grouville requested a review from cwlbraa June 25, 2025 18:19

aluzzardi approved these changes Jun 25, 2025

View reviewed changes

cwlbraa approved these changes Jun 25, 2025

View reviewed changes

grouville force-pushed the test-suite branch from 1c023af to c7d8f4e Compare June 25, 2025 22:08

grouville added 11 commits June 25, 2025 15:15

test: improve clarity with some helpers and some refacto

7d25c32

make it work Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

chore: implement recommendations

4c207ed

Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

fix concurrent misunderstanding on my end

793b769

Concurrency was properly implemented -- added a test showing that in a sequencial environment, every git operation is sequential and working Removed the impossible concurrency test Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

chore: implement suggestions

f866c3d

Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

chore: just some more contextual comments

efabe2c

Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>

refacto

42f05b9

Signed-off-by: home <guillaume@dagger.io>

painful rebase once again 😇

13f4068

Parallelise the tests -- rebased on top of the new function schema to open and get repos TODO: cleanup a bit the code / but it's working ! Signed-off-by: home <guillaume@dagger.io>

keep using the full repo path in the fork subdir path

514def7

Signed-off-by: home <guillaume@dagger.io>

grouville force-pushed the test-suite branch from c7d8f4e to 514def7 Compare June 25, 2025 22:18

grouville merged commit 2b175c5 into dagger:main Jun 25, 2025
2 checks passed

grouville mentioned this pull request Jul 1, 2025

CONTAINER_USE_CONFIG_DIR should be owned by Repository/Environment instead of global state #95

Closed

Conversation

grouville commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Improvements

Core Functionality Tests

Test Coverage

Implementation Notes

Status

UPDATE 06/25/2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aluzzardi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grouville Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cwlbraa left a comment

Choose a reason for hiding this comment

Uh oh!

aluzzardi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwlbraa commented Jun 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grouville commented Jun 13, 2025 •

edited

Loading

grouville Jun 16, 2025 •

edited

Loading