Index: trunk/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java (revision 18636) +++ trunk/test/unit/org/openstreetmap/josm/data/validation/TestErrorTest.java (revision 18636) @@ -0,0 +1,88 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.data.validation; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Stream; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.validation.tests.InternetTags; +import org.openstreetmap.josm.gui.progress.NullProgressMonitor; +import org.openstreetmap.josm.testutils.annotations.BasicPreferences; + +/** + * Test class for {@link TestError} + */ +@BasicPreferences +class TestErrorTest { + static Stream testCodeCompatibility() { + return Stream.of(Arguments.of(InternetTags.class, 3301, 1166507262, false, Collections.singletonList(TestUtils.newNode("url=invalid"))), + Arguments.of(InternetTags.class, 3301, 1166507262, true, Collections.singletonList(TestUtils.newNode("url=invalid")))); + } + + /** + * See #18230/#21423: Keep error codes unique + * + * @param testClass The test class to use + * @param originalCode The expected error code (original) + * @param expectedCode The expected error code (new, should be {@code testClass.getName().hashCode()}) + * @param switchOver {@code true} if the new code should be saved instead of the original code + * @param primitiveCollection The primitives to run the test on + * @throws ReflectiveOperationException If the test class could not be instantiated (no-op constructor) + */ + @ParameterizedTest + @MethodSource + void testCodeCompatibility(Class testClass, int originalCode, int expectedCode, + boolean switchOver, List primitiveCollection) throws ReflectiveOperationException { + // Ensure that this test always works + TestError.setUpdateErrorCodes(switchOver); + assertEquals(expectedCode, testClass.getName().hashCode()); + // Run the test + final Test test = testClass.getConstructor().newInstance(); + test.startTest(NullProgressMonitor.INSTANCE); + test.visit(primitiveCollection); + test.endTest(); + assertFalse(test.getErrors().isEmpty()); + assertEquals(1, test.getErrors().size()); + final TestError testError = test.getErrors().get(0); + final String ignoreGroup = testError.getIgnoreGroup(); + final String ignoreSubGroup = testError.getIgnoreSubGroup(); + if (primitiveCollection.size() == 1 && primitiveCollection.get(0).isNew()) { + assertNull(testError.getIgnoreState()); + primitiveCollection.get(0).setOsmId(1, 1); + } + final String ignoreState = testError.getIgnoreState(); + final String startUniqueCode = expectedCode + "_"; + assertAll(() -> assertTrue(ignoreGroup.startsWith(startUniqueCode + originalCode)), + () -> assertTrue(ignoreSubGroup.startsWith(startUniqueCode + originalCode)), + () -> assertTrue(ignoreState.startsWith(startUniqueCode + originalCode))); + for (String ignore : Arrays.asList(ignoreGroup, ignoreSubGroup, ignoreState)) { + OsmValidator.clearIgnoredErrors(); + final String oldIgnore = ignore.replace(startUniqueCode, ""); + OsmValidator.addIgnoredError(oldIgnore); + // Add the ignored error + assertTrue(testError.updateIgnored()); + assertAll(() -> assertEquals(switchOver, OsmValidator.hasIgnoredError(ignore)), + () -> assertNotEquals(switchOver, OsmValidator.hasIgnoredError(oldIgnore))); + + OsmValidator.clearIgnoredErrors(); + OsmValidator.addIgnoredError(ignore); + // Add the ignored error + assertTrue(testError.updateIgnored()); + assertAll(() -> assertTrue(OsmValidator.hasIgnoredError(ignore)), + () -> assertFalse(OsmValidator.hasIgnoredError(oldIgnore))); + } + } +} Index: trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (revision 18635) +++ trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (revision 18636) @@ -4,4 +4,5 @@ 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,6 +18,9 @@ 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,4 +28,5 @@ 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,5 +79,5 @@ Map 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 loadedPlugins = PluginHandler.getPlugins(); @@ -93,4 +98,6 @@ } + Map testCodeHashCollisions = checkForHashCollisions(); + Map noRestartExceptions = new HashMap<>(); testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions); @@ -100,4 +107,5 @@ debugPrint(layerExceptions); debugPrint(noRestartExceptions); + debugPrint(testCodeHashCollisions); invalidManifestEntries = filterKnownErrors(invalidManifestEntries); @@ -105,13 +113,16 @@ 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,4 +133,18 @@ private static void testCompletelyRestartlessPlugins(List loadedPlugins, Map noRestartExceptions) { + final List 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 restartable = loadedPlugins.parallelStream() @@ -142,5 +167,37 @@ 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 checkForHashCollisions() { + Map> 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,5 +211,5 @@ System.out.println(invalidManifestEntries.entrySet() .stream() - .map(e -> convertEntryToString(e)) + .map(PluginHandlerTestIT::convertEntryToString) .collect(Collectors.joining(", "))); } @@ -242,4 +299,5 @@ try { ClassLoader cl = PluginHandler.getPluginClassLoader(p.getName()); + assertNotNull(cl); String pluginPackage = cl.loadClass(p.className).getPackage().getName(); for (StackTraceElement e : root.getStackTrace()) {