diff --git a/pkg/appview/db/manifest_key_swap_test.go b/pkg/appview/db/manifest_key_swap_test.go index 30affe4..f703cae 100644 --- a/pkg/appview/db/manifest_key_swap_test.go +++ b/pkg/appview/db/manifest_key_swap_test.go @@ -225,10 +225,10 @@ func reshapeToPre0034WithChildren(t *testing.T, database *sql.DB) { } // TestLatestManifestUsesCreatedAtNotInsertOrder pins the recency change that -// dropping id forced. +// dropping id forced, across every query that makes it. // -// Repo cards, search results and starred repos all needed "the newest manifest -// in this repo", and all five queries answered it with MAX(id) — the rowid as a +// Repo cards, search, user pages and starred repos all needed "the newest +// manifest in this repo", and all four answered it with MAX(id) — the rowid as a // proxy for insert order. A derived key has no ordering, so recency now comes // from created_at, with manifest_key as a deterministic tiebreak. // @@ -236,6 +236,10 @@ func reshapeToPre0034WithChildren(t *testing.T, database *sql.DB) { // backfill does routinely: it walks a PDS and inserts whatever it finds, so the // last row inserted is not the most recently pushed. created_at is the push // time, and is the answer these queries always wanted. +// +// All four are covered because they carry four separate copies of the same CTE; +// fixing one and missing another would leave the UI disagreeing with itself +// about which manifest is current. func TestLatestManifestUsesCreatedAtNotInsertOrder(t *testing.T) { database := revTestDB(t) did := manifestKeyTestUser(t, database) @@ -255,21 +259,50 @@ func TestLatestManifestUsesCreatedAtNotInsertOrder(t *testing.T) { t.Fatalf("insert older: %v", err) } - cards, err := GetUserRepoCards(database, did, "") - if err != nil { - t.Fatalf("GetUserRepoCards: %v", err) + // Star it so the starred-repos query has something to return. + if err := UpsertStar(database, did, did, "myapp", now); err != nil { + t.Fatalf("UpsertStar: %v", err) } - if len(cards) != 1 { - t.Fatalf("expected 1 repo card, got %d", len(cards)) + + cases := []struct { + name string + run func() ([]RepoCardData, error) + }{ + {"GetUserRepoCards", func() ([]RepoCardData, error) { + return GetUserRepoCards(database, did, "") + }}, + {"GetRepoCards", func() ([]RepoCardData, error) { + return GetRepoCards(database, 10, "", SortByLastUpdate) + }}, + {"GetStarredRepoCards", func() ([]RepoCardData, error) { + return GetStarredRepoCards(database, did, "") + }}, + {"SearchRepositories", func() ([]RepoCardData, error) { + cards, _, err := SearchRepositories(database, "myapp", 10, 0, "") + return cards, err + }}, } - if cards[0].Digest != "sha256:newer" { - t.Errorf("repo card shows digest %q, want the most recently created manifest sha256:newer; "+ - "recency is following insert order rather than created_at", cards[0].Digest) + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + cards, err := c.run() + if err != nil { + t.Fatalf("%s: %v", c.name, err) + } + if len(cards) != 1 { + t.Fatalf("%s returned %d cards, want 1", c.name, len(cards)) + } + if cards[0].Digest != "sha256:newer" { + t.Errorf("%s shows digest %q, want the most recently created manifest sha256:newer; "+ + "recency is following insert order rather than created_at", c.name, cards[0].Digest) + } + }) } } // TestLatestManifestTieBreakIsDeterministic: two manifests pushed in the same -// second must not make the card flip between them from query to query. +// second must not make the card flip between them from query to query, or from +// one surface to another. func TestLatestManifestTieBreakIsDeterministic(t *testing.T) { database := revTestDB(t) did := manifestKeyTestUser(t, database) @@ -283,8 +316,12 @@ func TestLatestManifestTieBreakIsDeterministic(t *testing.T) { t.Fatalf("insert %s: %v", digest, err) } } + if err := UpsertStar(database, did, did, "myapp", same); err != nil { + t.Fatalf("UpsertStar: %v", err) + } - var first string + // Repeated runs must agree... + var want string for i := range 5 { cards, err := GetUserRepoCards(database, did, "") if err != nil { @@ -294,11 +331,35 @@ func TestLatestManifestTieBreakIsDeterministic(t *testing.T) { t.Fatalf("expected 1 repo card, got %d", len(cards)) } if i == 0 { - first = cards[0].Digest + want = cards[0].Digest continue } - if cards[0].Digest != first { - t.Fatalf("tie broken differently between runs: %q then %q", first, cards[0].Digest) + if cards[0].Digest != want { + t.Fatalf("tie broken differently between runs: %q then %q", want, cards[0].Digest) + } + } + + // ...and so must the other three surfaces, or the same repo shows a + // different "current" manifest depending on which page you are looking at. + others := map[string]func() ([]RepoCardData, error){ + "GetRepoCards": func() ([]RepoCardData, error) { return GetRepoCards(database, 10, "", SortByLastUpdate) }, + "GetStarredRepoCards": func() ([]RepoCardData, error) { return GetStarredRepoCards(database, did, "") }, + "SearchRepositories": func() ([]RepoCardData, error) { + cards, _, err := SearchRepositories(database, "myapp", 10, 0, "") + return cards, err + }, + } + for name, run := range others { + cards, err := run() + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if len(cards) != 1 { + t.Fatalf("%s returned %d cards, want 1", name, len(cards)) + } + if cards[0].Digest != want { + t.Errorf("%s picked %q but GetUserRepoCards picked %q; the tiebreak differs between surfaces", + name, cards[0].Digest, want) } } }