From 8e6a31bc6ab43b23ab5c8abb7b213dde1f7157bf Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 17:39:44 +0200 Subject: [PATCH] Fix the teller's integration test taking its result twice: it hung the merge check (issue 1240, review round 1) --- .../mesh-delivery-teller/integration_test.go | 20 ++--- .../cmd/mesh-delivery-teller/runner_test.go | 75 +++++++++++++++++++ 2 files changed, 81 insertions(+), 14 deletions(-) create mode 100644 modules/mesh-delivery-teller/cmd/mesh-delivery-teller/runner_test.go diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/integration_test.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/integration_test.go index a33a3e7..046b7b8 100644 --- a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/integration_test.go +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/integration_test.go @@ -83,7 +83,7 @@ func TestTellerWaitsForTheGate(t *testing.T) { // 1240/AC2 if err != nil { t.Fatal(err) } - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() broker := filepath.Join(t.TempDir(), "broker") @@ -93,10 +93,8 @@ func TestTellerWaitsForTheGate(t *testing.T) { // 1240/AC2 } env := map[string]string{"MESH_BROKER_FILE": broker} logs := &syncBuffer{} - runCtx, stop := context.WithCancel(ctx) - done := make(chan error, 1) - go func() { done <- run(runCtx, func(k string) string { return env[k] }, logs) }() - t.Cleanup(func() { stop(); <-done }) + rn := startRunner(ctx, func(c context.Context) error { return run(c, func(k string) string { return env[k] }, logs) }) + t.Cleanup(func() { rn.finish(t, 10*time.Second) }) for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline) && !strings.Contains(logs.String(), "waiting for the gate's asks"); time.Sleep(20 * time.Millisecond) { } @@ -104,7 +102,7 @@ func TestTellerWaitsForTheGate(t *testing.T) { // 1240/AC2 t.Fatalf("the teller said it waits %d times, want once: %q", n, logs.String()) } select { - case err := <-done: + case err := <-rn.done: t.Fatalf("the teller stopped: %v", err) case <-time.After(500 * time.Millisecond): } @@ -119,13 +117,7 @@ func TestTellerWaitsForTheGate(t *testing.T) { // 1240/AC2 if _, err := js.KeyValue(ctx, "mesh-delivery-gate_asks"); err == nil { t.Error("the gate's asks bucket exists") } - stop() - select { - case err := <-done: - if err != nil { - t.Errorf("the teller stopped with %v, want a clean stop", err) - } - case <-time.After(5 * time.Second): - t.Error("the teller did not stop when asked") + if err := rn.finish(t, 10*time.Second); err != nil { + t.Errorf("the teller stopped with %v, want a clean stop", err) } } diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/runner_test.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/runner_test.go new file mode 100644 index 0000000..7e4899f --- /dev/null +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/runner_test.go @@ -0,0 +1,75 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" +) + +// runner runs the program's run function in a goroutine and stops it, bounded: a program that does not return when its +// context ends fails the test after a deadline with a message, and the result can be taken more than once (the +// integration test takes it in its body and again in its cleanup; a plain buffered channel of one value gave the second +// taker nothing, and the cleanup waited for ever: the merge check's 10-minute timeout). +type runner struct { + stop context.CancelFunc + done chan error +} + +func startRunner(parent context.Context, run func(context.Context) error) *runner { + ctx, stop := context.WithCancel(parent) + r := &runner{stop: stop, done: make(chan error, 1)} + go func() { + r.done <- run(ctx) + close(r.done) // a second taker reads the closed channel at once + }() + return r +} + +// finish stops the program and waits at most d for it to return. Called again, it returns at once. +func (r *runner) finish(t testing.TB, d time.Duration) error { + t.Helper() + r.stop() + select { + case err := <-r.done: + return err + case <-time.After(d): + t.Errorf("the program did not return within %s of its context ending", d) + return errors.New("did not return") + } +} + +func TestRunnerCanBeFinishedTwice(t *testing.T) { // 1240 (review round 1: body and cleanup both take the result) + r := startRunner(context.Background(), func(ctx context.Context) error { <-ctx.Done(); return nil }) + if err := r.finish(t, 2*time.Second); err != nil { + t.Fatal(err) + } + second := make(chan struct{}) + go func() { r.finish(t, 2*time.Second); close(second) }() + select { + case <-second: + case <-time.After(5 * time.Second): + t.Fatal("the second finish waits for ever: the result was taken once") + } +} + +func TestRunnerReportsAProgramThatDoesNotReturn(t *testing.T) { // 1240: a hang fails fast with a message + block := make(chan struct{}) + t.Cleanup(func() { close(block) }) + r := startRunner(context.Background(), func(context.Context) error { <-block; return nil }) + probe := &recorder{TB: t} + start := time.Now() + if err := r.finish(probe, 100*time.Millisecond); err == nil || !probe.failed { + t.Fatalf("a program that never returns was not reported (err %v)", err) + } + if time.Since(start) > 5*time.Second { + t.Fatal("the report was not prompt") + } +} + +type recorder struct { + testing.TB + failed bool +} + +func (r *recorder) Errorf(string, ...any) { r.failed = true }