Changeset 19152 in josm for trunk/test

Timestamp:

2024-07-25T20:56:22+02:00 (4 months ago)

Author:

taylor.smock

Message:

See #23821: Refactor LoadAndZoomHandler and add tests

This fixes some lint issues, specifically in regard to method complexity.

Location:

trunk/test/unit/org/openstreetmap/josm

Files:

• io/remotecontrol/handler/LoadAndZoomHandlerTest.java (modified) (4 diffs)
• testutils/annotations/BasicWiremock.java (modified) (14 diffs)

trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.io.remotecontrol.handler;
 
+import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
-
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Collection;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
+import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
+import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
+import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.io.remotecontrol.handler.RequestHandler.RequestHandlerBadRequestException;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 
 import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.testutils.annotations.BasicWiremock;
+import org.openstreetmap.josm.testutils.annotations.Main;
+import org.openstreetmap.josm.testutils.annotations.Projection;
+import org.openstreetmap.josm.testutils.annotations.ThreadSync;
 
 /**
  * Unit tests of {@link LoadAndZoomHandler} class.
  */
+@BasicPreferences
+@BasicWiremock
+@ExtendWith(BasicWiremock.OsmApiExtension.class)
 class LoadAndZoomHandlerTest {
+    private static final String DEFAULT_BBOX_URL = "https://localhost/load_and_zoom?left=0&bottom=0&right=0.001&top=0.001";
     private static LoadAndZoomHandler newHandler(String url) throws RequestHandlerBadRequestException {
         LoadAndZoomHandler req = new LoadAndZoomHandler();
+        req.myCommand = LoadAndZoomHandler.command;
         if (url != null)
             req.setUrl(url);
@@ -22 +49 @@
     }
 
+    private static void syncThreads() {
+        // There are calls to the worker thread and EDT
+        new ThreadSync.ThreadSyncExtension().threadSync();
+    }
+
+    @BeforeEach
+    void setup(WireMockRuntimeInfo wireMockRuntimeInfo) {
+        String common = "visible=\"true\" version=\"1\" changeset=\"1\" timestamp=\"2000-01-01T00:00:00Z\" user=\"tsmock\" uid=\"1\"";
+        wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/0.6/map?bbox=0.0,0.0,0.001,0.001")
+                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
+                        " <bounds minlat=\"0\" minlon=\"0\" maxlat=\"0.001\" maxlon=\"0.001\"/>\n" +
+                        " <node id=\"1\" " + common + " lat=\"0\" lon=\"0\"/>\n" +
+                        " <node id=\"2\" " + common + " lat=\"0.0001\" lon=\"0.0001\"/>\n" +
+                        " <node id=\"3\" " + common + " lat=\"0.0002\" lon=\"0.0002\"/>\n" +
+                        "</osm>")));
+    }
+
     /**
      * Unit test for bad request - no param.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestNoParam() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler(null).handle());
+     */
+    @Test
+    void testBadRequestNoParam() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler(null));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("NumberFormatException (empty String)", e.getMessage());
     }
@@ -34 +79 @@
     /**
      * Unit test for bad request - invalid URL.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestInvalidUrl() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler("invalid_url").handle());
+     */
+    @Test
+    void testBadRequestInvalidUrl() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler("invalid_url"));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("The following keys are mandatory, but have not been provided: bottom, top, left, right", e.getMessage());
     }
@@ -44 +89 @@
     /**
      * Unit test for bad request - incomplete URL.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    void testBadRequestIncompleteUrl() throws Exception {
-        Exception e = assertThrows(RequestHandlerBadRequestException.class, () -> newHandler("https://localhost").handle());
+     */
+    @Test
+    void testBadRequestIncompleteUrl() {
+        final LoadAndZoomHandler handler = assertDoesNotThrow(() -> newHandler("https://localhost"));
+        Exception e = assertThrows(RequestHandlerBadRequestException.class, handler::handle);
         assertEquals("The following keys are mandatory, but have not been provided: bottom, top, left, right", e.getMessage());
     }
 
     /**
-     * Unit test for nominal request - local data file.
-     * @throws Exception if any error occurs
-     */
-    @Test
-    @BasicPreferences
-    void testNominalRequest() throws Exception {
-        assertDoesNotThrow(() -> newHandler("https://localhost?bottom=0&top=0&left=1&right=1").handle());
+     * Ensure that a download is called and completed
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testDownload(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertNotNull(ds);
+        assertAll(() -> assertNotNull(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(2, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(3, OsmPrimitiveType.NODE)),
+                () -> assertNull(ds.getPrimitiveById(4, OsmPrimitiveType.NODE)),
+                () -> assertTrue(ds.selectionEmpty()));
+        wireMockRuntimeInfo.getWireMock().verifyThat(1,
+                RequestPatternBuilder.newRequestPattern().withUrl("/api/0.6/map?bbox=0.0,0.0,0.001,0.001"));
+    }
+
+    /**
+     * Ensure that an area isn't downloaded twice
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testDoubleDownload(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        testDownload(wireMockRuntimeInfo);
+        testDownload(wireMockRuntimeInfo);
+        // testDownload checks that the URL has been called once. Since it doesn't reset anything, we don't need
+        // a specific test here.
+    }
+
+    /**
+     * Ensure that an overlapping area is trimmed before download
+     * @param wireMockRuntimeInfo The wiremock information
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testOverlappingArea(WireMockRuntimeInfo wireMockRuntimeInfo) throws RequestHandlerBadRequestException {
+        LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        // The scientific notation is ok server-side.
+        final String mapCall = "/api/0.6/map?bbox=2.5E-4,0.001,7.5E-4,0.00125";
+        final String commonNode = "visible=\"true\" version=\"1\" changeset=\"1\" timestamp=\"2000-01-01T00:00:00Z\" user=\"tsmock\" uid=\"1\"";
+        wireMockRuntimeInfo.getWireMock().register(WireMock.get(mapCall)
+                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
+                        " <bounds minlat=\"0.001\" minlon=\"0.00025\" maxlat=\"0.00125\" maxlon=\"0.00075\"/>\n" +
+                        " <node id=\"4\" " + commonNode + " lat=\"0.00111\" lon=\"0.00026\"/>\n" +
+                        " <node id=\"5\" " + commonNode + " lat=\"0.0011\" lon=\"0.00025\"/>\n" +
+                        " <node id=\"6\" " + commonNode + " lat=\"0.0012\" lon=\"0.000251\"/>\n" +
+                        "</osm>")));
+        String request = "https://localhost/load_and_zoom?left=0.00025&bottom=0.00025&right=0.00075&top=0.00125";
+        handler = newHandler(request);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        wireMockRuntimeInfo.getWireMock().verifyThat(1, RequestPatternBuilder.newRequestPattern().withUrl(mapCall));
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertNotNull(ds);
+        assertAll(() -> assertNotNull(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(2, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(3, OsmPrimitiveType.NODE)),
+                () -> assertNotNull(ds.getPrimitiveById(4, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check search and zoom functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Main
+    @Projection
+    @Test
+    void testSearchAndZoom() throws RequestHandlerBadRequestException {
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL + "&search=id:1");
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        Collection<OsmPrimitive> selected = ds.getSelected();
+        assertEquals(1, selected.size());
+        assertTrue(selected.contains(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+        assertTrue(ds.searchNodes(MainApplication.getMap().mapView.getRealBounds().toBBox())
+                .contains((Node) ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check select and zoom functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Main
+    @Projection
+    @Test
+    void testSelectAndZoom() throws RequestHandlerBadRequestException {
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL + "&select=n1");
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        Collection<OsmPrimitive> selected = ds.getSelected();
+        assertEquals(1, selected.size());
+        assertTrue(selected.contains(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+        assertTrue(ds.searchNodes(MainApplication.getMap().mapView.getRealBounds().toBBox())
+                .contains((Node) ds.getPrimitiveById(1, OsmPrimitiveType.NODE)));
+    }
+
+    /**
+     * Check changeset tag functionality
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testChangesetTags() throws RequestHandlerBadRequestException {
+        final String comment = "Add buildings, roads, and other random stuff";
+        final String source = "This isn't Bing";
+        final String hashtag = "#test-hashcodes";
+        final String customTags = "custom=tag|is=here";
+        final LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL
+                + "&changeset_comment=" + URLEncoder.encode(comment, StandardCharsets.UTF_8)
+                + "&changeset_source=" + URLEncoder.encode(source, StandardCharsets.UTF_8)
+                + "&changeset_hashtags=" + URLEncoder.encode(hashtag, StandardCharsets.UTF_8)
+                + "&changeset_tags=" + URLEncoder.encode(customTags, StandardCharsets.UTF_8));
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertEquals(comment, ds.getChangeSetTags().get("comment"));
+        assertEquals(source, ds.getChangeSetTags().get("source"));
+        assertEquals(hashtag, ds.getChangeSetTags().get("hashtags"));
+        assertEquals("tag", ds.getChangeSetTags().get("custom"));
+        assertEquals("here", ds.getChangeSetTags().get("is"));
     }
 }

trunk/test/unit/org/openstreetmap/josm/testutils/annotations/BasicWiremock.java

@@ -3 +3 @@
 
 import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -19 +19 @@
 import java.util.Arrays;
 import java.util.List;
-import java.util.stream.Collectors;
-
+
+import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
+import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
 import org.junit.jupiter.api.extension.AfterAllCallback;
-import org.junit.jupiter.api.extension.AfterEachCallback;
 import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.BeforeEachCallback;
@@ -29 +29 @@
 import org.junit.jupiter.api.extension.ParameterContext;
 import org.junit.jupiter.api.extension.ParameterResolutionException;
-import org.junit.jupiter.api.extension.ParameterResolver;
 import org.junit.platform.commons.support.AnnotationSupport;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
-import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.io.OsmApi;
 import org.openstreetmap.josm.spi.preferences.Config;
@@ -43 +41 @@
 import com.github.tomakehurst.wiremock.client.WireMock;
 import com.github.tomakehurst.wiremock.extension.ResponseTransformerV2;
-import com.github.tomakehurst.wiremock.verification.LoggedRequest;
 
 /**
@@ -81 +78 @@
      *
      */
-    class WireMockExtension
-            implements AfterAllCallback, AfterEachCallback, BeforeAllCallback, BeforeEachCallback, ParameterResolver {
+    class WireMockExtension extends com.github.tomakehurst.wiremock.junit5.WireMockExtension {
+        protected WireMockExtension() {
+            this(defaultOptions());
+        }
+
+        /**
+         * Create a new extension with options
+         *
+         * @param builder a {@link Builder}
+         *                instance holding the initialisation parameters for the extension.
+         */
+        protected WireMockExtension(Builder builder) {
+            super(builder);
+        }
+
         /**
          * Get the default wiremock server
@@ -98 +108 @@
                             new Pair<>(new Class<?>[] {ExtensionContext.class }, new Object[] {context }),
                             new Pair<>(new Class<?>[0], new Object[0]))) {
-                        try {
-                            Constructor<? extends ResponseTransformerV2> constructor = responseTransformer
-                                    .getConstructor(parameterMapping.a);
-                            transformers.add(constructor.newInstance(parameterMapping.b));
-                            break;
-                        } catch (ReflectiveOperationException e) {
-                            fail(e);
-                        }
+                        Constructor<? extends ResponseTransformerV2> constructor = assertDoesNotThrow(() ->
+                                responseTransformer.getConstructor(parameterMapping.a));
+                        ResponseTransformerV2 transformerV2 = assertDoesNotThrow(() -> constructor.newInstance(parameterMapping.b));
+                        transformers.add(transformerV2);
+                        break;
                     }
                 }
@@ -114 +121 @@
         }
 
+        static Builder defaultOptions() {
+            WireMockConfiguration options = WireMockConfiguration.options()
+                    .usingFilesUnderDirectory(TestUtils.getTestDataRoot())
+                    .dynamicPort();
+            return extensionOptions().options(options);
+        }
+
         /**
          * Replace URL servers with wiremock
@@ -120 +134 @@
          * @param url            The URL to fix
          * @return A url that points at the wiremock server
+         * @deprecated since 19152 (not used in core; no known users)
          */
+        @Deprecated(forRemoval = true, since = "19152")
         public static String replaceUrl(WireMockServer wireMockServer, String url) {
             try {
@@ -132 +148 @@
 
         @Override
-        public void afterAll(ExtensionContext context) throws Exception {
-            // Run in EDT to avoid stopping wiremock server before wiremock requests finish.
-            GuiHelper.runInEDTAndWait(getWiremock(context)::stop);
-        }
-
-        @Override
-        public void afterEach(ExtensionContext context) throws Exception {
-            List<LoggedRequest> missed = getWiremock(context).findUnmatchedRequests().getRequests();
-            missed.forEach(r -> Logging.error(r.getAbsoluteUrl()));
-            try {
-                assertTrue(missed.isEmpty(), missed.stream().map(LoggedRequest::getUrl).collect(Collectors.joining("\n\n")));
-            } finally {
-                getWiremock(context).resetRequests();
-                getWiremock(context).resetToDefaultMappings();
-                getWiremock(context).resetScenarios();
-                if (AnnotationUtils.elementIsAnnotated(context.getElement(), BasicWiremock.class)
-                        || getWiremock(context) == null) {
-                    this.afterAll(context);
-                }
-            }
-        }
-
-        @Override
-        public void beforeAll(ExtensionContext context) throws Exception {
-            getWiremock(context).start();
-        }
-
-        @Override
-        public void beforeEach(ExtensionContext context) throws Exception {
-            if (AnnotationUtils.elementIsAnnotated(context.getElement(), BasicWiremock.class) || getWiremock(context) == null) {
-                this.beforeAll(context);
-            }
+        protected void onBeforeAll(ExtensionContext extensionContext, WireMockRuntimeInfo wireMockRuntimeInfo) {
+            extensionContext.getStore(ExtensionContext.Namespace.create(BasicWiremock.WireMockExtension.class))
+                    .put(BasicWiremock.WireMockExtension.class, this);
+        }
+
+        @Override
+        protected void onAfterAll(ExtensionContext extensionContext, WireMockRuntimeInfo wireMockRuntimeInfo) {
+            // Sync threads to ensure that no further wiremock requests will be made
+            final ThreadSync.ThreadSyncExtension threadSyncExtension = new ThreadSync.ThreadSyncExtension();
+            assertDoesNotThrow(() -> threadSyncExtension.afterEach(extensionContext));
+        }
+
+        @Override
+        protected void onBeforeEach(ExtensionContext context, WireMockRuntimeInfo wireMockRuntimeInfo) {
             if (context.getTestClass().isPresent()) {
                 List<Field> wireMockFields = AnnotationSupport.findAnnotatedFields(context.getRequiredTestClass(), BasicWiremock.class);
@@ -172 +170 @@
                         try {
                             field.set(context.getTestInstance().orElse(null), getWiremock(context));
+                        } catch (IllegalAccessException e) {
+                            fail(e);
                         } finally {
                             field.setAccessible(isAccessible);
@@ -185 +185 @@
         public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
                 throws ParameterResolutionException {
+            if (super.supportsParameter(parameterContext, extensionContext)) {
+                return true;
+            }
+            if (WireMockRuntimeInfo.class.isAssignableFrom(parameterContext.getParameter().getType())) {
+                return true;
+            }
             return parameterContext.getParameter().getAnnotation(BasicWiremock.class) != null
                     && parameterContext.getParameter().getType() == WireMockServer.class;
@@ -192 +198 @@
         public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
                 throws ParameterResolutionException {
+            if (super.supportsParameter(parameterContext, extensionContext)) {
+                return super.resolveParameter(parameterContext, extensionContext);
+            }
+            if (WireMockRuntimeInfo.class.isAssignableFrom(parameterContext.getParameter().getType())) {
+                return getRuntimeInfo();
+            }
             return getWiremock(extensionContext);
         }
@@ -199 +211 @@
      * A class specifically to mock OSM API calls
      */
-    class OsmApiExtension extends WireMockExtension {
-        @Override
-        public void afterAll(ExtensionContext context) throws Exception {
-            try {
-                super.afterAll(context);
-            } finally {
-                Config.getPref().put("osm-server.url", "https://invalid.url");
-            }
+    class OsmApiExtension implements BeforeAllCallback, BeforeEachCallback, AfterAllCallback {
+        @Override
+        public void afterAll(ExtensionContext context) {
+            Config.getPref().put("osm-server.url", "https://invalid.url");
         }
 
@@ -214 +222 @@
                 fail("OsmApiExtension requires @BasicPreferences");
             }
-            super.beforeAll(context);
-            Config.getPref().put("osm-server.url", getWiremock(context).baseUrl() + "/api");
-            getWiremock(context).stubFor(WireMock.get("/api/0.6/capabilities")
+            this.beforeEach(context);
+        }
+
+        @Override
+        public void beforeEach(ExtensionContext extensionContext) throws Exception {
+            BasicWiremock.WireMockExtension extension =
+                    extensionContext.getStore(ExtensionContext.Namespace.create(BasicWiremock.WireMockExtension.class))
+                    .get(BasicWiremock.WireMockExtension.class, BasicWiremock.WireMockExtension.class);
+            WireMockRuntimeInfo wireMockRuntimeInfo = extension.getRuntimeInfo();
+            Config.getPref().put("osm-server.url", wireMockRuntimeInfo.getHttpBaseUrl() + "/api");
+            wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/0.6/capabilities")
                     .willReturn(WireMock.aResponse().withBodyFile("api/0.6/capabilities")));
-            getWiremock(context).stubFor(WireMock.get("/api/capabilities")
+            wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/capabilities")
                     .willReturn(WireMock.aResponse().withBodyFile("api/capabilities")));
-            OsmApi.getOsmApi().initialize(NullProgressMonitor.INSTANCE);
+            assertDoesNotThrow(() -> OsmApi.getOsmApi().initialize(NullProgressMonitor.INSTANCE));
         }
     }