NarInfoDiskCache: Also test id consistency with updated fields

And clarify test

src/libstore/tests/nar-info-disk-cache.cc

@@ -9,21 +9,34 @@
 namespace nix {
 
 TEST(NarInfoDiskCacheImpl, create_and_read) {
+    // This is a large single test to avoid some setup overhead.
+
     int prio = 12345;
     bool wantMassQuery = true;
 
     Path tmpDir = createTempDir();
     AutoDelete delTmpDir(tmpDir);
     Path dbPath(tmpDir + "/test-narinfo-disk-cache.sqlite");
+
+    int savedId;
+    int barId;
+    SQLite db;
+    SQLiteStmt getIds;
+
+    {
         auto cache = getTestNarInfoDiskCache(dbPath);
 
+        // Set up "background noise" and check that different caches receive different ids
         {
             auto bc1 = cache->createCache("https://bar", "/nix/storedir", wantMassQuery, prio);
             auto bc2 = cache->createCache("https://xyz", "/nix/storedir", false, 12);
             ASSERT_NE(bc1, bc2);
+            barId = bc1;
         }
 
-    int savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);;
+        // Check that the fields are saved and returned correctly. This does not test
+        // the select statement yet, because of in-memory caching.
+        savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);;
         {
             auto r = cache->upToDateCacheExists("http://foo");
             ASSERT_TRUE(r);
@@ -32,9 +45,10 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
             ASSERT_EQ(savedId, r->id);
         }
 
-    SQLite db(dbPath);
+        // We're going to pay special attention to the id field because we had a bug
-    SQLiteStmt getIds;
+        // that changed it.
-    getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'http://foo'");
+        db = SQLite(dbPath);
+        getIds.create(db, "select id from BinaryCaches where url = 'http://foo'");
 
         {
             auto q(getIds.use());
@@ -43,17 +57,21 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
             ASSERT_FALSE(q.next());
         }
 
-    db.exec("UPDATE BinaryCaches SET timestamp = timestamp - 1 - 7 * 24 * 3600;");
+        // Pretend that the caches are older, but keep one up to date, as "background noise"
+        db.exec("update BinaryCaches set timestamp = timestamp - 1 - 7 * 24 * 3600 where url <> 'https://xyz';");
 
-    // Relies on memory cache
+        // This shows that the in-memory cache works
         {
             auto r = cache->upToDateCacheExists("http://foo");
             ASSERT_TRUE(r);
             ASSERT_EQ(r->priority, prio);
             ASSERT_EQ(r->wantMassQuery, wantMassQuery);
         }
+    }
 
-    // We can't clear the in-memory cache, so we use a new cache object.
+    {
+        // We can't clear the in-memory cache, so we use a new cache object. This is
+        // more realistic anyway.
         auto cache2 = getTestNarInfoDiskCache(dbPath);
 
         {
@@ -61,7 +79,7 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
             ASSERT_FALSE(r);
         }
 
-    // Update, same data, check that the id number is reused
+        // "Update", same data, check that the id number is reused
         cache2->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);
 
         {
@@ -80,7 +98,7 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
             ASSERT_EQ(currentId, savedId);
         }
 
-    // Check that the fields can be modified
+        // Check that the fields can be modified, and the id remains the same
         {
             auto r0 = cache2->upToDateCacheExists("https://bar");
             ASSERT_FALSE(r0);
@@ -89,6 +107,7 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
             auto r = cache2->upToDateCacheExists("https://bar");
             ASSERT_EQ(r->wantMassQuery, !wantMassQuery);
             ASSERT_EQ(r->priority, prio + 10);
+            ASSERT_EQ(r->id, barId);
         }
 
         // // Force update (no use case yet; we only retrieve cache metadata when stale based on timestamp)
@@ -98,7 +117,7 @@ TEST(NarInfoDiskCacheImpl, create_and_read) {
         //     ASSERT_EQ(r->wantMassQuery, wantMassQuery);
         //     ASSERT_EQ(r->priority, prio + 20);
         // }
-
+    }
 }
 
 }