Changeset 18636 in josm for trunk/test/unit

Timestamp:

2023-01-17T14:41:59+01:00 (16 months ago)

Author:

taylor.smock

Message:

Fix #21423: Prevent error codes from clashing

This works by creating a unique code using the test class name. The new
format for ignore entries will look like UNIQUECODE_CODE_MESSAGE_STATE.
The switchover date for the new entries is set at 2024-01-01 in order to
give users sufficient time to update, such that if a user has multiple
installations of JOSM, all versions will be able to use the same ignore
list. The compatibility code is intended to be removed in 2025.

Location:

trunk/test/unit/org/openstreetmap/josm

Files:

• data/validation/TestErrorTest.java (added)
• plugins/PluginHandlerTestIT.java (modified) (11 diffs)

trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java

@@ -4 +4 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -17 +18 @@
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.logging.Handler;
+import java.util.logging.LogRecord;
 import java.util.stream.Collectors;
 
@@ -24 +28 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.platform.commons.util.ReflectionUtils;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.Preferences;
@@ -74 +79 @@
         Map<String, Throwable> loadingExceptions = PluginHandler.pluginLoadingExceptions.entrySet().stream()
                 .filter(e -> !(Utils.getRootCause(e.getValue()) instanceof HeadlessException))
-                .collect(Collectors.toMap(e -> e.getKey(), e -> Utils.getRootCause(e.getValue())));
+                .collect(Collectors.toMap(Map.Entry::getKey, e -> Utils.getRootCause(e.getValue())));
 
         List<PluginInformation> loadedPlugins = PluginHandler.getPlugins();
@@ -93 +98 @@
         }
 
+        Map<String, String> testCodeHashCollisions = checkForHashCollisions();
+
         Map<String, Throwable> noRestartExceptions = new HashMap<>();
         testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions);
@@ -100 +107 @@
         debugPrint(layerExceptions);
         debugPrint(noRestartExceptions);
+        debugPrint(testCodeHashCollisions);
 
         invalidManifestEntries = filterKnownErrors(invalidManifestEntries);
@@ -105 +113 @@
         layerExceptions = filterKnownErrors(layerExceptions);
         noRestartExceptions = filterKnownErrors(noRestartExceptions);
+        testCodeHashCollisions = filterKnownErrors(testCodeHashCollisions);
 
         String msg = errMsg("invalidManifestEntries", invalidManifestEntries) + '\n' +
                 errMsg("loadingExceptions", loadingExceptions) + '\n' +
                 errMsg("layerExceptions", layerExceptions) + '\n' +
-                errMsg("noRestartExceptions", noRestartExceptions);
+                errMsg("noRestartExceptions", noRestartExceptions) + '\n' +
+                errMsg("testCodeHashCollisions", testCodeHashCollisions);
         assertTrue(invalidManifestEntries.isEmpty()
                 && loadingExceptions.isEmpty()
                 && layerExceptions.isEmpty()
-                && noRestartExceptions.isEmpty(), msg);
+                && noRestartExceptions.isEmpty()
+                && testCodeHashCollisions.isEmpty(), msg);
     }
 
@@ -122 +133 @@
     private static void testCompletelyRestartlessPlugins(List<PluginInformation> loadedPlugins,
             Map<String, Throwable> noRestartExceptions) {
+        final List<LogRecord> records = new ArrayList<>();
+        Handler tempHandler = new Handler() {
+            @Override
+            public void publish(LogRecord record) {
+                records.add(record);
+            }
+
+            @Override
+            public void flush() { /* Do nothing */ }
+
+            @Override
+            public void close() throws SecurityException { /* Do nothing */ }
+        };
+        Logging.getLogger().addHandler(tempHandler);
         try {
             List<PluginInformation> restartable = loadedPlugins.parallelStream()
@@ -142 +167 @@
             root.printStackTrace();
             noRestartExceptions.put(findFaultyPlugin(loadedPlugins, root), root);
-        }
+            records.removeIf(record -> Objects.equals(Utils.getRootCause(record.getThrown()), root));
+        } catch (AssertionError assertionError) {
+            noRestartExceptions.put("Plugin load/unload failed", assertionError);
+        } finally {
+            Logging.getLogger().removeHandler(tempHandler);
+            for (LogRecord record : records) {
+                if (record.getThrown() != null) {
+                    Throwable root = Utils.getRootCause(record.getThrown());
+                    root.printStackTrace();
+                    noRestartExceptions.put(findFaultyPlugin(loadedPlugins, root), root);
+                }
+            }
+        }
+    }
+
+    private static Map<String, String> checkForHashCollisions() {
+        Map<Integer, List<String>> codes = new HashMap<>();
+        for (Class<?> clazz : ReflectionUtils.findAllClassesInPackage("org.openstreetmap",
+                org.openstreetmap.josm.data.validation.Test.class::isAssignableFrom, s -> true)) {
+            if (org.openstreetmap.josm.data.validation.Test.class.isAssignableFrom(clazz)
+            && !Objects.equals(org.openstreetmap.josm.data.validation.Test.class, clazz)) {
+                // clazz.getName().hashCode() is how the base error codes are calculated since xxx
+                // We want to avoid cases where the hashcode is too close, so we want to
+                // ensure that there is at least 1m available codes after the hashCode.
+                // This is needed since some plugins pick some really large number, and count up from there.
+                int hashCeil = (int) Math.ceil(clazz.getName().hashCode() / 1_000_000d);
+                int hashFloor = (int) Math.floor(clazz.getName().hashCode() / 1_000_000d);
+                codes.computeIfAbsent(hashCeil, k -> new ArrayList<>()).add(clazz.getName());
+                codes.computeIfAbsent(hashFloor, k -> new ArrayList<>()).add(clazz.getName());
+            }
+        }
+        return codes.entrySet().stream().filter(entry -> entry.getValue().size() > 1).collect(
+                Collectors.toMap(entry -> entry.getKey().toString(), entry -> String.join(", ", entry.getValue())));
     }
 
@@ -154 +211 @@
         System.out.println(invalidManifestEntries.entrySet()
                 .stream()
-                .map(e -> convertEntryToString(e))
+                .map(PluginHandlerTestIT::convertEntryToString)
                 .collect(Collectors.joining(", ")));
     }
@@ -242 +299 @@
             try {
                 ClassLoader cl = PluginHandler.getPluginClassLoader(p.getName());
+                assertNotNull(cl);
                 String pluginPackage = cl.loadClass(p.className).getPackage().getName();
                 for (StackTraceElement e : root.getStackTrace()) {