Changeset 15981 in josm

Timestamp:

2020-03-01T23:35:51+01:00 (4 years ago)

Author:

simon04

Message:

see #18802 - MapCSSTagChecker: no not persist assertions

Check assertions immediately, when needed, but do not keep them.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (modified) (10 diffs)
• src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (modified) (6 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java

@@ -22 +22 @@
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.function.Predicate;
 import java.util.regex.Matcher;
@@ -288 +289 @@
          * Is evaluated on the matching primitive to give the error message. Map is checked to contain exactly one element. */
         protected final Map<Instruction.AssignmentInstruction, Severity> errors = new HashMap<>();
-        /** Unit tests */
-        protected final Map<String, Boolean> assertions = new HashMap<>();
         /** MapCSS Classes to set on matching primitives */
         protected final Set<String> setClassExpressions = new HashSet<>();
@@ -303 +302 @@
         private static final String POSSIBLE_THROWS = "throwError/throwWarning/throwOther";
 
-        static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataException {
+        static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule, AssertionConsumer assertionConsumer) throws IllegalDataException {
             final TagCheck check = new TagCheck(rule);
+            final Map<String, Boolean> assertions = new HashMap<>();
             for (Instruction i : rule.declaration.instructions) {
                 if (i instanceof Instruction.AssignmentInstruction) {
@@ -346 +346 @@
                             check.alternatives.add(val);
                         } else if (val != null && "assertMatch".equals(ai.key)) {
-                            check.assertions.put(val, Boolean.TRUE);
+                            assertions.put(val, Boolean.TRUE);
                         } else if (val != null && "assertNoMatch".equals(ai.key)) {
-                            check.assertions.put(val, Boolean.FALSE);
+                            assertions.put(val, Boolean.FALSE);
                         } else if (val != null && "group".equals(ai.key)) {
                             check.group = val;
@@ -369 +369 @@
                                 + rule.selectors);
             }
+            if (assertionConsumer != null) {
+                MapCSSTagCheckerAsserts.checkAsserts(check, assertions, assertionConsumer);
+            }
             return check;
         }
 
         static ParseResult readMapCSS(Reader css) throws ParseException {
-            return readMapCSS(css, "");
-        }
-
-        static ParseResult readMapCSS(Reader css, String url) throws ParseException {
+            return readMapCSS(css, "", null);
+        }
+
+        static ParseResult readMapCSS(Reader css, String url, AssertionConsumer assertionConsumer) throws ParseException {
             CheckParameterUtil.ensureParameterNotNull(css, "css");
 
@@ -401 +404 @@
                 try {
                     parseChecks.add(TagCheck.ofMapCSSRule(
-                            new GroupedMapCSSRule(map.getValue(), map.getKey(), url)));
+                            new GroupedMapCSSRule(map.getValue(), map.getKey(), url), assertionConsumer));
                 } catch (IllegalDataException e) {
                     Logging.error("Cannot add MapCss rule: "+e.getMessage());
@@ -781 +784 @@
 
     /**
+     * A handler for assertion error messages (for not fulfilled "assertMatch", "assertNoMatch").
+     */
+    @FunctionalInterface
+    interface AssertionConsumer extends Consumer<String> {
+    }
+
+    /**
      * Adds a new MapCSS config file from the given URL.
      * @param url The unique URL of the MapCSS config file
@@ -789 +799 @@
      */
     public synchronized ParseResult addMapCSS(String url) throws ParseException, IOException {
+        // Check assertions, useful for development of local files
+        final boolean checkAssertions = Config.getPref().getBoolean("validator.check_assert_local_rules", false) && Utils.isLocalUrl(url);
+        return addMapCSS(url, checkAssertions ? Logging::warn : null);
+    }
+
+    synchronized ParseResult addMapCSS(String url, AssertionConsumer assertionConsumer) throws ParseException, IOException {
         CheckParameterUtil.ensureParameterNotNull(url, "url");
         ParseResult result;
@@ -797 +813 @@
             if (zip != null)
                 I18n.addTexts(cache.getFile());
-            result = TagCheck.readMapCSS(reader, url);
+            result = TagCheck.readMapCSS(reader, url, assertionConsumer);
             checks.remove(url);
             checks.putAll(url, result.parseChecks);
             indexData = null;
-            // Check assertions, useful for development of local files
-            if (Config.getPref().getBoolean("validator.check_assert_local_rules", false) && Utils.isLocalUrl(url)) {
-                for (String msg : MapCSSTagCheckerAsserts.checkAsserts(result.parseChecks)) {
-                    Logging.warn(msg);
-                }
-            }
         }
         return result;
@@ -838 +848 @@
             }
         }
+        MapCSSTagCheckerAsserts.clear();
     }
 

trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java

@@ -8 +8 @@
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -36 +35 @@
 
 /**
- * Utility class for checking rule {@linkplain MapCSSTagChecker.TagCheck#assertions assertions} of {@link MapCSSTagChecker.TagCheck}.
+ * Utility class for checking rule assertions of {@link MapCSSTagChecker.TagCheck}.
  */
-class MapCSSTagCheckerAsserts {
+final class MapCSSTagCheckerAsserts {
+
     private MapCSSTagCheckerAsserts() {
         // private constructor
     }
 
+    private static final ArrayList<MapCSSTagChecker.TagCheck> previousChecks = new ArrayList<>();
+
     /**
      * Checks that rule assertions are met for the given set of TagChecks.
-     * @param schecks The TagChecks for which assertions have to be checked
-     * @return A set of error messages, empty if all assertions are met
-     * @since 7356
+     * @param check The TagCheck for which assertions have to be checked
+     * @param assertionConsumer The handler for assertion error messages
      */
-    public static Set<String> checkAsserts(final Collection<MapCSSTagChecker.TagCheck> schecks) {
-        Set<String> assertionErrors = new LinkedHashSet<>();
+    static void checkAsserts(final MapCSSTagChecker.TagCheck check, final Map<String, Boolean> assertions,
+                                    final MapCSSTagChecker.AssertionConsumer assertionConsumer) {
         final Method insideMethod = getFunctionMethod("inside");
         final DataSet ds = new DataSet();
-        for (final MapCSSTagChecker.TagCheck check : schecks) {
-            Logging.debug("Check: {0}", check);
-            for (final Map.Entry<String, Boolean> i : check.assertions.entrySet()) {
-                Logging.debug("- Assertion: {0}", i);
-                final OsmPrimitive p = OsmUtils.createPrimitive(i.getKey(), getLocation(check, insideMethod), true);
-                // Build minimal ordered list of checks to run to test the assertion
-                List<Set<MapCSSTagChecker.TagCheck>> checksToRun = new ArrayList<>();
-                Set<MapCSSTagChecker.TagCheck> checkDependencies = getTagCheckDependencies(check, schecks);
-                if (!checkDependencies.isEmpty()) {
-                    checksToRun.add(checkDependencies);
+        Logging.debug("Check: {0}", check);
+        for (final Map.Entry<String, Boolean> i : assertions.entrySet()) {
+            Logging.debug("- Assertion: {0}", i);
+            final OsmPrimitive p = OsmUtils.createPrimitive(i.getKey(), getLocation(check, insideMethod), true);
+            // Build minimal ordered list of checks to run to test the assertion
+            List<Set<MapCSSTagChecker.TagCheck>> checksToRun = new ArrayList<>();
+            Set<MapCSSTagChecker.TagCheck> checkDependencies = getTagCheckDependencies(check, previousChecks);
+            if (!checkDependencies.isEmpty()) {
+                checksToRun.add(checkDependencies);
+            }
+            checksToRun.add(Collections.singleton(check));
+            // Add primitive to dataset to avoid DataIntegrityProblemException when evaluating selectors
+            addPrimitive(ds, p);
+            final Collection<TestError> pErrors = MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun);
+            Logging.debug("- Errors: {0}", pErrors);
+            final boolean isError = pErrors.stream().anyMatch(e -> e.getTester() instanceof MapCSSTagChecker.MapCSSTagCheckerAndRule
+                    && ((MapCSSTagChecker.MapCSSTagCheckerAndRule) e.getTester()).rule.equals(check.rule));
+            if (isError != i.getValue()) {
+                assertionConsumer.accept(MessageFormat.format("Expecting test ''{0}'' (i.e., {1}) to {2} {3} (i.e., {4})",
+                        check.getMessage(p), check.rule.selectors, i.getValue() ? "match" : "not match", i.getKey(), p.getKeys()));
+            }
+            if (isError) {
+                // Check that autofix works as expected
+                Command fix = check.fixPrimitive(p);
+                if (fix != null && fix.executeCommand() && !MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun).isEmpty()) {
+                    assertionConsumer.accept(MessageFormat.format("Autofix does not work for test ''{0}'' (i.e., {1})",
+                            check.getMessage(p), check.rule.selectors));
                 }
-                checksToRun.add(Collections.singleton(check));
-                // Add primitive to dataset to avoid DataIntegrityProblemException when evaluating selectors
-                addPrimitive(ds, p);
-                final Collection<TestError> pErrors = MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun);
-                Logging.debug("- Errors: {0}", pErrors);
-                final boolean isError = pErrors.stream().anyMatch(e -> e.getTester() instanceof MapCSSTagChecker.MapCSSTagCheckerAndRule
-                        && ((MapCSSTagChecker.MapCSSTagCheckerAndRule) e.getTester()).rule.equals(check.rule));
-                if (isError != i.getValue()) {
-                    assertionErrors.add(MessageFormat.format("Expecting test ''{0}'' (i.e., {1}) to {2} {3} (i.e., {4})",
-                            check.getMessage(p), check.rule.selectors, i.getValue() ? "match" : "not match", i.getKey(), p.getKeys()));
-                }
-                if (isError) {
-                    // Check that autofix works as expected
-                    Command fix = check.fixPrimitive(p);
-                    if (fix != null && fix.executeCommand() && !MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun).isEmpty()) {
-                        assertionErrors.add(MessageFormat.format("Autofix does not work for test ''{0}'' (i.e., {1})",
-                                check.getMessage(p), check.rule.selectors));
-                    }
-                }
-                ds.removePrimitive(p);
             }
+            ds.removePrimitive(p);
         }
-        return assertionErrors;
+        previousChecks.add(check);
+    }
+
+    public static void clear() {
+        previousChecks.clear();
+        previousChecks.trimToSize();
     }
 

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java

@@ -16 +16 @@
 import java.util.Set;
 
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -28 +29 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.OsmUtils;
+import org.openstreetmap.josm.data.preferences.sources.ExtendedSourceEntry;
+import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.TestError;
@@ -53 +56 @@
     public JOSMTestRules test = new JOSMTestRules().projection().territories().preferences();
 
+    /**
+     * Setup test.
+     */
+    @Before
+    public void setUp() {
+        MapCSSTagCheckerAsserts.clear();
+    }
+
     static MapCSSTagChecker buildTagChecker(String css) throws ParseException {
         final MapCSSTagChecker test = new MapCSSTagChecker();
-        test.checks.putAll("test", TagCheck.readMapCSS(new StringReader(css)).parseChecks);
+        Set<String> errors = new HashSet<>();
+        test.checks.putAll("test", TagCheck.readMapCSS(new StringReader(css), "", errors::add).parseChecks);
+        assertTrue(errors.toString(), errors.isEmpty());
         return test;
     }
@@ -182 +195 @@
         MapCSSTagChecker c = new MapCSSTagChecker();
         c.initialize();
-
+    }
+
+    /**
+     * Unit test for all {@link MapCSSTagChecker.TagTest} assertions.
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testAssertions() throws Exception {
+        MapCSSTagChecker c = new MapCSSTagChecker();
         Set<String> assertionErrors = new LinkedHashSet<>();
-        for (Set<TagCheck> schecks : c.checks.values()) {
-            assertionErrors.addAll(MapCSSTagCheckerAsserts.checkAsserts(schecks));
-        }
+
+        // initialize
+        for (ExtendedSourceEntry entry : ValidatorPrefHelper.INSTANCE.getDefault()) {
+            c.addMapCSS(entry.url, assertionErrors::add);
+        }
+
         for (String msg : assertionErrors) {
             Logging.error(msg);
@@ -205 +229 @@
                 "  assertNoMatch: \"node amenity=restaurant\";\n" +
                 "}");
-        Set<String> errors = MapCSSTagCheckerAsserts.checkAsserts(test.checks.get("test"));
-        assertTrue(errors.toString(), errors.isEmpty());
+        assertNotNull(test);
     }
 
@@ -221 +244 @@
                 "  assertNoMatch: \"way name=Hauptstrasse\";\n" +
                 "}");
-        Set<String> errors = MapCSSTagCheckerAsserts.checkAsserts(test.checks.get("test"));
-        assertTrue(errors.toString(), errors.isEmpty());
+        assertNotNull(test);
     }