ci: e2e tests are part of CI #15
Conversation
WalkthroughMigrates end-to-end tests from Python (pytest) to Go (testify suite pattern), introduces a GitHub Actions CI workflow for automated linting and testing, and adds a Makefile for development task automation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Key areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Makefile (1)
1-24: Well-structured Makefile for Go project automation.The targets align well with the CI workflow. A couple of observations:
- The
test-e2etarget doesn't include the-raceflag, unliketest. This is likely intentional for E2E tests (performance reasons), but worth confirming.- Consider adding a dependency on
buildfortest-e2eto ensure the binary is built before running E2E tests locally.🔎 Optional: Add build dependency for test-e2e
-test-e2e: +test-e2e: build go test -v ./tests/e2e/ -timeout 10m.github/workflows/ci.yaml (2)
28-31: Consider pinning golangci-lint version for reproducible builds.Using
version: latestmay cause unexpected CI failures when new linter rules are introduced. Pinning to a specific version ensures consistent behavior.🔎 Suggested fix
- name: Lint uses: golangci/golangci-lint-action@v6 with: - version: latest + version: v1.62
53-57: Minor inconsistency: git default branch config.The
init.defaultBranchis set tomaster, but the E2E tests explicitly usemainbranch (viagit branch -M main). While this works because the tests override the branch name, consider aligning the config with the actual branch used:🔎 Suggested fix for consistency
- name: Configure git run: | git config --global user.email "ci@test.test" git config --global user.name "CI" - git config --global init.defaultBranch master + git config --global init.defaultBranch maintests/e2e/e2e_test.go (4)
143-160: Consider preserving file permissions in copyDir.The helper uses hardcoded permissions (0755 for dirs, 0644 for files). If any fixture files require executable permissions, they'll be lost. For test fixtures this is likely acceptable, but worth noting.
🔎 Optional: Preserve file permissions
func (s *E2ESuite) copyDir(src, dst string) { entries, err := os.ReadDir(src) s.Require().NoError(err) for _, entry := range entries { srcPath := filepath.Join(src, entry.Name()) dstPath := filepath.Join(dst, entry.Name()) if entry.IsDir() { s.Require().NoError(os.MkdirAll(dstPath, 0755)) s.copyDir(srcPath, dstPath) } else { + info, err := entry.Info() + s.Require().NoError(err) data, err := os.ReadFile(srcPath) s.Require().NoError(err) - s.Require().NoError(os.WriteFile(dstPath, data, 0644)) + s.Require().NoError(os.WriteFile(dstPath, data, info.Mode())) } } }
223-234: Add timeout to HTTP client.
http.Getuses the default client with no timeout. If the service hangs on a request, the individual check could block longer than expected. Consider using a client with a short timeout.🔎 Suggested fix
func (s *E2ESuite) checkServiceResponse(url, expected string) bool { - resp, err := http.Get(url) + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Get(url) if err != nil { return false } defer resp.Body.Close() body, err := io.ReadAll(resp.Body) if err != nil { return false } return strings.Contains(string(body), expected) }
421-521: Test55 and Test57 share significant code duplication.These tests are nearly identical except for
removeVolumeFromDockerCompose()vsremoveBuildContextFromDockerCompose(). Consider refactoring to a table-driven test or extracting the common logic.
553-573: Test90_ProjectCleanup has redundant check.Lines 563-564 and 570-572 perform the same check (verifying project directory doesn't exist). The second check is redundant.
🔎 Suggested fix
func (s *E2ESuite) Test90_ProjectCleanup() { s.cleanupProject() // Initialize and start project s.devboxRun("init", s.manifestRepo, "--name", "test-app", "--branch", "main") s.devboxRun("up", "--name", "test-app") // Test project destroy s.devboxRun("destroy", "--name", "test-app") _, err := os.Stat(s.projectDir) s.True(os.IsNotExist(err), "Project directory should be removed") // Verify no containers out, _ := exec.Command("docker", "ps", "-q", "--filter", "name=test-app").Output() s.Empty(strings.TrimSpace(string(out)), "No containers should be running") - - // Make sure files were removed - _, err = os.Stat(s.projectDir) - s.True(os.IsNotExist(err), "Project files should be removed") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yaml(1 hunks)Makefile(1 hunks)tests/e2e/conftest.py(0 hunks)tests/e2e/e2e_test.go(1 hunks)tests/e2e/fixtures/manifest/docker-compose.yml(1 hunks)tests/e2e/helpers.py(0 hunks)tests/e2e/pytest.ini(0 hunks)tests/e2e/requirements.txt(0 hunks)tests/e2e/run_tests.sh(0 hunks)tests/e2e/test_10_init.py(0 hunks)tests/e2e/test_30_info.py(0 hunks)tests/e2e/test_40_project_context.py(0 hunks)tests/e2e/test_50_mount_context.py(0 hunks)tests/e2e/test_55_mount_for_build.py(0 hunks)tests/e2e/test_57_mount_for_volume.py(0 hunks)tests/e2e/test_60_operations.py(0 hunks)tests/e2e/test_90_cleanup.py(0 hunks)
💤 Files with no reviewable changes (13)
- tests/e2e/test_60_operations.py
- tests/e2e/conftest.py
- tests/e2e/run_tests.sh
- tests/e2e/test_40_project_context.py
- tests/e2e/pytest.ini
- tests/e2e/helpers.py
- tests/e2e/test_30_info.py
- tests/e2e/test_10_init.py
- tests/e2e/test_57_mount_for_volume.py
- tests/e2e/test_90_cleanup.py
- tests/e2e/requirements.txt
- tests/e2e/test_55_mount_for_build.py
- tests/e2e/test_50_mount_context.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests
🔇 Additional comments (7)
tests/e2e/fixtures/manifest/docker-compose.yml (1)
4-4: LGTM!The branch change to
mainis consistent with the Go E2E test suite setup that pushes to themainbranch..github/workflows/ci.yaml (1)
36-60: E2E job structure looks good.The job properly builds the binary, adds it to PATH, configures git for test repo operations, and runs tests with appropriate timeout.
tests/e2e/e2e_test.go (5)
16-28: Well-designed E2ESuite struct.The struct captures all necessary state for the test lifecycle. Good separation of concerns with dedicated fields for repos, directories, and fixtures.
34-109: SetupSuite is comprehensive and well-structured.Good practices observed:
- Cleanup of leftover temp directories from previous runs
- Proper git repo initialization with bare repos
- Clear separation between manifest and source repos
111-132: TearDownSuite handles cleanup properly.The empty string check on line 116 correctly handles the case where no containers exist.
236-256: Hardcoded YAML snippets are fragile but acceptable for tests.The string replacement approach for modifying docker-compose.yml relies on exact whitespace matching. Any change to the fixture file's formatting could break these helpers. This is a reasonable tradeoff for E2E test simplicity.
288-313: Good test structure with proper isolation.Each test starts with
cleanupProject()ensuring proper isolation. The numbered naming (Test10_, Test30_, etc.) ensures consistent execution order with testify's alphabetical sorting.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.