refactor: e2e tests in go#3
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request migrates end-to-end testing from shell scripts to Go's native testing framework. It introduces a comprehensive Go test suite with ~40 test scenarios, removes all Bash test runners and helpers, updates CI and build configurations to execute tests via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/e2e_test.go`:
- Around line 380-384: The test currently ignores the error from the wtw call
(wtw(t, repo, "create", "hook-fail-test") //nolint:errcheck), which allows hook
failures to be silently swallowed; change the test to capture the returned error
(or command result) from wtw and assert it indicates failure (e.g.,
require.Error/NotNil or check non‑zero exit code) before asserting filesystem
side effects; update references in this test to use the captured error variable
and assert the command failed for the "create" invocation that targets
"hook-fail-test" so hook failures are enforced.
- Around line 23-45: TestMain currently lets the real $HOME leak into the e2e
suite; create an isolated temp home directory at the start of TestMain (e.g.,
via os.MkdirTemp), set os.Setenv("HOME", tmpHome) before any tests/run or before
writeGlobalConfig is ever called so all subprocesses inherit it, and defer
cleanup: remove the temp dir and restore the original HOME after m.Run; this
ensures writeGlobalConfig and any wtw/git subprocesses operate against the
isolated HOME during tests.
- Around line 195-206: In TestCreate, the test hardcodes checkout to "main"
which is host-dependent; capture the repository's initial branch immediately
after initRepo (e.g., call/get the current branch name into a variable like
initialBranch) before switching to "feature-one", and then use that variable
(initialBranch) instead of the literal "main" when checking out back; update
references in TestCreate (and any helper usages) to use the captured initial
branch so the test works regardless of the git default branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54c7eabc-b4d7-405e-8e76-3150374261d1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
.github/workflows/ci.ymlMakefileREADME.mde2e/e2e_test.goe2e/helpers.she2e/run.she2e/setup.she2e/test_create.she2e/test_create_duplicate.she2e/test_create_new_branch.she2e/test_create_second.she2e/test_custom_naming.she2e/test_defaults.she2e/test_force_create.she2e/test_force_rm.she2e/test_help.she2e/test_hooks.she2e/test_hooks_fail.she2e/test_init_local.she2e/test_init_naming.she2e/test_list.she2e/test_no_sync.she2e/test_not_git_repo.she2e/test_rm.she2e/test_rm_custom_naming.she2e/test_rm_force.she2e/test_rm_force_custom_naming.she2e/test_rm_nonexistent.she2e/test_sync.shgo.modinternal/cmd/create.gointernal/cmd/rm.go
💤 Files with no reviewable changes (25)
- e2e/setup.sh
- e2e/test_no_sync.sh
- e2e/test_force_rm.sh
- e2e/test_not_git_repo.sh
- e2e/test_rm_nonexistent.sh
- e2e/run.sh
- e2e/test_force_create.sh
- e2e/test_create_duplicate.sh
- e2e/test_hooks_fail.sh
- e2e/test_sync.sh
- e2e/test_rm.sh
- e2e/test_defaults.sh
- e2e/test_rm_custom_naming.sh
- e2e/helpers.sh
- e2e/test_rm_force_custom_naming.sh
- e2e/test_create_new_branch.sh
- e2e/test_init_naming.sh
- e2e/test_create.sh
- e2e/test_rm_force.sh
- e2e/test_create_second.sh
- e2e/test_help.sh
- e2e/test_list.sh
- e2e/test_hooks.sh
- e2e/test_init_local.sh
- e2e/test_custom_naming.sh
Comment on lines
+23
to
+45
| func TestMain(m *testing.M) { | ||
| bin := os.Getenv("WTW") | ||
| if bin == "" { | ||
| bin = "../wtw" | ||
| } | ||
| if !filepath.IsAbs(bin) { | ||
| _, src, _, _ := runtime.Caller(0) | ||
| repoRoot := filepath.Dir(filepath.Dir(src)) | ||
| bin = filepath.Join(repoRoot, bin) | ||
| } | ||
| abs, err := filepath.Abs(bin) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| wtwBin = abs | ||
|
|
||
| code := m.Run() | ||
| if code == 0 { | ||
| fmt.Printf("\n%s%s✓ All e2e tests passed%s\n", colorBold, colorGreen, colorReset) | ||
| } else { | ||
| fmt.Printf("\n%s%s✗ E2E tests failed%s\n", colorBold, colorRed, colorReset) | ||
| } | ||
| os.Exit(code) |
There was a problem hiding this comment.
Isolate $HOME for the E2E suite.
writeGlobalConfig later writes to $HOME/.config/worktree-workflow/config.json, and every wtw/git subprocess inherits this environment. As written, go test ./e2e/... will overwrite the developer’s real config and leak ambient home-state into the suite.
Proposed fix
func TestMain(m *testing.M) {
+ testHome, err := os.MkdirTemp("", "wtw-e2e-home-*")
+ if err != nil {
+ panic(err)
+ }
+ if err := os.Setenv("HOME", testHome); err != nil {
+ panic(err)
+ }
+ if err := os.Setenv("XDG_CONFIG_HOME", filepath.Join(testHome, ".config")); err != nil {
+ panic(err)
+ }
+
bin := os.Getenv("WTW")
if bin == "" {
bin = "../wtw"
}
if !filepath.IsAbs(bin) {
@@
wtwBin = abs
code := m.Run()
+ _ = os.RemoveAll(testHome)
if code == 0 {
fmt.Printf("\n%s%s✓ All e2e tests passed%s\n", colorBold, colorGreen, colorReset)
} else {
fmt.Printf("\n%s%s✗ E2E tests failed%s\n", colorBold, colorRed, colorReset)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestMain(m *testing.M) { | |
| bin := os.Getenv("WTW") | |
| if bin == "" { | |
| bin = "../wtw" | |
| } | |
| if !filepath.IsAbs(bin) { | |
| _, src, _, _ := runtime.Caller(0) | |
| repoRoot := filepath.Dir(filepath.Dir(src)) | |
| bin = filepath.Join(repoRoot, bin) | |
| } | |
| abs, err := filepath.Abs(bin) | |
| if err != nil { | |
| panic(err) | |
| } | |
| wtwBin = abs | |
| code := m.Run() | |
| if code == 0 { | |
| fmt.Printf("\n%s%s✓ All e2e tests passed%s\n", colorBold, colorGreen, colorReset) | |
| } else { | |
| fmt.Printf("\n%s%s✗ E2E tests failed%s\n", colorBold, colorRed, colorReset) | |
| } | |
| os.Exit(code) | |
| func TestMain(m *testing.M) { | |
| testHome, err := os.MkdirTemp("", "wtw-e2e-home-*") | |
| if err != nil { | |
| panic(err) | |
| } | |
| if err := os.Setenv("HOME", testHome); err != nil { | |
| panic(err) | |
| } | |
| if err := os.Setenv("XDG_CONFIG_HOME", filepath.Join(testHome, ".config")); err != nil { | |
| panic(err) | |
| } | |
| bin := os.Getenv("WTW") | |
| if bin == "" { | |
| bin = "../wtw" | |
| } | |
| if !filepath.IsAbs(bin) { | |
| _, src, _, _ := runtime.Caller(0) | |
| repoRoot := filepath.Dir(filepath.Dir(src)) | |
| bin = filepath.Join(repoRoot, bin) | |
| } | |
| abs, err := filepath.Abs(bin) | |
| if err != nil { | |
| panic(err) | |
| } | |
| wtwBin = abs | |
| code := m.Run() | |
| _ = os.RemoveAll(testHome) | |
| if code == 0 { | |
| fmt.Printf("\n%s%s✓ All e2e tests passed%s\n", colorBold, colorGreen, colorReset) | |
| } else { | |
| fmt.Printf("\n%s%s✗ E2E tests failed%s\n", colorBold, colorRed, colorReset) | |
| } | |
| os.Exit(code) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/e2e_test.go` around lines 23 - 45, TestMain currently lets the real $HOME
leak into the e2e suite; create an isolated temp home directory at the start of
TestMain (e.g., via os.MkdirTemp), set os.Setenv("HOME", tmpHome) before any
tests/run or before writeGlobalConfig is ever called so all subprocesses inherit
it, and defer cleanup: remove the temp dir and restore the original HOME after
m.Run; this ensures writeGlobalConfig and any wtw/git subprocesses operate
against the isolated HOME during tests.
Comment on lines
+195
to
+206
| func TestCreate(t *testing.T) { | ||
| tmp := t.TempDir() | ||
| repo := filepath.Join(tmp, "test-project") | ||
| initRepo(t, repo, "feature-one") | ||
|
|
||
| gitCmd(t, repo, "checkout", "feature-one") | ||
| if err := os.WriteFile(filepath.Join(repo, "feature.js"), []byte("feature one code"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| gitCmd(t, repo, "add", "feature.js") | ||
| gitCmd(t, repo, "commit", "-m", "add feature one") | ||
| gitCmd(t, repo, "checkout", "main") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" -o -name "Makefile" -o -name "ci.yml" | head -20Repository: krystxf/worktree-workflow
Length of output: 532
🏁 Script executed:
cd . && git ls-files | grep -E "(e2e_test\.go|Makefile|ci\.yml)" | head -10Repository: krystxf/worktree-workflow
Length of output: 132
🏁 Script executed:
ls -laRepository: krystxf/worktree-workflow
Length of output: 1291
🏁 Script executed:
find . -type f \( -name "*.go" -o -name "Makefile" \) | grep -E "(e2e|Makefile)" | head -20Repository: krystxf/worktree-workflow
Length of output: 97
🏁 Script executed:
cat -n e2e/e2e_test.go | head -250Repository: krystxf/worktree-workflow
Length of output: 7956
🏁 Script executed:
cat -n MakefileRepository: krystxf/worktree-workflow
Length of output: 973
🏁 Script executed:
cat -n .github/workflows/ci.yml | head -120Repository: krystxf/worktree-workflow
Length of output: 3128
🏁 Script executed:
grep -n "checkout.*main" e2e/e2e_test.goRepository: krystxf/worktree-workflow
Length of output: 109
🏁 Script executed:
grep -n "currentBranch" e2e/e2e_test.goRepository: krystxf/worktree-workflow
Length of output: 233
🏁 Script executed:
cat -n e2e/e2e_test.go | sed -n '195,230p'Repository: krystxf/worktree-workflow
Length of output: 1470
Replace hardcoded main in TestCreate with captured initial branch.
The hardcoded main at line 206 is host-dependent because initRepo does not explicitly set the default branch. While CI (.github/workflows/ci.yml line 99) and make e2e-docker (Makefile line 28) force repos to start on main via git config --global init.defaultBranch main, plain make e2e does not, making the test fail on hosts with a different default branch.
Capture the starting branch before switching away, as shown below:
Suggested fix
func TestCreate(t *testing.T) {
tmp := t.TempDir()
repo := filepath.Join(tmp, "test-project")
initRepo(t, repo, "feature-one")
+ baseBranch := currentBranch(t, repo)
gitCmd(t, repo, "checkout", "feature-one")
if err := os.WriteFile(filepath.Join(repo, "feature.js"), []byte("feature one code"), 0o644); err != nil {
t.Fatal(err)
}
gitCmd(t, repo, "add", "feature.js")
gitCmd(t, repo, "commit", "-m", "add feature one")
- gitCmd(t, repo, "checkout", "main")
+ gitCmd(t, repo, "checkout", baseBranch)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/e2e_test.go` around lines 195 - 206, In TestCreate, the test hardcodes
checkout to "main" which is host-dependent; capture the repository's initial
branch immediately after initRepo (e.g., call/get the current branch name into a
variable like initialBranch) before switching to "feature-one", and then use
that variable (initialBranch) instead of the literal "main" when checking out
back; update references in TestCreate (and any helper usages) to use the
captured initial branch so the test works regardless of the git default branch.
Comment on lines
+380
to
+384
| wtw(t, repo, "create", "hook-fail-test") //nolint:errcheck | ||
|
|
||
| worktree := filepath.Join(tmp, "test-project--worktrees", "test-project--hook-fail-test") | ||
| assertFileExists(t, filepath.Join(worktree, "before-fail.txt")) | ||
| assertFileNotExists(t, filepath.Join(worktree, "after-fail.txt")) |
There was a problem hiding this comment.
Assert that hook failures return a non-zero command result.
Ignoring err weakens the migrated coverage: this test still passes if create starts swallowing hook failures but simply stops running later hooks. Assert the command failed, not just the filesystem side effects.
Proposed fix
- wtw(t, repo, "create", "hook-fail-test") //nolint:errcheck
+ out, err := wtw(t, repo, "create", "hook-fail-test")
+ if err == nil {
+ t.Fatalf("expected create to fail when a post_copy_hook exits non-zero\n%s", out)
+ }
worktree := filepath.Join(tmp, "test-project--worktrees", "test-project--hook-fail-test")
assertFileExists(t, filepath.Join(worktree, "before-fail.txt"))
assertFileNotExists(t, filepath.Join(worktree, "after-fail.txt"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wtw(t, repo, "create", "hook-fail-test") //nolint:errcheck | |
| worktree := filepath.Join(tmp, "test-project--worktrees", "test-project--hook-fail-test") | |
| assertFileExists(t, filepath.Join(worktree, "before-fail.txt")) | |
| assertFileNotExists(t, filepath.Join(worktree, "after-fail.txt")) | |
| out, err := wtw(t, repo, "create", "hook-fail-test") | |
| if err == nil { | |
| t.Fatalf("expected create to fail when a post_copy_hook exits non-zero\n%s", out) | |
| } | |
| worktree := filepath.Join(tmp, "test-project--worktrees", "test-project--hook-fail-test") | |
| assertFileExists(t, filepath.Join(worktree, "before-fail.txt")) | |
| assertFileNotExists(t, filepath.Join(worktree, "after-fail.txt")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/e2e_test.go` around lines 380 - 384, The test currently ignores the error
from the wtw call (wtw(t, repo, "create", "hook-fail-test") //nolint:errcheck),
which allows hook failures to be silently swallowed; change the test to capture
the returned error (or command result) from wtw and assert it indicates failure
(e.g., require.Error/NotNil or check non‑zero exit code) before asserting
filesystem side effects; update references in this test to use the captured
error variable and assert the command failed for the "create" invocation that
targets "hook-fail-test" so hook failures are enforced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.