Add option to rebuild and check NoteDb after acceptance tests

Similar to GERRIT_ENABLE_NOTEDB, use GERRIT_CHECK_NOTEDB to tell the
test harness to rebuild and check all ChangeBundles immediately before
test server shutdown. We do this once at server shutdown rather than
after each test method since we don't have an easy way to see which
changes were created by a given test method. Plus, this avoids putting
notedb in an awkward partially-migrated state.

This exposes some test failures, but should be submitted sooner rather
than later to enable multiple people to work on fixing up the tests.
And these tests aren't currently run by CI, so it won't block other
Gerrit development.

Change-Id: Ic8ff4722a59177949aa53bc4b0b3e979cce2fd8e

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.acceptance;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
@@ -29,6 +31,8 @@ import com.google.gerrit.server.ssh.NoSshModule;
 import com.google.gerrit.server.util.SocketUtil;
 import com.google.gerrit.server.util.SystemLog;
 import com.google.gerrit.testutil.FakeEmailSender;
+import com.google.gerrit.testutil.GerritServerTests;
+import com.google.gerrit.testutil.NoteDbChecker;
 import com.google.gerrit.testutil.TempFileUtil;
 import com.google.inject.Injector;
 import com.google.inject.Key;
@@ -283,13 +287,22 @@ public class GerritServer {
   }
 
   void stop() throws Exception {
-    daemon.getLifecycleManager().stop();
+    try {
-    if (daemonService != null) {
+      if (GerritServerTests.isEnvVarTrue("GERRIT_CHECK_NOTEDB")) {
-      System.out.println("Gerrit Server Shutdown");
+        checkState(!GerritServerTests.isNoteDbTestEnabled(),
-      daemonService.shutdownNow();
+            "cannot rebuild and check NoteDb when starting from scratch with"
-      daemonService.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
+                + " NoteDb enabled");
+        testInjector.getInstance(NoteDbChecker.class).checkAllChanges();
+      }
+    } finally {
+      daemon.getLifecycleManager().stop();
+      if (daemonService != null) {
+        System.out.println("Gerrit Server Shutdown");
+        daemonService.shutdownNow();
+        daemonService.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
+      }
+      RepositoryCache.clear();
     }
-    RepositoryCache.clear();
   }
 
   @Override

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java

@@ -16,6 +16,7 @@ package com.google.gerrit.reviewdb.server;
 
 import com.google.common.base.Function;
 import com.google.common.collect.Ordering;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gwtorm.client.IntKey;
 
 /** Static utilities for ReviewDb types. */
@@ -28,6 +29,14 @@ public class ReviewDbUtil {
         }
       };
 
+  private static final Function<Change, Change.Id> CHANGE_ID_FUNCTION =
+      new Function<Change, Change.Id>() {
+        @Override
+        public Change.Id apply(Change in) {
+          return in.getId();
+        }
+      };
+
   private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING =
       Ordering.natural().onResultOf(INT_KEY_FUNCTION);
 
@@ -36,6 +45,10 @@ public class ReviewDbUtil {
     return (Ordering<K>) INT_KEY_ORDERING;
   }
 
+  public static Function<Change, Change.Id> changeIdFunction() {
+    return CHANGE_ID_FUNCTION;
+  }
+
   private ReviewDbUtil() {
   }
 }

gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java

@@ -23,10 +23,13 @@ import org.junit.runner.Description;
 import org.junit.runner.RunWith;
 import org.junit.runners.model.Statement;
 
-import java.util.List;
+import java.util.Collection;
 
 @RunWith(ConfigSuite.class)
 public class GerritServerTests extends GerritBaseTests {
+  private static final Collection<String> ENV_VAR_TRUE_VALUES =
+      ImmutableList.of("yes", "y", "true", "1");
+
   @ConfigSuite.Parameter
   public Config config;
 
@@ -36,9 +39,12 @@ public class GerritServerTests extends GerritBaseTests {
   protected TestNotesMigration notesMigration;
 
   public static boolean isNoteDbTestEnabled() {
-    List<String> runValues = ImmutableList.of("yes", "y", "true", "1");
+    return isEnvVarTrue("GERRIT_ENABLE_NOTEDB");
-    String value = System.getenv("GERRIT_ENABLE_NOTEDB");
+  }
-    return value != null && runValues.contains(value.toLowerCase());
+
+  public static boolean isEnvVarTrue(String name) {
+    String value = System.getenv(name);
+    return value != null && ENV_VAR_TRUE_VALUES.contains(value.toLowerCase());
   }
 
   @Rule

gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java

@@ -14,11 +14,11 @@
 
 package com.google.gerrit.testutil;
 
-import com.google.common.base.Function;
 import com.google.common.base.Joiner;
-import com.google.common.collect.Ordering;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.notedb.ChangeBundle;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -30,7 +30,6 @@ import com.google.inject.Singleton;
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 
 @Singleton
@@ -54,6 +53,13 @@ public class NoteDbChecker {
     this.plcUtil = plcUtil;
   }
 
+  public void checkAllChanges() throws Exception {
+    checkChanges(
+        Iterables.transform(
+            unwrapDb().changes().all(),
+            ReviewDbUtil.changeIdFunction()));
+  }
+
   public void checkChanges(Change.Id... changeIds) throws Exception {
     checkChanges(Arrays.asList(changeIds));
   }
@@ -62,17 +68,12 @@ public class NoteDbChecker {
     ReviewDb db = unwrapDb();
 
     notesMigration.setReadChanges(false);
-    List<ChangeBundle> allExpected = new ArrayList<>();
+    List<Change.Id> sortedIds =
-    for (Change.Id id : changeIds) {
+        ReviewDbUtil.intKeyOrdering().sortedCopy(changeIds);
+    List<ChangeBundle> allExpected = new ArrayList<>(sortedIds.size());
+    for (Change.Id id : sortedIds) {
       allExpected.add(ChangeBundle.fromReviewDb(db, id));
     }
-    Collections.sort(allExpected, Ordering.natural().onResultOf(
-        new Function<ChangeBundle, Integer>() {
-          @Override
-          public Integer apply(ChangeBundle in) {
-            return in.getChange().getId().get();
-          }
-        }));
 
     notesMigration.setWriteChanges(true);
     notesMigration.setReadChanges(true);
@@ -86,6 +87,9 @@ public class NoteDbChecker {
         throw new AssertionError(
             "Differences between ReviewDb and NoteDb for " + c + ":\n"
             + Joiner.on('\n').join(diff));
+      } else {
+        System.err.println(
+            "NoteDb conversion of change " + c.getId() + " successful");
       }
     }
   }