See #23049, fix an issue where tests might visit the entire dataset during upload

git-svn-id: https://josm.openstreetmap.de/svn/trunk@18776 0c6e7542-c601-0410-84e7-c038aed88b3b

Author: taylor.smock

src/org/openstreetmap/josm/actions/upload/ValidateUploadHook.java

@@ -14,9 +14,7 @@
 
 import org.openstreetmap.josm.data.APIDataSet;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
-import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
 import org.openstreetmap.josm.data.validation.OsmValidator;
-import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.data.validation.ValidationTask;
 import org.openstreetmap.josm.data.validation.util.AggregatePrimitivesVisitor;
@@ -52,10 +50,6 @@ public boolean checkUpload(APIDataSet apiDataSet) {
         Collection<OsmPrimitive> visited = v.visit(apiDataSet.getPrimitivesToUpdate());
         OsmValidator.initializeTests();
         new ValidationTask(errors -> {
-            if (!Boolean.TRUE.equals(ValidatorPrefHelper.PREF_OTHER_UPLOAD.get())) {
-                // Use >= just in case we add additional levels.
-                errors.removeIf(error -> error.getSeverity().getLevel() >= Severity.OTHER.getLevel());
-            }
             if (errors.stream().allMatch(TestError::isIgnored)) {
                 returnCode.set(true);
             } else {

src/org/openstreetmap/josm/data/validation/ValidationTask.java

@@ -107,6 +107,12 @@ protected void finish() {
             });
         }
         if (this.onFinish != null) {
+            // Remove any low severity issues if they are not desired.
+            if (!(Boolean.TRUE.equals(ValidatorPrefHelper.PREF_OTHER.get()) &&
+                    (!this.beforeUpload || Boolean.TRUE.equals(ValidatorPrefHelper.PREF_OTHER_UPLOAD.get())))) {
+                // Use >= just in case we add additional levels.
+                this.errors.removeIf(error -> error.getSeverity().getLevel() >= Severity.OTHER.getLevel());
+            }
             this.onFinish.accept(this.errors);
         }
     }
@@ -124,7 +130,8 @@ protected void realRun() {
             testCounter++;
             getProgressMonitor().setCustomText(tr("Test {0}/{1}: Starting {2}", testCounter, tests.size(), test.getName()));
             test.setBeforeUpload(this.beforeUpload);
-            test.setPartialSelection(formerValidatedPrimitives != null);
+            // Pre-upload checks only run on a partial selection.
+            test.setPartialSelection(this.beforeUpload || formerValidatedPrimitives != null);
             test.startTest(getProgressMonitor().createSubTaskMonitor(validatedPrimitives.size(), false));
             test.visit(validatedPrimitives);
             test.endTest();

test/unit/org/openstreetmap/josm/actions/upload/ValidateUploadHookTest.java

@@ -5,13 +5,15 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Collections;
+import java.util.stream.Stream;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import mockit.Mock;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.ValueSource;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.APIDataSet;
 import org.openstreetmap.josm.data.Bounds;
@@ -47,10 +49,20 @@ void testCheckUpload() {
         assertTrue(new ValidateUploadHook().checkUpload(new APIDataSet()));
     }
 
+    static Stream<Arguments> testUploadOtherErrors() {
+        return Stream.of(
+                Arguments.of(true, true),
+                Arguments.of(true, false),
+                Arguments.of(false, false),
+                Arguments.of(false, true)
+        );
+    }
+
     @ParameterizedTest
-    @ValueSource(booleans = {true, false})
-    void testUploadOtherErrors(boolean otherEnabled) {
-        ValidatorPrefHelper.PREF_OTHER_UPLOAD.put(otherEnabled);
+    @MethodSource
+    void testUploadOtherErrors(boolean otherUploadEnabled, boolean otherEnabled) {
+        ValidatorPrefHelper.PREF_OTHER.put(otherEnabled);
+        ValidatorPrefHelper.PREF_OTHER_UPLOAD.put(otherUploadEnabled);
         final DataSet ds = new DataSet();
         final Way building = TestUtils.newWay("building=yes", new Node(new LatLon(33.2287665, -111.8259225)),
                 new Node(new LatLon(33.2287335, -111.8257513)), new Node(new LatLon(33.2285316, -111.8258086)),
@@ -73,6 +85,6 @@ public void dispose() {
                     }
                 };
         new ValidateUploadHook().checkUpload(new APIDataSet(ds));
-        assertEquals(!otherEnabled, mocker.getInvocationLog().isEmpty());
+        assertEquals(!(otherEnabled && otherUploadEnabled), mocker.getInvocationLog().isEmpty());
     }
 }