refactor: simplify config lookup, git default and create errors; strengthen tests
- commit
- 4e14be1bfc1c4fa0c20a9ac9b915543164c6391e
- author
- brice · · 2026-10-10 06:41 UTC
- parent
- 763b41f
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
11 files · +242 -119
M cmd/ermite/main_test.go +92 -10
| @@ -86,6 +86,32 @@ func freeAddr(t *testing.T) string { | |||
| 86 | 86 | return addr | |
| 87 | 87 | } | |
| 88 | 88 | ||
| 89 | + | // unusedAddr is a valid address no test listens on: serve fails before it | |
| 90 | + | // listens when it is used. | |
| 91 | + | const unusedAddr = "127.0.0.1:1" | |
| 92 | + | ||
| 93 | + | // requireGit returns a git usable by ermite. Locally the test is skipped | |
| 94 | + | // without one; in CI (CI set) it fails, so that CI cannot pass vacuously. | |
| 95 | + | func requireGit(t *testing.T) string { | |
| 96 | + | t.Helper() | |
| 97 | + | unusable := func(format string, args ...any) { | |
| 98 | + | t.Helper() | |
| 99 | + | if os.Getenv("CI") != "" { | |
| 100 | + | t.Fatalf(format, args...) | |
| 101 | + | } | |
| 102 | + | t.Skipf(format, args...) | |
| 103 | + | } | |
| 104 | + | gitBin, err := exec.LookPath("git") | |
| 105 | + | if err != nil { | |
| 106 | + | unusable("git not found") | |
| 107 | + | } | |
| 108 | + | probe := gitcli.New(gitcli.Config{Bin: gitBin, DataDir: t.TempDir(), Path: os.Getenv("PATH")}) | |
| 109 | + | if _, err := probe.CheckVersion(context.Background()); err != nil { | |
| 110 | + | unusable("git is not usable by ermite: %v", err) | |
| 111 | + | } | |
| 112 | + | return gitBin | |
| 113 | + | } | |
| 114 | + | ||
| 89 | 115 | func baseEnv() []string { | |
| 90 | 116 | return []string{"PATH=" + os.Getenv("PATH")} | |
| 91 | 117 | } | |
| @@ -149,7 +175,7 @@ func TestServeGitTooOldNamesVariable(t *testing.T) { | |||
| 149 | 175 | "ERMITE_DATA_DIR="+data, | |
| 150 | 176 | "ERMITE_PUBLIC_URL=http://localhost", | |
| 151 | 177 | "ERMITE_ADMIN_TOKEN="+adminToken, | |
| 152 | - | "ERMITE_ADDR="+freeAddr(t), | |
| 178 | + | "ERMITE_ADDR="+unusedAddr, | |
| 153 | 179 | "ERMITE_GIT="+script, | |
| 154 | 180 | ) | |
| 155 | 181 | var out bytes.Buffer | |
| @@ -168,7 +194,7 @@ func TestServeMissingGitNamesVariable(t *testing.T) { | |||
| 168 | 194 | "ERMITE_DATA_DIR="+t.TempDir(), | |
| 169 | 195 | "ERMITE_PUBLIC_URL=http://localhost", | |
| 170 | 196 | "ERMITE_ADMIN_TOKEN="+adminToken, | |
| 171 | - | "ERMITE_ADDR="+freeAddr(t), | |
| 197 | + | "ERMITE_ADDR="+unusedAddr, | |
| 172 | 198 | "ERMITE_GIT=/nonexistent/git", | |
| 173 | 199 | ) | |
| 174 | 200 | var out bytes.Buffer | |
| @@ -181,14 +207,7 @@ func TestServeMissingGitNamesVariable(t *testing.T) { | |||
| 181 | 207 | } | |
| 182 | 208 | ||
| 183 | 209 | func TestServeHealthAndSigterm(t *testing.T) { | |
| 184 | - | gitBin, err := exec.LookPath("git") | |
| 185 | - | if err != nil { | |
| 186 | - | t.Skip("git not found") | |
| 187 | - | } | |
| 188 | - | probe := gitcli.New(gitcli.Config{Bin: gitBin, DataDir: t.TempDir(), Path: os.Getenv("PATH")}) | |
| 189 | - | if _, err := probe.CheckVersion(context.Background()); err != nil { | |
| 190 | - | t.Skipf("git is not usable by ermite: %v", err) | |
| 191 | - | } | |
| 210 | + | requireGit(t) | |
| 192 | 211 | data := t.TempDir() | |
| 193 | 212 | // State from a previous run: run/ is recreated, an orphan repository is | |
| 194 | 213 | // registered, a staging leftover is removed. | |
| @@ -274,3 +293,66 @@ func TestServeHealthAndSigterm(t *testing.T) { | |||
| 274 | 293 | _, err = http.Get(base + "/-/health") | |
| 275 | 294 | assert.Error(t, err, "the port is closed") | |
| 276 | 295 | } | |
| 296 | + | ||
| 297 | + | func TestSecondServeLeavesRunDirUntouched(t *testing.T) { | |
| 298 | + | requireGit(t) | |
| 299 | + | data := t.TempDir() | |
| 300 | + | addr := freeAddr(t) | |
| 301 | + | env := append(baseEnv(), | |
| 302 | + | "ERMITE_ADDR="+addr, | |
| 303 | + | "ERMITE_DATA_DIR="+data, | |
| 304 | + | "ERMITE_PUBLIC_URL=http://"+addr, | |
| 305 | + | "ERMITE_ADMIN_TOKEN="+adminToken, | |
| 306 | + | ) | |
| 307 | + | ||
| 308 | + | first := exec.Command(binary(t), "serve") | |
| 309 | + | first.Env = env | |
| 310 | + | var firstLogs syncBuffer | |
| 311 | + | first.Stdout, first.Stderr = &firstLogs, &firstLogs | |
| 312 | + | require.NoError(t, first.Start()) | |
| 313 | + | exited := make(chan error, 1) | |
| 314 | + | go func() { exited <- first.Wait() }() | |
| 315 | + | t.Cleanup(func() { _ = first.Process.Kill() }) | |
| 316 | + | ||
| 317 | + | deadline := time.Now().Add(15 * time.Second) | |
| 318 | + | for { | |
| 319 | + | resp, err := http.Get("http://" + addr + "/-/health") | |
| 320 | + | if err == nil { | |
| 321 | + | _ = resp.Body.Close() | |
| 322 | + | break | |
| 323 | + | } | |
| 324 | + | select { | |
| 325 | + | case werr := <-exited: | |
| 326 | + | t.Fatalf("first server exited: %v\n%s", werr, firstLogs.String()) | |
| 327 | + | default: | |
| 328 | + | } | |
| 329 | + | if time.Now().After(deadline) { | |
| 330 | + | t.Fatalf("health never answered: %v\n%s", err, firstLogs.String()) | |
| 331 | + | } | |
| 332 | + | time.Sleep(50 * time.Millisecond) | |
| 333 | + | } | |
| 334 | + | ||
| 335 | + | marker := filepath.Join(data, "run", "marker") | |
| 336 | + | require.NoError(t, os.WriteFile(marker, []byte("keep"), 0o600)) | |
| 337 | + | ||
| 338 | + | second := exec.Command(binary(t), "serve") | |
| 339 | + | second.Env = append(env[:len(env):len(env)], "ERMITE_ADDR="+freeAddr(t)) | |
| 340 | + | var out bytes.Buffer | |
| 341 | + | second.Stdout, second.Stderr = &out, &out | |
| 342 | + | err := second.Run() | |
| 343 | + | var exit *exec.ExitError | |
| 344 | + | require.True(t, errors.As(err, &exit), "second serve must fail, got %v\n%s", err, out.String()) | |
| 345 | + | assert.NotZero(t, exit.ExitCode()) | |
| 346 | + | ||
| 347 | + | got, rerr := os.ReadFile(marker) | |
| 348 | + | require.NoError(t, rerr, "run/ must be untouched by the second serve") | |
| 349 | + | assert.Equal(t, "keep", string(got)) | |
| 350 | + | assert.DirExists(t, filepath.Join(data, "run", "git-home")) | |
| 351 | + | ||
| 352 | + | require.NoError(t, first.Process.Signal(syscall.SIGTERM)) | |
| 353 | + | select { | |
| 354 | + | case <-exited: | |
| 355 | + | case <-time.After(15 * time.Second): | |
| 356 | + | t.Fatal("first server did not stop") | |
| 357 | + | } | |
| 358 | + | } | |
| @@ -86,6 +86,32 @@ func freeAddr(t *testing.T) string { | |||||
| 86 | return addr | 86 | return addr | ||
| 87 | } | 87 | } | ||
| 88 | 88 | ||||
| 89 | + | // unusedAddr is a valid address no test listens on: serve fails before it | |||
| 90 | + | // listens when it is used. | |||
| 91 | + | const unusedAddr = "127.0.0.1:1" | |||
| 92 | + | ||||
| 93 | + | // requireGit returns a git usable by ermite. Locally the test is skipped | |||
| 94 | + | // without one; in CI (CI set) it fails, so that CI cannot pass vacuously. | |||
| 95 | + | func requireGit(t *testing.T) string { | |||
| 96 | + | t.Helper() | |||
| 97 | + | unusable := func(format string, args ...any) { | |||
| 98 | + | t.Helper() | |||
| 99 | + | if os.Getenv("CI") != "" { | |||
| 100 | + | t.Fatalf(format, args...) | |||
| 101 | + | } | |||
| 102 | + | t.Skipf(format, args...) | |||
| 103 | + | } | |||
| 104 | + | gitBin, err := exec.LookPath("git") | |||
| 105 | + | if err != nil { | |||
| 106 | + | unusable("git not found") | |||
| 107 | + | } | |||
| 108 | + | probe := gitcli.New(gitcli.Config{Bin: gitBin, DataDir: t.TempDir(), Path: os.Getenv("PATH")}) | |||
| 109 | + | if _, err := probe.CheckVersion(context.Background()); err != nil { | |||
| 110 | + | unusable("git is not usable by ermite: %v", err) | |||
| 111 | + | } | |||
| 112 | + | return gitBin | |||
| 113 | + | } | |||
| 114 | + | ||||
| 89 | func baseEnv() []string { | 115 | func baseEnv() []string { | ||
| 90 | return []string{"PATH=" + os.Getenv("PATH")} | 116 | return []string{"PATH=" + os.Getenv("PATH")} | ||
| 91 | } | 117 | } | ||
| @@ -149,7 +175,7 @@ func TestServeGitTooOldNamesVariable(t *testing.T) { | |||||
| 149 | "ERMITE_DATA_DIR="+data, | 175 | "ERMITE_DATA_DIR="+data, | ||
| 150 | "ERMITE_PUBLIC_URL=http://localhost", | 176 | "ERMITE_PUBLIC_URL=http://localhost", | ||
| 151 | "ERMITE_ADMIN_TOKEN="+adminToken, | 177 | "ERMITE_ADMIN_TOKEN="+adminToken, | ||
| 152 | - | "ERMITE_ADDR="+freeAddr(t), | 178 | + | "ERMITE_ADDR="+unusedAddr, |
| 153 | "ERMITE_GIT="+script, | 179 | "ERMITE_GIT="+script, | ||
| 154 | ) | 180 | ) | ||
| 155 | var out bytes.Buffer | 181 | var out bytes.Buffer | ||
| @@ -168,7 +194,7 @@ func TestServeMissingGitNamesVariable(t *testing.T) { | |||||
| 168 | "ERMITE_DATA_DIR="+t.TempDir(), | 194 | "ERMITE_DATA_DIR="+t.TempDir(), | ||
| 169 | "ERMITE_PUBLIC_URL=http://localhost", | 195 | "ERMITE_PUBLIC_URL=http://localhost", | ||
| 170 | "ERMITE_ADMIN_TOKEN="+adminToken, | 196 | "ERMITE_ADMIN_TOKEN="+adminToken, | ||
| 171 | - | "ERMITE_ADDR="+freeAddr(t), | 197 | + | "ERMITE_ADDR="+unusedAddr, |
| 172 | "ERMITE_GIT=/nonexistent/git", | 198 | "ERMITE_GIT=/nonexistent/git", | ||
| 173 | ) | 199 | ) | ||
| 174 | var out bytes.Buffer | 200 | var out bytes.Buffer | ||
| @@ -181,14 +207,7 @@ func TestServeMissingGitNamesVariable(t *testing.T) { | |||||
| 181 | } | 207 | } | ||
| 182 | 208 | ||||
| 183 | func TestServeHealthAndSigterm(t *testing.T) { | 209 | func TestServeHealthAndSigterm(t *testing.T) { | ||
| 184 | - | gitBin, err := exec.LookPath("git") | 210 | + | requireGit(t) |
| 185 | - | if err != nil { | |||
| 186 | - | t.Skip("git not found") | |||
| 187 | - | } | |||
| 188 | - | probe := gitcli.New(gitcli.Config{Bin: gitBin, DataDir: t.TempDir(), Path: os.Getenv("PATH")}) | |||
| 189 | - | if _, err := probe.CheckVersion(context.Background()); err != nil { | |||
| 190 | - | t.Skipf("git is not usable by ermite: %v", err) | |||
| 191 | - | } | |||
| 192 | data := t.TempDir() | 211 | data := t.TempDir() | ||
| 193 | // State from a previous run: run/ is recreated, an orphan repository is | 212 | // State from a previous run: run/ is recreated, an orphan repository is | ||
| 194 | // registered, a staging leftover is removed. | 213 | // registered, a staging leftover is removed. | ||
| @@ -274,3 +293,66 @@ func TestServeHealthAndSigterm(t *testing.T) { | |||||
| 274 | _, err = http.Get(base + "/-/health") | 293 | _, err = http.Get(base + "/-/health") | ||
| 275 | assert.Error(t, err, "the port is closed") | 294 | assert.Error(t, err, "the port is closed") | ||
| 276 | } | 295 | } | ||
| 296 | + | ||||
| 297 | + | func TestSecondServeLeavesRunDirUntouched(t *testing.T) { | |||
| 298 | + | requireGit(t) | |||
| 299 | + | data := t.TempDir() | |||
| 300 | + | addr := freeAddr(t) | |||
| 301 | + | env := append(baseEnv(), | |||
| 302 | + | "ERMITE_ADDR="+addr, | |||
| 303 | + | "ERMITE_DATA_DIR="+data, | |||
| 304 | + | "ERMITE_PUBLIC_URL=http://"+addr, | |||
| 305 | + | "ERMITE_ADMIN_TOKEN="+adminToken, | |||
| 306 | + | ) | |||
| 307 | + | ||||
| 308 | + | first := exec.Command(binary(t), "serve") | |||
| 309 | + | first.Env = env | |||
| 310 | + | var firstLogs syncBuffer | |||
| 311 | + | first.Stdout, first.Stderr = &firstLogs, &firstLogs | |||
| 312 | + | require.NoError(t, first.Start()) | |||
| 313 | + | exited := make(chan error, 1) | |||
| 314 | + | go func() { exited <- first.Wait() }() | |||
| 315 | + | t.Cleanup(func() { _ = first.Process.Kill() }) | |||
| 316 | + | ||||
| 317 | + | deadline := time.Now().Add(15 * time.Second) | |||
| 318 | + | for { | |||
| 319 | + | resp, err := http.Get("http://" + addr + "/-/health") | |||
| 320 | + | if err == nil { | |||
| 321 | + | _ = resp.Body.Close() | |||
| 322 | + | break | |||
| 323 | + | } | |||
| 324 | + | select { | |||
| 325 | + | case werr := <-exited: | |||
| 326 | + | t.Fatalf("first server exited: %v\n%s", werr, firstLogs.String()) | |||
| 327 | + | default: | |||
| 328 | + | } | |||
| 329 | + | if time.Now().After(deadline) { | |||
| 330 | + | t.Fatalf("health never answered: %v\n%s", err, firstLogs.String()) | |||
| 331 | + | } | |||
| 332 | + | time.Sleep(50 * time.Millisecond) | |||
| 333 | + | } | |||
| 334 | + | ||||
| 335 | + | marker := filepath.Join(data, "run", "marker") | |||
| 336 | + | require.NoError(t, os.WriteFile(marker, []byte("keep"), 0o600)) | |||
| 337 | + | ||||
| 338 | + | second := exec.Command(binary(t), "serve") | |||
| 339 | + | second.Env = append(env[:len(env):len(env)], "ERMITE_ADDR="+freeAddr(t)) | |||
| 340 | + | var out bytes.Buffer | |||
| 341 | + | second.Stdout, second.Stderr = &out, &out | |||
| 342 | + | err := second.Run() | |||
| 343 | + | var exit *exec.ExitError | |||
| 344 | + | require.True(t, errors.As(err, &exit), "second serve must fail, got %v\n%s", err, out.String()) | |||
| 345 | + | assert.NotZero(t, exit.ExitCode()) | |||
| 346 | + | ||||
| 347 | + | got, rerr := os.ReadFile(marker) | |||
| 348 | + | require.NoError(t, rerr, "run/ must be untouched by the second serve") | |||
| 349 | + | assert.Equal(t, "keep", string(got)) | |||
| 350 | + | assert.DirExists(t, filepath.Join(data, "run", "git-home")) | |||
| 351 | + | ||||
| 352 | + | require.NoError(t, first.Process.Signal(syscall.SIGTERM)) | |||
| 353 | + | select { | |||
| 354 | + | case <-exited: | |||
| 355 | + | case <-time.After(15 * time.Second): | |||
| 356 | + | t.Fatal("first server did not stop") | |||
| 357 | + | } | |||
| 358 | + | } | |||
M cmd/ermite/serve.go +1 -1
| @@ -37,7 +37,7 @@ func newServeCmd() *cobra.Command { | |||
| 37 | 37 | ||
| 38 | 38 | // serve wires the adapters and runs the server until SIGINT or SIGTERM. | |
| 39 | 39 | func serve(ctx context.Context) error { | |
| 40 | - | cfg, err := config.Load(os.LookupEnv) | |
| 40 | + | cfg, err := config.Load(os.Getenv) | |
| 41 | 41 | if err != nil { | |
| 42 | 42 | return err | |
| 43 | 43 | } | |
| @@ -37,7 +37,7 @@ func newServeCmd() *cobra.Command { | |||||
| 37 | 37 | ||||
| 38 | // serve wires the adapters and runs the server until SIGINT or SIGTERM. | 38 | // serve wires the adapters and runs the server until SIGINT or SIGTERM. | ||
| 39 | func serve(ctx context.Context) error { | 39 | func serve(ctx context.Context) error { | ||
| 40 | - | cfg, err := config.Load(os.LookupEnv) | 40 | + | cfg, err := config.Load(os.Getenv) |
| 41 | if err != nil { | 41 | if err != nil { | ||
| 42 | return err | 42 | return err | ||
| 43 | } | 43 | } | ||
M internal/adapters/gitcli/git.go +2 -5
| @@ -20,6 +20,7 @@ import ( | |||
| 20 | 20 | // environment: the caller passes what it needs. | |
| 21 | 21 | type Config struct { | |
| 22 | 22 | // Bin is the git executable: a name looked up in the process PATH, or a path. | |
| 23 | + | // It is required; the default belongs to the configuration. | |
| 23 | 24 | Bin string | |
| 24 | 25 | // DataDir is the data root (absolute). repos/, repos/.staging/ and | |
| 25 | 26 | // trash/ below it must exist; run/git-home/ is the HOME of git. | |
| @@ -37,11 +38,7 @@ type Git struct { | |||
| 37 | 38 | ||
| 38 | 39 | // New builds an adapter. It does not run git: call CheckVersion. | |
| 39 | 40 | func New(cfg Config) *Git { | |
| 40 | - | bin := cfg.Bin | |
| 41 | - | if bin == "" { | |
| 42 | - | bin = "git" | |
| 43 | - | } | |
| 44 | - | return &Git{bin: bin, dataDir: cfg.DataDir, env: buildEnv(cfg.DataDir, cfg.Path)} | |
| 41 | + | return &Git{bin: cfg.Bin, dataDir: cfg.DataDir, env: buildEnv(cfg.DataDir, cfg.Path)} | |
| 45 | 42 | } | |
| 46 | 43 | ||
| 47 | 44 | // buildEnv returns the complete environment of every git invocation. Nothing | |
| @@ -20,6 +20,7 @@ import ( | |||||
| 20 | // environment: the caller passes what it needs. | 20 | // environment: the caller passes what it needs. | ||
| 21 | type Config struct { | 21 | type Config struct { | ||
| 22 | // Bin is the git executable: a name looked up in the process PATH, or a path. | 22 | // Bin is the git executable: a name looked up in the process PATH, or a path. | ||
| 23 | + | // It is required; the default belongs to the configuration. | |||
| 23 | Bin string | 24 | Bin string | ||
| 24 | // DataDir is the data root (absolute). repos/, repos/.staging/ and | 25 | // DataDir is the data root (absolute). repos/, repos/.staging/ and | ||
| 25 | // trash/ below it must exist; run/git-home/ is the HOME of git. | 26 | // trash/ below it must exist; run/git-home/ is the HOME of git. | ||
| @@ -37,11 +38,7 @@ type Git struct { | |||||
| 37 | 38 | ||||
| 38 | // New builds an adapter. It does not run git: call CheckVersion. | 39 | // New builds an adapter. It does not run git: call CheckVersion. | ||
| 39 | func New(cfg Config) *Git { | 40 | func New(cfg Config) *Git { | ||
| 40 | - | bin := cfg.Bin | 41 | + | return &Git{bin: cfg.Bin, dataDir: cfg.DataDir, env: buildEnv(cfg.DataDir, cfg.Path)} |
| 41 | - | if bin == "" { | |||
| 42 | - | bin = "git" | |||
| 43 | - | } | |||
| 44 | - | return &Git{bin: bin, dataDir: cfg.DataDir, env: buildEnv(cfg.DataDir, cfg.Path)} | |||
| 45 | } | 42 | } | ||
| 46 | 43 | ||||
| 47 | // buildEnv returns the complete environment of every git invocation. Nothing | 44 | // buildEnv returns the complete environment of every git invocation. Nothing | ||
M internal/adapters/gitcli/gitcli_test.go +22 -7
| @@ -59,18 +59,32 @@ func TestVersionLess(t *testing.T) { | |||
| 59 | 59 | assert.False(t, gitcli.Version{Major: 3}.Less(min)) | |
| 60 | 60 | } | |
| 61 | 61 | ||
| 62 | - | // newGit prepares a data dir and an adapter; it skips when git is missing. | |
| 62 | + | // unusable skips the test, or fails it in CI so that CI cannot pass vacuously. | |
| 63 | + | func unusable(t *testing.T, format string, args ...any) { | |
| 64 | + | t.Helper() | |
| 65 | + | if os.Getenv("CI") != "" { | |
| 66 | + | t.Fatalf(format, args...) | |
| 67 | + | } | |
| 68 | + | t.Skipf(format, args...) | |
| 69 | + | } | |
| 70 | + | ||
| 71 | + | // newGit prepares a data dir and an adapter. Without a usable git (2.45 or | |
| 72 | + | // newer) it skips locally and fails when CI is set. | |
| 63 | 73 | func newGit(t *testing.T) (*gitcli.Git, string) { | |
| 64 | 74 | t.Helper() | |
| 65 | 75 | bin, err := exec.LookPath("git") | |
| 66 | 76 | if err != nil { | |
| 67 | - | t.Skip("git not found") | |
| 77 | + | unusable(t, "git not found") | |
| 68 | 78 | } | |
| 69 | 79 | data := t.TempDir() | |
| 70 | 80 | for _, d := range []string{"repos/.staging", "trash", "run/git-home", "kv"} { | |
| 71 | 81 | require.NoError(t, os.MkdirAll(filepath.Join(data, d), 0o700)) | |
| 72 | 82 | } | |
| 73 | - | return gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}), data | |
| 83 | + | g := gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}) | |
| 84 | + | if _, err := g.CheckVersion(context.Background()); err != nil { | |
| 85 | + | unusable(t, "git is not usable by ermite: %v", err) | |
| 86 | + | } | |
| 87 | + | return g, data | |
| 74 | 88 | } | |
| 75 | 89 | ||
| 76 | 90 | func name(t *testing.T, s string) domain.RepoName { | |
| @@ -97,7 +111,7 @@ func writeScript(t *testing.T, body string) string { | |||
| 97 | 111 | func TestCheckVersionRejects(t *testing.T) { | |
| 98 | 112 | ctx := context.Background() | |
| 99 | 113 | data := t.TempDir() | |
| 100 | - | cfg := func(bin string) gitcli.Config { return gitcli.Config{Bin: bin, DataDir: data, Path: "/usr/bin:/bin"} } | |
| 114 | + | cfg := func(bin string) gitcli.Config { return gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")} } | |
| 101 | 115 | ||
| 102 | 116 | t.Run("too old", func(t *testing.T) { | |
| 103 | 117 | _, err := gitcli.New(cfg(writeScript(t, `echo "git version 2.44.9"`))).CheckVersion(ctx) | |
| @@ -131,7 +145,7 @@ func TestIsolatedEnvironment(t *testing.T) { | |||
| 131 | 145 | t.Setenv("XDG_CONFIG_HOME", "/leak") | |
| 132 | 146 | t.Setenv("SECRET_TOKEN", "leak") | |
| 133 | 147 | ||
| 134 | - | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: "/usr/bin:/bin"}) | |
| 148 | + | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: os.Getenv("PATH")}) | |
| 135 | 149 | _, err := g.CheckVersion(ctx) | |
| 136 | 150 | require.NoError(t, err) | |
| 137 | 151 | ||
| @@ -145,7 +159,8 @@ func TestIsolatedEnvironment(t *testing.T) { | |||
| 145 | 159 | assert.Equal(t, "1", got["GIT_CONFIG_NOSYSTEM"]) | |
| 146 | 160 | assert.Equal(t, filepath.Join(data, "run", "git-home"), got["HOME"]) | |
| 147 | 161 | assert.Equal(t, "0", got["GIT_TERMINAL_PROMPT"]) | |
| 148 | - | assert.Equal(t, "/usr/bin:/bin", got["PATH"]) | |
| 162 | + | assert.Equal(t, os.Getenv("PATH"), got["PATH"]) | |
| 163 | + | assert.Equal(t, "C", got["LC_ALL"]) | |
| 149 | 164 | for k, v := range got { | |
| 150 | 165 | assert.NotContains(t, v, "leak", k) | |
| 151 | 166 | switch k { | |
| @@ -265,7 +280,7 @@ func TestStageFailureLeavesNothing(t *testing.T) { | |||
| 265 | 280 | require.NoError(t, os.MkdirAll(filepath.Join(data, "repos", ".staging"), 0o700)) | |
| 266 | 281 | // A git that creates the directory and then fails. | |
| 267 | 282 | script := writeScript(t, `for last; do :; done; mkdir -p "$last"; echo nope >&2; exit 1`) | |
| 268 | - | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: "/usr/bin:/bin"}) | |
| 283 | + | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: os.Getenv("PATH")}) | |
| 269 | 284 | _, err := g.Stage(ctx) | |
| 270 | 285 | require.Error(t, err) | |
| 271 | 286 | entries, rerr := os.ReadDir(filepath.Join(data, "repos", ".staging")) | |
| @@ -59,18 +59,32 @@ func TestVersionLess(t *testing.T) { | |||||
| 59 | assert.False(t, gitcli.Version{Major: 3}.Less(min)) | 59 | assert.False(t, gitcli.Version{Major: 3}.Less(min)) | ||
| 60 | } | 60 | } | ||
| 61 | 61 | ||||
| 62 | - | // newGit prepares a data dir and an adapter; it skips when git is missing. | 62 | + | // unusable skips the test, or fails it in CI so that CI cannot pass vacuously. |
| 63 | + | func unusable(t *testing.T, format string, args ...any) { | |||
| 64 | + | t.Helper() | |||
| 65 | + | if os.Getenv("CI") != "" { | |||
| 66 | + | t.Fatalf(format, args...) | |||
| 67 | + | } | |||
| 68 | + | t.Skipf(format, args...) | |||
| 69 | + | } | |||
| 70 | + | ||||
| 71 | + | // newGit prepares a data dir and an adapter. Without a usable git (2.45 or | |||
| 72 | + | // newer) it skips locally and fails when CI is set. | |||
| 63 | func newGit(t *testing.T) (*gitcli.Git, string) { | 73 | func newGit(t *testing.T) (*gitcli.Git, string) { | ||
| 64 | t.Helper() | 74 | t.Helper() | ||
| 65 | bin, err := exec.LookPath("git") | 75 | bin, err := exec.LookPath("git") | ||
| 66 | if err != nil { | 76 | if err != nil { | ||
| 67 | - | t.Skip("git not found") | 77 | + | unusable(t, "git not found") |
| 68 | } | 78 | } | ||
| 69 | data := t.TempDir() | 79 | data := t.TempDir() | ||
| 70 | for _, d := range []string{"repos/.staging", "trash", "run/git-home", "kv"} { | 80 | for _, d := range []string{"repos/.staging", "trash", "run/git-home", "kv"} { | ||
| 71 | require.NoError(t, os.MkdirAll(filepath.Join(data, d), 0o700)) | 81 | require.NoError(t, os.MkdirAll(filepath.Join(data, d), 0o700)) | ||
| 72 | } | 82 | } | ||
| 73 | - | return gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}), data | 83 | + | g := gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}) |
| 84 | + | if _, err := g.CheckVersion(context.Background()); err != nil { | |||
| 85 | + | unusable(t, "git is not usable by ermite: %v", err) | |||
| 86 | + | } | |||
| 87 | + | return g, data | |||
| 74 | } | 88 | } | ||
| 75 | 89 | ||||
| 76 | func name(t *testing.T, s string) domain.RepoName { | 90 | func name(t *testing.T, s string) domain.RepoName { | ||
| @@ -97,7 +111,7 @@ func writeScript(t *testing.T, body string) string { | |||||
| 97 | func TestCheckVersionRejects(t *testing.T) { | 111 | func TestCheckVersionRejects(t *testing.T) { | ||
| 98 | ctx := context.Background() | 112 | ctx := context.Background() | ||
| 99 | data := t.TempDir() | 113 | data := t.TempDir() | ||
| 100 | - | cfg := func(bin string) gitcli.Config { return gitcli.Config{Bin: bin, DataDir: data, Path: "/usr/bin:/bin"} } | 114 | + | cfg := func(bin string) gitcli.Config { return gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")} } |
| 101 | 115 | ||||
| 102 | t.Run("too old", func(t *testing.T) { | 116 | t.Run("too old", func(t *testing.T) { | ||
| 103 | _, err := gitcli.New(cfg(writeScript(t, `echo "git version 2.44.9"`))).CheckVersion(ctx) | 117 | _, err := gitcli.New(cfg(writeScript(t, `echo "git version 2.44.9"`))).CheckVersion(ctx) | ||
| @@ -131,7 +145,7 @@ func TestIsolatedEnvironment(t *testing.T) { | |||||
| 131 | t.Setenv("XDG_CONFIG_HOME", "/leak") | 145 | t.Setenv("XDG_CONFIG_HOME", "/leak") | ||
| 132 | t.Setenv("SECRET_TOKEN", "leak") | 146 | t.Setenv("SECRET_TOKEN", "leak") | ||
| 133 | 147 | ||||
| 134 | - | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: "/usr/bin:/bin"}) | 148 | + | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: os.Getenv("PATH")}) |
| 135 | _, err := g.CheckVersion(ctx) | 149 | _, err := g.CheckVersion(ctx) | ||
| 136 | require.NoError(t, err) | 150 | require.NoError(t, err) | ||
| 137 | 151 | ||||
| @@ -145,7 +159,8 @@ func TestIsolatedEnvironment(t *testing.T) { | |||||
| 145 | assert.Equal(t, "1", got["GIT_CONFIG_NOSYSTEM"]) | 159 | assert.Equal(t, "1", got["GIT_CONFIG_NOSYSTEM"]) | ||
| 146 | assert.Equal(t, filepath.Join(data, "run", "git-home"), got["HOME"]) | 160 | assert.Equal(t, filepath.Join(data, "run", "git-home"), got["HOME"]) | ||
| 147 | assert.Equal(t, "0", got["GIT_TERMINAL_PROMPT"]) | 161 | assert.Equal(t, "0", got["GIT_TERMINAL_PROMPT"]) | ||
| 148 | - | assert.Equal(t, "/usr/bin:/bin", got["PATH"]) | 162 | + | assert.Equal(t, os.Getenv("PATH"), got["PATH"]) |
| 163 | + | assert.Equal(t, "C", got["LC_ALL"]) | |||
| 149 | for k, v := range got { | 164 | for k, v := range got { | ||
| 150 | assert.NotContains(t, v, "leak", k) | 165 | assert.NotContains(t, v, "leak", k) | ||
| 151 | switch k { | 166 | switch k { | ||
| @@ -265,7 +280,7 @@ func TestStageFailureLeavesNothing(t *testing.T) { | |||||
| 265 | require.NoError(t, os.MkdirAll(filepath.Join(data, "repos", ".staging"), 0o700)) | 280 | require.NoError(t, os.MkdirAll(filepath.Join(data, "repos", ".staging"), 0o700)) | ||
| 266 | // A git that creates the directory and then fails. | 281 | // A git that creates the directory and then fails. | ||
| 267 | script := writeScript(t, `for last; do :; done; mkdir -p "$last"; echo nope >&2; exit 1`) | 282 | script := writeScript(t, `for last; do :; done; mkdir -p "$last"; echo nope >&2; exit 1`) | ||
| 268 | - | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: "/usr/bin:/bin"}) | 283 | + | g := gitcli.New(gitcli.Config{Bin: script, DataDir: data, Path: os.Getenv("PATH")}) |
| 269 | _, err := g.Stage(ctx) | 284 | _, err := g.Stage(ctx) | ||
| 270 | require.Error(t, err) | 285 | require.Error(t, err) | ||
| 271 | entries, rerr := os.ReadDir(filepath.Join(data, "repos", ".staging")) | 286 | entries, rerr := os.ReadDir(filepath.Join(data, "repos", ".staging")) | ||
M internal/app/repos.go +1 -1
| @@ -84,7 +84,7 @@ func (r *Repos) Create(ctx context.Context, p domain.Principal, params CreatePar | |||
| 84 | 84 | } | |
| 85 | 85 | r.discard(cleanup, id) | |
| 86 | 86 | if errors.Is(err, domain.ErrAlreadyExists) { | |
| 87 | - | return domain.Repo{}, fmt.Errorf("repository %q: %w", repo.Name, domain.ErrAlreadyExists) | |
| 87 | + | return domain.Repo{}, fmt.Errorf("repository %q: directory %w", repo.Name, domain.ErrAlreadyExists) | |
| 88 | 88 | } | |
| 89 | 89 | return domain.Repo{}, fmt.Errorf("publish repository %q: %w", repo.Name, err) | |
| 90 | 90 | } | |
| @@ -84,7 +84,7 @@ func (r *Repos) Create(ctx context.Context, p domain.Principal, params CreatePar | |||||
| 84 | } | 84 | } | ||
| 85 | r.discard(cleanup, id) | 85 | r.discard(cleanup, id) | ||
| 86 | if errors.Is(err, domain.ErrAlreadyExists) { | 86 | if errors.Is(err, domain.ErrAlreadyExists) { | ||
| 87 | - | return domain.Repo{}, fmt.Errorf("repository %q: %w", repo.Name, domain.ErrAlreadyExists) | 87 | + | return domain.Repo{}, fmt.Errorf("repository %q: directory %w", repo.Name, domain.ErrAlreadyExists) |
| 88 | } | 88 | } | ||
| 89 | return domain.Repo{}, fmt.Errorf("publish repository %q: %w", repo.Name, err) | 89 | return domain.Repo{}, fmt.Errorf("publish repository %q: %w", repo.Name, err) | ||
| 90 | } | 90 | } | ||
A internal/app/repos_integration_test.go +105 -0
| @@ -0,0 +1,105 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | package app_test | |
| 4 | + | ||
| 5 | + | import ( | |
| 6 | + | "context" | |
| 7 | + | "errors" | |
| 8 | + | "os" | |
| 9 | + | "os/exec" | |
| 10 | + | "path/filepath" | |
| 11 | + | "sync" | |
| 12 | + | "testing" | |
| 13 | + | "time" | |
| 14 | + | ||
| 15 | + | "github.com/stretchr/testify/assert" | |
| 16 | + | "github.com/stretchr/testify/require" | |
| 17 | + | ||
| 18 | + | "bnema.dev/ermite/internal/adapters/gitcli" | |
| 19 | + | "bnema.dev/ermite/internal/adapters/kvstore" | |
| 20 | + | "bnema.dev/ermite/internal/app" | |
| 21 | + | "bnema.dev/ermite/internal/domain" | |
| 22 | + | ) | |
| 23 | + | ||
| 24 | + | type fixedClock struct{} | |
| 25 | + | ||
| 26 | + | func (fixedClock) Now() time.Time { return now } | |
| 27 | + | ||
| 28 | + | // Concurrent Create of one name over the real adapters: one wins, the others | |
| 29 | + | // get ErrAlreadyExists and leave nothing behind. | |
| 30 | + | func TestCreateConcurrentSameNameRealAdapters(t *testing.T) { | |
| 31 | + | ctx := context.Background() | |
| 32 | + | bin, err := exec.LookPath("git") | |
| 33 | + | if err != nil { | |
| 34 | + | unusable(t, "git not found") | |
| 35 | + | } | |
| 36 | + | data := t.TempDir() | |
| 37 | + | for _, d := range []string{"repos/.staging", "trash", "run/git-home", "kv"} { | |
| 38 | + | require.NoError(t, os.MkdirAll(filepath.Join(data, d), 0o700)) | |
| 39 | + | } | |
| 40 | + | git := gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}) | |
| 41 | + | if _, err := git.CheckVersion(ctx); err != nil { | |
| 42 | + | unusable(t, "git is not usable by ermite: %v", err) | |
| 43 | + | } | |
| 44 | + | store, err := kvstore.Open(ctx, filepath.Join(data, "kv")) | |
| 45 | + | require.NoError(t, err) | |
| 46 | + | t.Cleanup(func() { require.NoError(t, store.Close(context.Background())) }) | |
| 47 | + | ||
| 48 | + | repos := app.NewRepos(store, git, fixedClock{}) | |
| 49 | + | n := name(t, "tools") | |
| 50 | + | ||
| 51 | + | const workers = 8 | |
| 52 | + | errs := make(chan error, workers) | |
| 53 | + | var wg sync.WaitGroup | |
| 54 | + | for range workers { | |
| 55 | + | wg.Add(1) | |
| 56 | + | go func() { | |
| 57 | + | defer wg.Done() | |
| 58 | + | _, err := repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |
| 59 | + | errs <- err | |
| 60 | + | }() | |
| 61 | + | } | |
| 62 | + | wg.Wait() | |
| 63 | + | close(errs) | |
| 64 | + | ||
| 65 | + | ok, exists := 0, 0 | |
| 66 | + | for err := range errs { | |
| 67 | + | switch { | |
| 68 | + | case err == nil: | |
| 69 | + | ok++ | |
| 70 | + | case errors.Is(err, domain.ErrAlreadyExists): | |
| 71 | + | exists++ | |
| 72 | + | default: | |
| 73 | + | t.Fatalf("unexpected error: %v", err) | |
| 74 | + | } | |
| 75 | + | } | |
| 76 | + | assert.Equal(t, 1, ok) | |
| 77 | + | assert.Equal(t, workers-1, exists) | |
| 78 | + | ||
| 79 | + | entries, err := os.ReadDir(filepath.Join(data, "repos")) | |
| 80 | + | require.NoError(t, err) | |
| 81 | + | var dirs []string | |
| 82 | + | for _, e := range entries { | |
| 83 | + | if e.Name() != ".staging" { | |
| 84 | + | dirs = append(dirs, e.Name()) | |
| 85 | + | } | |
| 86 | + | } | |
| 87 | + | assert.Equal(t, []string{"tools.git"}, dirs) | |
| 88 | + | ||
| 89 | + | all, err := store.List(ctx) | |
| 90 | + | require.NoError(t, err) | |
| 91 | + | assert.Len(t, all, 1) | |
| 92 | + | ||
| 93 | + | staging, err := os.ReadDir(filepath.Join(data, "repos", ".staging")) | |
| 94 | + | require.NoError(t, err) | |
| 95 | + | assert.Empty(t, staging) | |
| 96 | + | } | |
| 97 | + | ||
| 98 | + | // unusable skips the test, or fails it in CI so that CI cannot pass vacuously. | |
| 99 | + | func unusable(t *testing.T, format string, args ...any) { | |
| 100 | + | t.Helper() | |
| 101 | + | if os.Getenv("CI") != "" { | |
| 102 | + | t.Fatalf(format, args...) | |
| 103 | + | } | |
| 104 | + | t.Skipf(format, args...) | |
| 105 | + | } | |
| @@ -0,0 +1,105 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | package app_test | |||
| 4 | + | ||||
| 5 | + | import ( | |||
| 6 | + | "context" | |||
| 7 | + | "errors" | |||
| 8 | + | "os" | |||
| 9 | + | "os/exec" | |||
| 10 | + | "path/filepath" | |||
| 11 | + | "sync" | |||
| 12 | + | "testing" | |||
| 13 | + | "time" | |||
| 14 | + | ||||
| 15 | + | "github.com/stretchr/testify/assert" | |||
| 16 | + | "github.com/stretchr/testify/require" | |||
| 17 | + | ||||
| 18 | + | "bnema.dev/ermite/internal/adapters/gitcli" | |||
| 19 | + | "bnema.dev/ermite/internal/adapters/kvstore" | |||
| 20 | + | "bnema.dev/ermite/internal/app" | |||
| 21 | + | "bnema.dev/ermite/internal/domain" | |||
| 22 | + | ) | |||
| 23 | + | ||||
| 24 | + | type fixedClock struct{} | |||
| 25 | + | ||||
| 26 | + | func (fixedClock) Now() time.Time { return now } | |||
| 27 | + | ||||
| 28 | + | // Concurrent Create of one name over the real adapters: one wins, the others | |||
| 29 | + | // get ErrAlreadyExists and leave nothing behind. | |||
| 30 | + | func TestCreateConcurrentSameNameRealAdapters(t *testing.T) { | |||
| 31 | + | ctx := context.Background() | |||
| 32 | + | bin, err := exec.LookPath("git") | |||
| 33 | + | if err != nil { | |||
| 34 | + | unusable(t, "git not found") | |||
| 35 | + | } | |||
| 36 | + | data := t.TempDir() | |||
| 37 | + | for _, d := range []string{"repos/.staging", "trash", "run/git-home", "kv"} { | |||
| 38 | + | require.NoError(t, os.MkdirAll(filepath.Join(data, d), 0o700)) | |||
| 39 | + | } | |||
| 40 | + | git := gitcli.New(gitcli.Config{Bin: bin, DataDir: data, Path: os.Getenv("PATH")}) | |||
| 41 | + | if _, err := git.CheckVersion(ctx); err != nil { | |||
| 42 | + | unusable(t, "git is not usable by ermite: %v", err) | |||
| 43 | + | } | |||
| 44 | + | store, err := kvstore.Open(ctx, filepath.Join(data, "kv")) | |||
| 45 | + | require.NoError(t, err) | |||
| 46 | + | t.Cleanup(func() { require.NoError(t, store.Close(context.Background())) }) | |||
| 47 | + | ||||
| 48 | + | repos := app.NewRepos(store, git, fixedClock{}) | |||
| 49 | + | n := name(t, "tools") | |||
| 50 | + | ||||
| 51 | + | const workers = 8 | |||
| 52 | + | errs := make(chan error, workers) | |||
| 53 | + | var wg sync.WaitGroup | |||
| 54 | + | for range workers { | |||
| 55 | + | wg.Add(1) | |||
| 56 | + | go func() { | |||
| 57 | + | defer wg.Done() | |||
| 58 | + | _, err := repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |||
| 59 | + | errs <- err | |||
| 60 | + | }() | |||
| 61 | + | } | |||
| 62 | + | wg.Wait() | |||
| 63 | + | close(errs) | |||
| 64 | + | ||||
| 65 | + | ok, exists := 0, 0 | |||
| 66 | + | for err := range errs { | |||
| 67 | + | switch { | |||
| 68 | + | case err == nil: | |||
| 69 | + | ok++ | |||
| 70 | + | case errors.Is(err, domain.ErrAlreadyExists): | |||
| 71 | + | exists++ | |||
| 72 | + | default: | |||
| 73 | + | t.Fatalf("unexpected error: %v", err) | |||
| 74 | + | } | |||
| 75 | + | } | |||
| 76 | + | assert.Equal(t, 1, ok) | |||
| 77 | + | assert.Equal(t, workers-1, exists) | |||
| 78 | + | ||||
| 79 | + | entries, err := os.ReadDir(filepath.Join(data, "repos")) | |||
| 80 | + | require.NoError(t, err) | |||
| 81 | + | var dirs []string | |||
| 82 | + | for _, e := range entries { | |||
| 83 | + | if e.Name() != ".staging" { | |||
| 84 | + | dirs = append(dirs, e.Name()) | |||
| 85 | + | } | |||
| 86 | + | } | |||
| 87 | + | assert.Equal(t, []string{"tools.git"}, dirs) | |||
| 88 | + | ||||
| 89 | + | all, err := store.List(ctx) | |||
| 90 | + | require.NoError(t, err) | |||
| 91 | + | assert.Len(t, all, 1) | |||
| 92 | + | ||||
| 93 | + | staging, err := os.ReadDir(filepath.Join(data, "repos", ".staging")) | |||
| 94 | + | require.NoError(t, err) | |||
| 95 | + | assert.Empty(t, staging) | |||
| 96 | + | } | |||
| 97 | + | ||||
| 98 | + | // unusable skips the test, or fails it in CI so that CI cannot pass vacuously. | |||
| 99 | + | func unusable(t *testing.T, format string, args ...any) { | |||
| 100 | + | t.Helper() | |||
| 101 | + | if os.Getenv("CI") != "" { | |||
| 102 | + | t.Fatalf(format, args...) | |||
| 103 | + | } | |||
| 104 | + | t.Skipf(format, args...) | |||
| 105 | + | } | |||
M internal/app/repos_test.go +12 -78
| @@ -5,7 +5,6 @@ package app_test | |||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | 7 | "errors" | |
| 8 | - | "sync" | |
| 9 | 8 | "testing" | |
| 10 | 9 | "time" | |
| 11 | 10 | ||
| @@ -98,6 +97,18 @@ func TestCreate(t *testing.T) { | |||
| 98 | 97 | assert.ErrorIs(t, err, domain.ErrAlreadyExists) | |
| 99 | 98 | }) | |
| 100 | 99 | ||
| 100 | + | t.Run("publish conflict removes metadata and staging", func(t *testing.T) { | |
| 101 | + | f := newFixture(t) | |
| 102 | + | n := name(t, "tools") | |
| 103 | + | f.git.EXPECT().Stage(ctx).Return(ports.StagedID("s1"), nil) | |
| 104 | + | f.meta.EXPECT().Create(ctx, mock.Anything).Return(nil) | |
| 105 | + | f.git.EXPECT().Publish(ctx, ports.StagedID("s1"), n).Return(domain.ErrAlreadyExists) | |
| 106 | + | f.meta.EXPECT().Delete(mock.Anything, n).Return(nil) | |
| 107 | + | f.git.EXPECT().Discard(mock.Anything, ports.StagedID("s1")).Return(nil) | |
| 108 | + | _, err := f.repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |
| 109 | + | assert.ErrorIs(t, err, domain.ErrAlreadyExists) | |
| 110 | + | }) | |
| 111 | + | ||
| 101 | 112 | t.Run("failed publish removes metadata and staging", func(t *testing.T) { | |
| 102 | 113 | f := newFixture(t) | |
| 103 | 114 | n := name(t, "tools") | |
| @@ -132,83 +143,6 @@ func TestCreate(t *testing.T) { | |||
| 132 | 143 | }) | |
| 133 | 144 | } | |
| 134 | 145 | ||
| 135 | - | // A fake store that enforces absence, to model concurrent creates. | |
| 136 | - | type memMeta struct { | |
| 137 | - | mu sync.Mutex | |
| 138 | - | m map[domain.RepoName]domain.Repo | |
| 139 | - | } | |
| 140 | - | ||
| 141 | - | func (s *memMeta) create(repo domain.Repo) error { | |
| 142 | - | s.mu.Lock() | |
| 143 | - | defer s.mu.Unlock() | |
| 144 | - | if _, ok := s.m[repo.Name]; ok { | |
| 145 | - | return domain.ErrAlreadyExists | |
| 146 | - | } | |
| 147 | - | s.m[repo.Name] = repo | |
| 148 | - | return nil | |
| 149 | - | } | |
| 150 | - | ||
| 151 | - | func TestCreateConcurrentSameName(t *testing.T) { | |
| 152 | - | ctx := context.Background() | |
| 153 | - | n := name(t, "tools") | |
| 154 | - | store := &memMeta{m: map[domain.RepoName]domain.Repo{}} | |
| 155 | - | ||
| 156 | - | f := newFixture(t) | |
| 157 | - | var mu sync.Mutex | |
| 158 | - | staged, discarded, published := 0, 0, 0 | |
| 159 | - | f.git.EXPECT().Stage(mock.Anything).RunAndReturn(func(context.Context) (ports.StagedID, error) { | |
| 160 | - | mu.Lock() | |
| 161 | - | defer mu.Unlock() | |
| 162 | - | staged++ | |
| 163 | - | return ports.StagedID(string(rune('a' + staged))), nil | |
| 164 | - | }) | |
| 165 | - | f.meta.EXPECT().Create(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, r domain.Repo) error { | |
| 166 | - | return store.create(r) | |
| 167 | - | }) | |
| 168 | - | f.git.EXPECT().Publish(mock.Anything, mock.Anything, n).RunAndReturn(func(context.Context, ports.StagedID, domain.RepoName) error { | |
| 169 | - | mu.Lock() | |
| 170 | - | defer mu.Unlock() | |
| 171 | - | published++ | |
| 172 | - | return nil | |
| 173 | - | }) | |
| 174 | - | f.git.EXPECT().Discard(mock.Anything, mock.Anything).RunAndReturn(func(context.Context, ports.StagedID) error { | |
| 175 | - | mu.Lock() | |
| 176 | - | defer mu.Unlock() | |
| 177 | - | discarded++ | |
| 178 | - | return nil | |
| 179 | - | }) | |
| 180 | - | ||
| 181 | - | const workers = 8 | |
| 182 | - | errs := make(chan error, workers) | |
| 183 | - | var wg sync.WaitGroup | |
| 184 | - | for range workers { | |
| 185 | - | wg.Add(1) | |
| 186 | - | go func() { | |
| 187 | - | defer wg.Done() | |
| 188 | - | _, err := f.repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |
| 189 | - | errs <- err | |
| 190 | - | }() | |
| 191 | - | } | |
| 192 | - | wg.Wait() | |
| 193 | - | close(errs) | |
| 194 | - | ||
| 195 | - | ok, exists := 0, 0 | |
| 196 | - | for err := range errs { | |
| 197 | - | switch { | |
| 198 | - | case err == nil: | |
| 199 | - | ok++ | |
| 200 | - | case errors.Is(err, domain.ErrAlreadyExists): | |
| 201 | - | exists++ | |
| 202 | - | default: | |
| 203 | - | t.Fatalf("unexpected error: %v", err) | |
| 204 | - | } | |
| 205 | - | } | |
| 206 | - | assert.Equal(t, 1, ok) | |
| 207 | - | assert.Equal(t, workers-1, exists) | |
| 208 | - | assert.Equal(t, 1, published) | |
| 209 | - | assert.Equal(t, workers-1, discarded) | |
| 210 | - | } | |
| 211 | - | ||
| 212 | 146 | func TestGetAndListAccess(t *testing.T) { | |
| 213 | 147 | ctx := context.Background() | |
| 214 | 148 | pub := domain.Repo{Name: name(t, "pub"), Visibility: domain.VisibilityPublic} | |
| @@ -5,7 +5,6 @@ package app_test | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | "errors" | 7 | "errors" | ||
| 8 | - | "sync" | |||
| 9 | "testing" | 8 | "testing" | ||
| 10 | "time" | 9 | "time" | ||
| 11 | 10 | ||||
| @@ -98,6 +97,18 @@ func TestCreate(t *testing.T) { | |||||
| 98 | assert.ErrorIs(t, err, domain.ErrAlreadyExists) | 97 | assert.ErrorIs(t, err, domain.ErrAlreadyExists) | ||
| 99 | }) | 98 | }) | ||
| 100 | 99 | ||||
| 100 | + | t.Run("publish conflict removes metadata and staging", func(t *testing.T) { | |||
| 101 | + | f := newFixture(t) | |||
| 102 | + | n := name(t, "tools") | |||
| 103 | + | f.git.EXPECT().Stage(ctx).Return(ports.StagedID("s1"), nil) | |||
| 104 | + | f.meta.EXPECT().Create(ctx, mock.Anything).Return(nil) | |||
| 105 | + | f.git.EXPECT().Publish(ctx, ports.StagedID("s1"), n).Return(domain.ErrAlreadyExists) | |||
| 106 | + | f.meta.EXPECT().Delete(mock.Anything, n).Return(nil) | |||
| 107 | + | f.git.EXPECT().Discard(mock.Anything, ports.StagedID("s1")).Return(nil) | |||
| 108 | + | _, err := f.repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |||
| 109 | + | assert.ErrorIs(t, err, domain.ErrAlreadyExists) | |||
| 110 | + | }) | |||
| 111 | + | ||||
| 101 | t.Run("failed publish removes metadata and staging", func(t *testing.T) { | 112 | t.Run("failed publish removes metadata and staging", func(t *testing.T) { | ||
| 102 | f := newFixture(t) | 113 | f := newFixture(t) | ||
| 103 | n := name(t, "tools") | 114 | n := name(t, "tools") | ||
| @@ -132,83 +143,6 @@ func TestCreate(t *testing.T) { | |||||
| 132 | }) | 143 | }) | ||
| 133 | } | 144 | } | ||
| 134 | 145 | ||||
| 135 | - | // A fake store that enforces absence, to model concurrent creates. | |||
| 136 | - | type memMeta struct { | |||
| 137 | - | mu sync.Mutex | |||
| 138 | - | m map[domain.RepoName]domain.Repo | |||
| 139 | - | } | |||
| 140 | - | ||||
| 141 | - | func (s *memMeta) create(repo domain.Repo) error { | |||
| 142 | - | s.mu.Lock() | |||
| 143 | - | defer s.mu.Unlock() | |||
| 144 | - | if _, ok := s.m[repo.Name]; ok { | |||
| 145 | - | return domain.ErrAlreadyExists | |||
| 146 | - | } | |||
| 147 | - | s.m[repo.Name] = repo | |||
| 148 | - | return nil | |||
| 149 | - | } | |||
| 150 | - | ||||
| 151 | - | func TestCreateConcurrentSameName(t *testing.T) { | |||
| 152 | - | ctx := context.Background() | |||
| 153 | - | n := name(t, "tools") | |||
| 154 | - | store := &memMeta{m: map[domain.RepoName]domain.Repo{}} | |||
| 155 | - | ||||
| 156 | - | f := newFixture(t) | |||
| 157 | - | var mu sync.Mutex | |||
| 158 | - | staged, discarded, published := 0, 0, 0 | |||
| 159 | - | f.git.EXPECT().Stage(mock.Anything).RunAndReturn(func(context.Context) (ports.StagedID, error) { | |||
| 160 | - | mu.Lock() | |||
| 161 | - | defer mu.Unlock() | |||
| 162 | - | staged++ | |||
| 163 | - | return ports.StagedID(string(rune('a' + staged))), nil | |||
| 164 | - | }) | |||
| 165 | - | f.meta.EXPECT().Create(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, r domain.Repo) error { | |||
| 166 | - | return store.create(r) | |||
| 167 | - | }) | |||
| 168 | - | f.git.EXPECT().Publish(mock.Anything, mock.Anything, n).RunAndReturn(func(context.Context, ports.StagedID, domain.RepoName) error { | |||
| 169 | - | mu.Lock() | |||
| 170 | - | defer mu.Unlock() | |||
| 171 | - | published++ | |||
| 172 | - | return nil | |||
| 173 | - | }) | |||
| 174 | - | f.git.EXPECT().Discard(mock.Anything, mock.Anything).RunAndReturn(func(context.Context, ports.StagedID) error { | |||
| 175 | - | mu.Lock() | |||
| 176 | - | defer mu.Unlock() | |||
| 177 | - | discarded++ | |||
| 178 | - | return nil | |||
| 179 | - | }) | |||
| 180 | - | ||||
| 181 | - | const workers = 8 | |||
| 182 | - | errs := make(chan error, workers) | |||
| 183 | - | var wg sync.WaitGroup | |||
| 184 | - | for range workers { | |||
| 185 | - | wg.Add(1) | |||
| 186 | - | go func() { | |||
| 187 | - | defer wg.Done() | |||
| 188 | - | _, err := f.repos.Create(ctx, domain.Root(), app.CreateParams{Name: n}) | |||
| 189 | - | errs <- err | |||
| 190 | - | }() | |||
| 191 | - | } | |||
| 192 | - | wg.Wait() | |||
| 193 | - | close(errs) | |||
| 194 | - | ||||
| 195 | - | ok, exists := 0, 0 | |||
| 196 | - | for err := range errs { | |||
| 197 | - | switch { | |||
| 198 | - | case err == nil: | |||
| 199 | - | ok++ | |||
| 200 | - | case errors.Is(err, domain.ErrAlreadyExists): | |||
| 201 | - | exists++ | |||
| 202 | - | default: | |||
| 203 | - | t.Fatalf("unexpected error: %v", err) | |||
| 204 | - | } | |||
| 205 | - | } | |||
| 206 | - | assert.Equal(t, 1, ok) | |||
| 207 | - | assert.Equal(t, workers-1, exists) | |||
| 208 | - | assert.Equal(t, 1, published) | |||
| 209 | - | assert.Equal(t, workers-1, discarded) | |||
| 210 | - | } | |||
| 211 | - | ||||
| 212 | func TestGetAndListAccess(t *testing.T) { | 146 | func TestGetAndListAccess(t *testing.T) { | ||
| 213 | ctx := context.Background() | 147 | ctx := context.Background() | ||
| 214 | pub := domain.Repo{Name: name(t, "pub"), Visibility: domain.VisibilityPublic} | 148 | pub := domain.Repo{Name: name(t, "pub"), Visibility: domain.VisibilityPublic} | ||
M internal/config/config.go +2 -6
| @@ -81,14 +81,10 @@ func (e *Error) Error() string { | |||
| 81 | 81 | } | |
| 82 | 82 | ||
| 83 | 83 | // Load reads the configuration through lookup, which returns the value of a | |
| 84 | - | // variable and whether it is set (os.LookupEnv fits). An empty value counts as | |
| 84 | + | // variable, or "" when it is unset (os.Getenv fits). An empty value counts as | |
| 85 | 85 | // unset. All problems are reported together in an *Error. Messages name the | |
| 86 | 86 | // variable and never contain the admin token. | |
| 87 | - | func Load(lookup func(string) (string, bool)) (Config, error) { | |
| 88 | - | get := func(key string) string { | |
| 89 | - | v, _ := lookup(key) | |
| 90 | - | return v | |
| 91 | - | } | |
| 87 | + | func Load(get func(string) string) (Config, error) { | |
| 92 | 88 | var problems []string | |
| 93 | 89 | bad := func(key, format string, args ...any) { | |
| 94 | 90 | problems = append(problems, key+": "+fmt.Sprintf(format, args...)) | |
| @@ -81,14 +81,10 @@ func (e *Error) Error() string { | |||||
| 81 | } | 81 | } | ||
| 82 | 82 | ||||
| 83 | // Load reads the configuration through lookup, which returns the value of a | 83 | // Load reads the configuration through lookup, which returns the value of a | ||
| 84 | - | // variable and whether it is set (os.LookupEnv fits). An empty value counts as | 84 | + | // variable, or "" when it is unset (os.Getenv fits). An empty value counts as |
| 85 | // unset. All problems are reported together in an *Error. Messages name the | 85 | // unset. All problems are reported together in an *Error. Messages name the | ||
| 86 | // variable and never contain the admin token. | 86 | // variable and never contain the admin token. | ||
| 87 | - | func Load(lookup func(string) (string, bool)) (Config, error) { | 87 | + | func Load(get func(string) string) (Config, error) { |
| 88 | - | get := func(key string) string { | |||
| 89 | - | v, _ := lookup(key) | |||
| 90 | - | return v | |||
| 91 | - | } | |||
| 92 | var problems []string | 88 | var problems []string | ||
| 93 | bad := func(key, format string, args ...any) { | 89 | bad := func(key, format string, args ...any) { | ||
| 94 | problems = append(problems, key+": "+fmt.Sprintf(format, args...)) | 90 | problems = append(problems, key+": "+fmt.Sprintf(format, args...)) | ||
M internal/config/config_test.go +2 -5
| @@ -17,11 +17,8 @@ import ( | |||
| 17 | 17 | ||
| 18 | 18 | const token = "0123456789abcdef0123456789abcdef" // 32 chars | |
| 19 | 19 | ||
| 20 | - | func lookup(env map[string]string) func(string) (string, bool) { | |
| 21 | - | return func(k string) (string, bool) { | |
| 22 | - | v, ok := env[k] | |
| 23 | - | return v, ok | |
| 24 | - | } | |
| 20 | + | func lookup(env map[string]string) func(string) string { | |
| 21 | + | return func(k string) string { return env[k] } | |
| 25 | 22 | } | |
| 26 | 23 | ||
| 27 | 24 | func valid(overrides map[string]string) map[string]string { | |
| @@ -17,11 +17,8 @@ import ( | |||||
| 17 | 17 | ||||
| 18 | const token = "0123456789abcdef0123456789abcdef" // 32 chars | 18 | const token = "0123456789abcdef0123456789abcdef" // 32 chars | ||
| 19 | 19 | ||||
| 20 | - | func lookup(env map[string]string) func(string) (string, bool) { | 20 | + | func lookup(env map[string]string) func(string) string { |
| 21 | - | return func(k string) (string, bool) { | 21 | + | return func(k string) string { return env[k] } |
| 22 | - | v, ok := env[k] | |||
| 23 | - | return v, ok | |||
| 24 | - | } | |||
| 25 | } | 22 | } | ||
| 26 | 23 | ||||
| 27 | func valid(overrides map[string]string) map[string]string { | 24 | func valid(overrides map[string]string) map[string]string { | ||
M internal/domain/name_test.go +0 -5
| @@ -91,11 +91,6 @@ func TestParseRepoPath(t *testing.T) { | |||
| 91 | 91 | } | |
| 92 | 92 | } | |
| 93 | 93 | ||
| 94 | - | func TestParseRepoNameDoesNotStripGit(t *testing.T) { | |
| 95 | - | _, err := domain.ParseRepoName("tools.git") | |
| 96 | - | assert.ErrorIs(t, err, domain.ErrInvalidName) | |
| 97 | - | } | |
| 98 | - | ||
| 99 | 94 | func TestParseVisibility(t *testing.T) { | |
| 100 | 95 | for in, want := range map[string]domain.Visibility{"public": domain.VisibilityPublic, "private": domain.VisibilityPrivate} { | |
| 101 | 96 | got, err := domain.ParseVisibility(in) | |
| @@ -91,11 +91,6 @@ func TestParseRepoPath(t *testing.T) { | |||||
| 91 | } | 91 | } | ||
| 92 | } | 92 | } | ||
| 93 | 93 | ||||
| 94 | - | func TestParseRepoNameDoesNotStripGit(t *testing.T) { | |||
| 95 | - | _, err := domain.ParseRepoName("tools.git") | |||
| 96 | - | assert.ErrorIs(t, err, domain.ErrInvalidName) | |||
| 97 | - | } | |||
| 98 | - | ||||
| 99 | func TestParseVisibility(t *testing.T) { | 94 | func TestParseVisibility(t *testing.T) { | ||
| 100 | for in, want := range map[string]domain.Visibility{"public": domain.VisibilityPublic, "private": domain.VisibilityPrivate} { | 95 | for in, want := range map[string]domain.Visibility{"public": domain.VisibilityPublic, "private": domain.VisibilityPrivate} { | ||
| 101 | got, err := domain.ParseVisibility(in) | 96 | got, err := domain.ParseVisibility(in) | ||
M internal/ports/ports.go +3 -1
| @@ -23,7 +23,9 @@ type RepoMetaStore interface { | |||
| 23 | 23 | Get(ctx context.Context, name domain.RepoName) (domain.Repo, error) | |
| 24 | 24 | // List returns every repository ordered by name. | |
| 25 | 25 | List(ctx context.Context) ([]domain.Repo, error) | |
| 26 | - | // Update replaces the stored repository, or returns domain.ErrNotFound. | |
| 26 | + | // Update replaces the stored repository. It returns domain.ErrNotFound if | |
| 27 | + | // there is none and domain.ErrConflict if another write changed the | |
| 28 | + | // stored repository since it was read. | |
| 27 | 29 | Update(ctx context.Context, repo domain.Repo) error | |
| 28 | 30 | // Delete removes the repository. Deleting a missing one is not an error. | |
| 29 | 31 | Delete(ctx context.Context, name domain.RepoName) error | |
| @@ -23,7 +23,9 @@ type RepoMetaStore interface { | |||||
| 23 | Get(ctx context.Context, name domain.RepoName) (domain.Repo, error) | 23 | Get(ctx context.Context, name domain.RepoName) (domain.Repo, error) | ||
| 24 | // List returns every repository ordered by name. | 24 | // List returns every repository ordered by name. | ||
| 25 | List(ctx context.Context) ([]domain.Repo, error) | 25 | List(ctx context.Context) ([]domain.Repo, error) | ||
| 26 | - | // Update replaces the stored repository, or returns domain.ErrNotFound. | 26 | + | // Update replaces the stored repository. It returns domain.ErrNotFound if |
| 27 | + | // there is none and domain.ErrConflict if another write changed the | |||
| 28 | + | // stored repository since it was read. | |||
| 27 | Update(ctx context.Context, repo domain.Repo) error | 29 | Update(ctx context.Context, repo domain.Repo) error | ||
| 28 | // Delete removes the repository. Deleting a missing one is not an error. | 30 | // Delete removes the repository. Deleting a missing one is not an error. | ||
| 29 | Delete(ctx context.Context, name domain.RepoName) error | 31 | Delete(ctx context.Context, name domain.RepoName) error | ||