GEODE-9486: Improve AnalyzeSerializables integration tests (apache#6878)

Allow geode modules to have an AnalyzeSerializables integration test without implementing SanctionedSerializablesService. Improve debugging information for AnalyzeSerializables integration tests: - Provide new failure message when no SanctionedSerializablesService exists. - Create ANALYZE_SERIALIZABLES.md to provide detailed instructions for failures involving AnalyzeSerializables integration tests. - Reference ANALYZE_SERIALIZABLES.md in failure messages. Remove geode-serialization and geode-common jars from geode-pulse .war file: - Change getModuleClass() to return Optional. - Delete PulseSanctionedSerializablesService and its resources. - Change geode-pulse dependency on geode-serialization to be for integrationTest only.
meslubi2021 · Sep 21, 2021 · 7bd1d73 · 7bd1d73
1 parent cf1b35c
commit 7bd1d73
Show file tree

Hide file tree

Showing 23 changed files with 254 additions and 121 deletions.
diff --git a/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeRedisSerializablesIntegrationTest.java b/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeRedisSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.redis.internal.RedisSanctionedSerializablesService;
@@ -28,7 +30,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return RedisSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(RedisSanctionedSerializablesService.class);
   }
 }
diff --git a/...est/java/org/apache/geode/codeAnalysis/AnalyzeConnectorsSerializablesIntegrationTest.java b/...est/java/org/apache/geode/codeAnalysis/AnalyzeConnectorsSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.connectors.jdbc.internal.ConnectorsSanctionedSerializablesService;
@@ -29,7 +31,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return ConnectorsSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(ConnectorsSanctionedSerializablesService.class);
   }
 }
diff --git a/...ationTest/java/org/apache/geode/codeAnalysis/AnalyzeCoreSerializablesIntegrationTest.java b/...ationTest/java/org/apache/geode/codeAnalysis/AnalyzeCoreSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.internal.CoreSanctionedSerializablesService;
@@ -28,7 +30,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return CoreSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(CoreSanctionedSerializablesService.class);
   }
 }
diff --git a/...grationTest/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesIntegrationTest.java b/...grationTest/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.cache.query.cq.internal.CQSanctionedSerializablesService;
@@ -29,7 +31,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return CQSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(CQSanctionedSerializablesService.class);
   }
 }
diff --git a/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeDUnitSerializablesIntegrationTest.java b/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeDUnitSerializablesIntegrationTest.java
@@ -43,8 +43,8 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return DUnitSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(DUnitSanctionedSerializablesService.class);
   }
 
   @Override

diff --git a/...ationTest/java/org/apache/geode/codeAnalysis/AnalyzeGfshSerializablesIntegrationTest.java b/...ationTest/java/org/apache/geode/codeAnalysis/AnalyzeGfshSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.gfsh.internal.management.GfshSanctionedSerializablesService;
@@ -28,7 +30,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return GfshSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(GfshSanctionedSerializablesService.class);
   }
 }
diff --git a/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeJUnitSerializablesIntegrationTest.java b/...tionTest/java/org/apache/geode/codeAnalysis/AnalyzeJUnitSerializablesIntegrationTest.java
@@ -17,6 +17,7 @@
 import static java.util.Arrays.asList;
 
 import java.util.HashSet;
+import java.util.Optional;
 import java.util.Set;
 
 import org.junit.experimental.categories.Category;
@@ -38,8 +39,8 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return JUnitSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(JUnitSanctionedSerializablesService.class);
   }
 
   @Override

diff --git a/...it/src/main/java/org/apache/geode/codeAnalysis/AnalyzeDataSerializablesJUnitTestBase.java b/...it/src/main/java/org/apache/geode/codeAnalysis/AnalyzeDataSerializablesJUnitTestBase.java
@@ -26,7 +26,6 @@
 import java.io.InputStream;
 import java.io.InvalidClassException;
 import java.io.Serializable;
-import java.net.URI;
 import java.net.URL;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -35,6 +34,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 import org.junit.AfterClass;
 import org.junit.Rule;
@@ -62,17 +62,26 @@ public abstract class AnalyzeDataSerializablesJUnitTestBase {
 
   private static final Path MODULE_ROOT = Paths.get("..", "..", "..").toAbsolutePath().normalize();
   private static final Path SOURCE_ROOT = MODULE_ROOT.resolve(Paths.get("src"));
-  private static final Path TEST_RESOURCES_SOURCE_ROOT =
+  static final Path INTEGRATION_TEST_RESOURCES_SOURCE_ROOT =
       SOURCE_ROOT.resolve(Paths.get("integrationTest", "resources"));
-  private static final Path BUILD_RESOURCES_ROOT =
-      MODULE_ROOT.resolve(Paths.get("build", "resources"));
-
-  private static final String NEW_LINE = System.getProperty("line.separator");
-
-  static final String FAIL_MESSAGE = NEW_LINE + NEW_LINE
-      + "If the class is not persisted or sent over the wire add it to the file " + NEW_LINE + "%s"
-      + NEW_LINE + "Otherwise if this doesn't break backward compatibility, copy the file "
-      + NEW_LINE + "%s to " + NEW_LINE + "%s.";
+  static final Path MAIN_RESOURCES_SOURCE_ROOT =
+      SOURCE_ROOT.resolve(Paths.get("main", "resources"));
+
+  static final String FAIL_MESSAGE = "%n" +
+      "If the class is not persisted or sent over the wire, add it to the file%n" +
+      "    %s%n" + // excluded file in integrationTest resources
+      "Otherwise, if this doesn't break backward compatibility, copy the file%n" +
+      "    %s%n" + // actual file in build
+      "    to %n" +
+      "    %s%n" + // sanctioned file in main resources
+      "If this potentially breaks backward compatibility, follow the instructions in%n" +
+      "    geode-serialization/ANALYZE_SERIALIZABLES.md%n"; // readme in geode-serialization
+
+  static final String FAIL_MESSAGE_NO_SERVICE = "%n" +
+      "If the class is not persisted or sent over the wire, add it to the file%n" +
+      "    %s%n" + // excluded file in integrationTest resources
+      "Otherwise, follow the instructions in%n" +
+      "    geode-serialization/ANALYZE_SERIALIZABLES.md%n"; // readme in geode-serialization
 
   static final String EXCLUDED_CLASSES_TXT = "excludedClasses.txt";
   private static final String ACTUAL_DATA_SERIALIZABLES_DAT = "actualDataSerializables.dat";
@@ -109,7 +118,7 @@ public static void afterClass() {
    * you have put your sanctioned-modulename-serializables.txt file in the production resources
    * tree.
    */
-  protected abstract Class<?> getModuleClass();
+  protected abstract Optional<Class<?>> getModuleClass();
 
   /**
    * Implement this to deserialize an object that was serialized with serializeObject()
@@ -234,31 +243,15 @@ private void serializeAndDeserializeObject(Object object) throws Exception {
     }
   }
 
-  String toBuildPathString(File file) {
+  private String toBuildPathString(File file) {
     if (file == null) {
       return null;
     }
     return file.toPath().toAbsolutePath().normalize().toString();
   }
 
   private String toTestResourcesSourcePathString(Path relativeFilePath) {
-    return TEST_RESOURCES_SOURCE_ROOT.resolve(relativeFilePath).normalize().toString();
-  }
-
-  /**
-   * <pre>
-   * FROM:
-   * /Users/user/dev/geode/geode-junit/build/resources/main/org/apache/geode/test/junit/internal/sanctioned-geode-junit-serializables.txt
-   *
-   * TO:
-   * /Users/user/dev/geode/geode-junit/src/main/resources/org/apache/geode/test/junit/internal/sanctioned-geode-junit-serializables.txt
-   * </pre>
-   */
-  String mainResourceToSourcePath(URI buildResourceURI) {
-    Path mainBuildResources = BUILD_RESOURCES_ROOT.resolve(Paths.get("main"));
-    Path mainSourceResources = SOURCE_ROOT.resolve(Paths.get("main", "resources"));
-    Path relativeFilePath = mainBuildResources.relativize(Paths.get(buildResourceURI));
-    return mainSourceResources.resolve(relativeFilePath).toString();
+    return INTEGRATION_TEST_RESOURCES_SOURCE_ROOT.resolve(relativeFilePath).normalize().toString();
   }
 
   List<String> loadExcludedClasses(File excludedClassesFile) throws IOException {
@@ -388,7 +381,7 @@ protected InputStream getResourceAsStream(Class<?> associatedClass, String resou
     return getResource(associatedClass, resourceName).openStream();
   }
 
-  static URL getResource(final Class<?> classInSamePackage, final String resourceName) {
+  private static URL getResource(final Class<?> classInSamePackage, final String resourceName) {
     return classInSamePackage.getResource(resourceName);
   }
 }
diff --git a/...-junit/src/main/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTestBase.java b/...-junit/src/main/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTestBase.java
@@ -14,7 +14,6 @@
  */
 package org.apache.geode.codeAnalysis;
 
-import static java.util.Collections.emptyList;
 import static org.apache.geode.distributed.ConfigurationProperties.SERIALIZABLE_OBJECT_FILTER;
 import static org.apache.geode.distributed.ConfigurationProperties.VALIDATE_SERIALIZABLE_OBJECTS;
 import static org.assertj.core.api.Assertions.fail;
@@ -29,8 +28,10 @@
 import java.io.InputStream;
 import java.io.InvalidClassException;
 import java.io.Serializable;
+import java.io.UncheckedIOException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Modifier;
+import java.nio.file.Path;
 import java.rmi.RemoteException;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -71,7 +72,7 @@ public abstract class AnalyzeSerializablesJUnitTestBase
   private final String expectedSerializablesFileName =
       "sanctioned-" + getModuleName() + "-serializables.txt";
 
-  protected List<ClassAndVariableDetails> expectedSerializables;
+  protected final List<ClassAndVariableDetails> expectedSerializables = new ArrayList<>();
 
   @Before
   public void setUp() throws Exception {
@@ -97,13 +98,47 @@ public void testSerializables() throws Exception {
       System.out.println(
           "++++++++++++++++++++++++++++++testSerializables found discrepancies++++++++++++++++++++++++++++++++++++");
       System.out.println(diff);
-      fail(diff + FAIL_MESSAGE, toBuildPathString(getResourceAsFile(EXCLUDED_CLASSES_TXT)),
-          actualSerializablesFile.getAbsolutePath(),
-          mainResourceToSourcePath(
-              getResource(getModuleClass(), expectedSerializablesFileName).toURI()));
+
+      String codeAnalysisPackageDir = getPackageDirForClass(getClass());
+      Path excludedClassesSourceFile = INTEGRATION_TEST_RESOURCES_SOURCE_ROOT
+          .resolve(codeAnalysisPackageDir)
+          .resolve(EXCLUDED_CLASSES_TXT);
+
+      String failureMessage = getModuleClass()
+          .map(clazz -> failWithServiceMessage(
+              actualSerializablesFile, diff, excludedClassesSourceFile, clazz))
+          .orElse(failWithoutServiceMessage(
+              diff, excludedClassesSourceFile));
+
+      fail(failureMessage);
     }
   }
 
+  private String failWithServiceMessage(File actualSerializablesFile,
+      String diff,
+      Path excludedClassesSourceFile,
+      Class<?> serviceClass) {
+    Path sanctionedSerializablesSourceFile =
+        getSanctionedSerializablesSourceFileForServiceClass(serviceClass);
+    return String.format(diff + FAIL_MESSAGE,
+        excludedClassesSourceFile,
+        actualSerializablesFile.getAbsolutePath(),
+        sanctionedSerializablesSourceFile);
+  }
+
+  private String failWithoutServiceMessage(String diff,
+      Path excludedClassesSourceFile) {
+    return String.format(diff + FAIL_MESSAGE_NO_SERVICE,
+        excludedClassesSourceFile);
+  }
+
+  private Path getSanctionedSerializablesSourceFileForServiceClass(Class<?> serviceClass) {
+    String moduleServicePackageDir = getPackageDirForClass(serviceClass);
+    return MAIN_RESOURCES_SOURCE_ROOT
+        .resolve(moduleServicePackageDir)
+        .resolve(expectedSerializablesFileName);
+  }
+
   @Test
   public void testSanctionedClassesExistAndDoDeserialize() throws Exception {
     loadExpectedSerializables();
@@ -238,15 +273,17 @@ public void testExcludedClassesAreNotInSanctionedSerializables() throws Exceptio
   }
 
   public void loadExpectedSerializables() throws Exception {
-    if (expectedSerializablesFileName == null) {
-      expectedSerializables = emptyList();
-    } else {
-      try (InputStream expectedSerializablesStream =
-          getResourceAsStream(getModuleClass(), expectedSerializablesFileName)) {
-        // the expectedSerializablesStream will be automatically closed when we exit this block
-        expectedSerializables =
-            CompiledClassUtils.loadClassesAndVariables(expectedSerializablesStream);
-      }
+    getModuleClass().ifPresent(this::loadSanctionedSerializables);
+  }
+
+  private void loadSanctionedSerializables(Class<?> clazz) {
+    try (InputStream expectedSerializablesStream =
+        getResourceAsStream(clazz, expectedSerializablesFileName)) {
+      // the expectedSerializablesStream will be automatically closed when we exit this block
+      expectedSerializables.addAll(
+          CompiledClassUtils.loadClassesAndVariables(expectedSerializablesStream));
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
     }
   }
 
@@ -343,4 +380,8 @@ private boolean isSerializableAndNotDataSerializable(CompiledClass compiledClass
     }
     return false;
   }
+
+  private static String getPackageDirForClass(Class<?> theClass) {
+    return theClass.getPackage().getName().replace(".", File.separator);
+  }
 }
diff --git a/...ionTest/java/org/apache/geode/codeAnalysis/AnalyzeLuceneSerializablesIntegrationTest.java b/...ionTest/java/org/apache/geode/codeAnalysis/AnalyzeLuceneSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.cache.lucene.internal.LuceneSanctionedSerializablesService;
@@ -29,7 +31,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return LuceneSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(LuceneSanctionedSerializablesService.class);
   }
 }
diff --git a/...est/java/org/apache/geode/codeAnalysis/AnalyzeManagementSerializablesIntegrationTest.java b/...est/java/org/apache/geode/codeAnalysis/AnalyzeManagementSerializablesIntegrationTest.java
@@ -14,6 +14,8 @@
  */
 package org.apache.geode.codeAnalysis;
 
+import java.util.Optional;
+
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.management.internal.ManagementSanctionedSerializablesService;
@@ -29,7 +31,7 @@ protected String getModuleName() {
   }
 
   @Override
-  protected Class<?> getModuleClass() {
-    return ManagementSanctionedSerializablesService.class;
+  protected Optional<Class<?>> getModuleClass() {
+    return Optional.of(ManagementSanctionedSerializablesService.class);
   }
 }