From b1bd1d97cfa2939b7716936b49ada97f7ce23ee9 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 11 Nov 2026 18:15:10 +0000 Subject: [PATCH] Make two tests independent of host load (closes #256) The home-card refresh test runs keepHomeCardsFresh in a synctest bubble, so its waits are exact on a fake clock instead of 15-second polls. The geo fallback subtest in which geojs never answers used a 50 ms time limit and counted requests at the fake server, which a slow host could miss. It now uses a transport that counts each request as it is sent and holds it until the time limit ends it. Model: opus-5-5 --- TODO.md | 4 ++ internal/geo/locator_internal_test.go | 40 +++++++++------ internal/server/home_internal_test.go | 71 +++++++++++++++++---------- 3 files changed, 72 insertions(+), 43 deletions(-) diff --git a/TODO.md b/TODO.md index a461ce7..6dd87e1 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,10 @@ File an issue for each problem the run finds. # Completed Steps +- 2026-11-11: two more tests no longer fail on a busy host. The test of the home + page's card refresh runs on a fake clock. The geo test in which geojs never + answers counts each request as it is sent, not when it reaches the server + (closes https://git.eeqj.de/sneak/routewatch/issues/256). - 2026-11-10: `github.com/mattn/go-sqlite3` is at v1.14.53, so building on the host no longer prints its two C compiler warnings (closes https://git.eeqj.de/sneak/routewatch/issues/245). diff --git a/internal/geo/locator_internal_test.go b/internal/geo/locator_internal_test.go index 17cc853..0839cd1 100644 --- a/internal/geo/locator_internal_test.go +++ b/internal/geo/locator_internal_test.go @@ -90,14 +90,12 @@ func parseAddresses(t *testing.T, addresses ...string) []netip.Addr { return addrs } -// fakeGeoJS is a geojs server. It sends body, or status when that is set, or -// waits for the request to end when hang is true. It keeps each request's -// address list. +// fakeGeoJS is a geojs server. It sends body, or status when that is set. It +// keeps each request's address list. type fakeGeoJS struct { server *httptest.Server body string status int - hang bool mu sync.Mutex requests []string @@ -119,12 +117,6 @@ func (f *fakeGeoJS) serve(w http.ResponseWriter, r *http.Request) { f.requests = append(f.requests, r.URL.Query().Get("ip")) f.mu.Unlock() - if f.hang { - <-r.Context().Done() - - return - } - if f.status != 0 { w.WriteHeader(f.status) @@ -142,6 +134,20 @@ func (f *fakeGeoJS) requestsSeen() []string { return append([]string{}, f.requests...) } +// hangingTransport never answers: each request waits until its context ends. +// It counts the requests as they are sent, so the count does not depend on +// whether one reaches a server before its time limit runs out. +type hangingTransport struct { + requests atomic.Int32 +} + +func (h *hangingTransport) RoundTrip(r *http.Request) (*http.Response, error) { + h.requests.Add(1) + <-r.Context().Done() + + return nil, r.Context().Err() +} + // openCacheDB opens a new cache.db in a temporary directory. func openCacheDB(t *testing.T, dir string) *sql.DB { t.Helper() @@ -385,13 +391,15 @@ func TestLookupFallsBackToTheDatabase(t *testing.T) { fake := newFakeGeoJS(t, tt.body) fake.status = tt.status - fake.hang = tt.hang dir := t.TempDir() locator := newTestLocator(t, fake, openCacheDB(t, dir), dir) - locator.timeLimit = shortTimeLimit + requestCount := func() int { return len(fake.requestsSeen()) } - if !tt.hang { - locator.timeLimit = testTimeLimit + if tt.hang { + hanging := &hangingTransport{} + locator.client = &http.Client{Transport: hanging} + locator.timeLimit = shortTimeLimit + requestCount = func() int { return int(hanging.requests.Load()) } } giveDatabase(t, locator) @@ -413,8 +421,8 @@ func TestLookupFallsBackToTheDatabase(t *testing.T) { // asks geojs again. lookupOne(t, locator, testAddress) - if requests := fake.requestsSeen(); len(requests) != 2 { - t.Errorf("%d requests, want 2", len(requests)) + if requests := requestCount(); requests != 2 { + t.Errorf("%d requests, want 2", requests) } }) } diff --git a/internal/server/home_internal_test.go b/internal/server/home_internal_test.go index 40f1b01..0cf7162 100644 --- a/internal/server/home_internal_test.go +++ b/internal/server/home_internal_test.go @@ -275,49 +275,66 @@ func TestPagesServeTheCopyOfTheCards(t *testing.T) { } } -// TestKeepHomeCardsFreshWorksThemOutEveryInterval runs keepHomeCardsFresh with -// a short interval. It works the cards out at once, again after the table -// changes, and stops when its context ends. +// TestKeepHomeCardsFreshWorksThemOutEveryInterval runs keepHomeCardsFresh. It +// works the cards out at once, again an interval after the table changes, and +// stops when its context ends. It runs in a synctest bubble, whose clock is +// fake, so a stall on the host cannot fail it. func TestKeepHomeCardsFreshWorksThemOutEveryInterval(t *testing.T) { t.Parallel() - const ( - interval = 10 * time.Millisecond - maxWait = 15 * time.Second - ) + const interval = time.Minute table := emptyTable() announce(table, "1.0.0.0/24", twoPrefixASN, testPeer(1), testRouteTime(0)) s := newServerWithTable(t, table) - ctx, cancel := context.WithCancel(t.Context()) - stopped := make(chan struct{}) - - go func() { - s.keepHomeCardsFresh(ctx, interval) - close(stopped) - }() - - newestRouteASNIs := func(asn int) bool { + // newestRouteOrigin returns the origin AS of the newest route on the + // cards, or 0 when there is none. + newestRouteOrigin := func() int { cards := s.cards.Load() + if cards == nil || cards.NewestRoute == nil { + return 0 + } - return cards != nil && cards.NewestRoute != nil && - cards.NewestRoute.OriginASN == asn + return cards.NewestRoute.OriginASN } - waitUntil(t, maxWait, func() bool { return newestRouteASNIs(twoPrefixASN) }) + synctest.Test(t, func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + stopped := make(chan struct{}) - announce(table, "8.8.8.0/24", newestRouteASN, testPeer(2), testRouteTime(1)) - waitUntil(t, maxWait, func() bool { return newestRouteASNIs(newestRouteASN) }) + go func() { + s.keepHomeCardsFresh(ctx, interval) + close(stopped) + }() - cancel() + synctest.Wait() - select { - case <-stopped: - case <-time.After(maxWait): - t.Fatal("keepHomeCardsFresh did not stop when its context ended") - } + if got := newestRouteOrigin(); got != twoPrefixASN { + t.Fatalf("at the start, the newest route's AS is %d, want %d", + got, twoPrefixASN) + } + + announce(table, "8.8.8.0/24", newestRouteASN, testPeer(2), + testRouteTime(1)) + time.Sleep(interval) + synctest.Wait() + + if got := newestRouteOrigin(); got != newestRouteASN { + t.Fatalf("an interval after the table changed, the newest "+ + "route's AS is %d, want %d", got, newestRouteASN) + } + + cancel() + synctest.Wait() + + select { + case <-stopped: + default: + t.Fatal("keepHomeCardsFresh did not stop when its context ended") + } + }) } // TestStartedServerWorksOutTheCards starts a server and waits until its home -- 2.54.0