Index: /trunk/src/org/openstreetmap/josm/gui/download/PlaceSelection.java =================================================================== --- /trunk/src/org/openstreetmap/josm/gui/download/PlaceSelection.java (revision 18804) +++ /trunk/src/org/openstreetmap/josm/gui/download/PlaceSelection.java (revision 18805) @@ -83,8 +83,8 @@ private static class Server { - public final String name; - public final BiFunction, URL> urlFunction; - public final String thirdcol; - public final String fourthcol; + final String name; + final BiFunction, URL> urlFunction; + final String thirdcol; + final String fourthcol; Server(String n, BiFunction, URL> u, String t, String f) { @@ -133,5 +133,5 @@ /** * Adds a new tab to the download dialog in JOSM. - * + *

* This method is, for all intents and purposes, the constructor for this class. */ @@ -192,5 +192,5 @@ public void actionPerformed(ActionEvent e) { String searchExpression = cbSearchExpression.getText(); - if (!isEnabled() || searchExpression.trim().isEmpty()) + if (!isEnabled() || searchExpression.trim().isEmpty() || serverComboBox.getSelectedItem() == null) return; cbSearchExpression.addCurrentItemToHistory(); @@ -298,11 +298,21 @@ JOptionPane.WARNING_MESSAGE, null )); + generateLastException(e); } } catch (IOException | ParserConfigurationException e) { - if (!canceled) { - OsmTransferException ex = new OsmTransferException(e); - ex.setUrl(url.toString()); - lastException = ex; - } + generateLastException(e); + } + } + + /** + * Generate an {@link OsmTransferException} that will be stored in {@link #lastException} if the operation is + * not cancelled. + * @param throwable The throwable to store as an {@link OsmTransferException} + */ + private void generateLastException(Throwable throwable) { + if (!canceled) { + OsmTransferException ex = new OsmTransferException(throwable); + ex.setUrl(url.toString()); + lastException = ex; } } @@ -337,4 +347,8 @@ } + /** + * Add data to the table + * @param data The data to add + */ public void addData(List data) { this.data.addAll(data); @@ -403,4 +417,9 @@ } + /** + * Set the header column values for the third and fourth columns + * @param third The new header for the third column + * @param fourth The new header for the fourth column + */ public void setHeadlines(String third, String fourth) { col3.setHeaderValue(third); Index: /trunk/test/unit/org/openstreetmap/josm/gui/download/PlaceSelectionTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/gui/download/PlaceSelectionTest.java (revision 18804) +++ /trunk/test/unit/org/openstreetmap/josm/gui/download/PlaceSelectionTest.java (revision 18805) @@ -2,8 +2,43 @@ package org.openstreetmap.josm.gui.download; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.IOException; +import java.io.Reader; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; +import java.util.stream.Stream; + +import javax.swing.JButton; +import javax.swing.JPanel; +import javax.xml.parsers.ParserConfigurationException; + +import org.awaitility.Awaitility; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.platform.commons.support.ReflectionSupport; +import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.data.Bounds; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.util.GuiHelper; +import org.openstreetmap.josm.gui.widgets.HistoryComboBox; +import org.openstreetmap.josm.io.NameFinder; import org.openstreetmap.josm.testutils.annotations.BasicPreferences; +import org.openstreetmap.josm.testutils.mockers.BugReportMock; +import org.openstreetmap.josm.testutils.mockers.ImageProviderMock; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; -import org.junit.jupiter.api.Test; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import mockit.Mock; +import mockit.MockUp; /** @@ -11,4 +46,5 @@ */ @BasicPreferences +@WireMockTest class PlaceSelectionTest { /** @@ -22,3 +58,84 @@ sel.setDownloadArea(new Bounds(0, 0, 1, 1)); } + + static Stream testTicket23117() { + return Stream.of(Arguments.of(SAXParseException.class, (Supplier) () -> new SAXParseException("", "", "", 0, 0)), + Arguments.of(IOException.class, (Supplier) IOException::new), + Arguments.of(ParserConfigurationException.class, (Supplier) ParserConfigurationException::new)); + } + + /** + * This checks to make certain that an exception thrown in parsing does not cause a {@link RuntimeException}. + * @param expectedThrowable The expected throwable class, mostly used to indicate which throwable was under test + * @param exceptionSupplier The supplier for the throwable + * @param wireMockRuntimeInfo The wiremock to avoid network requests + */ + @ParameterizedTest + @MethodSource + void testTicket23117(Class expectedThrowable, Supplier exceptionSupplier, + WireMockRuntimeInfo wireMockRuntimeInfo) throws Exception { + TestUtils.assumeWorkingJMockit(); + NameFinder.NOMINATIM_URL_PROP.put(wireMockRuntimeInfo.getHttpBaseUrl() + '/'); + // We just want to return something quickly so we can get to the place where we throw an exception in a mock. + wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.anyUrl()).willReturn(WireMock.aResponse())); + new ImageProviderMock(); + final NameFinderMock nameFinderMock = new NameFinderMock(exceptionSupplier); + final BugReportMock bugReportMock = new BugReportMock(); + final PlaceSelection placeSelection = new PlaceSelection(); + // We need to call addGui prior to buildSearchPanel -- we really want the panel, but it needs objects initialized + // in addGui + placeSelection.addGui(null); + final JPanel searchPanel = placeSelection.buildSearchPanel(); + final HistoryComboBox cbSearchExpression = (HistoryComboBox) ((JPanel) searchPanel.getComponent(0)).getComponent(3); + final PlaceSelection.SearchAction search = (PlaceSelection.SearchAction) ((JButton) searchPanel.getComponent(1)).getAction(); + final PlaceSelection.NamedResultTableModel model = (PlaceSelection.NamedResultTableModel) + ReflectionSupport.tryToReadFieldValue(PlaceSelection.class.getDeclaredField("model"), placeSelection).get(); + + // This is needed in order to enable the action + cbSearchExpression.setText("Somewhere, Podunk"); + search.updateState(); + search.actionPerformed(null); + // Sync threads + final AtomicBoolean threadSync = new AtomicBoolean(); + MainApplication.worker.execute(() -> GuiHelper.runInEDTAndWaitWithException(() -> threadSync.set(true))); + Awaitility.await().untilTrue(threadSync); + model.addData(Collections.singletonList(new NameFinder.SearchResult())); + assertEquals(1, nameFinderMock.calledTimes); + // Call the search again -- this is needed since we need to be in the "Search more..." code paths + search.updateState(); + search.actionPerformed(null); + // Sync threads again + threadSync.set(false); + MainApplication.worker.execute(() -> GuiHelper.runInEDTAndWaitWithException(() -> threadSync.set(true))); + Awaitility.await().untilTrue(threadSync); + assertEquals(2, nameFinderMock.calledTimes); + if (bugReportMock.throwable() != null) { + assertInstanceOf(expectedThrowable, bugReportMock.throwable().getCause(), + "Some explanations will cause a bug report window. " + + "In those cases, the expected throwable should be the same class as the cause."); + } + } + + private static class NameFinderMock extends MockUp { + private final Supplier exceptionSupplier; + int calledTimes; + NameFinderMock(Supplier exceptionSupplier) { + this.exceptionSupplier = exceptionSupplier; + } + + @Mock + public List parseSearchResults(Reader reader) throws IOException, ParserConfigurationException, SAXException { + this.calledTimes++; + final Throwable throwable = this.exceptionSupplier.get(); + if (throwable instanceof IOException) { + throw (IOException) throwable; + } else if (throwable instanceof ParserConfigurationException) { + throw (ParserConfigurationException) throwable; + } else if (throwable instanceof SAXException) { + throw (SAXException) throwable; + } + fail("This method does not throw the specified throwable", throwable); + return Collections.emptyList(); + } + } } Index: /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/BugReportMock.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/BugReportMock.java (revision 18805) +++ /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/BugReportMock.java (revision 18805) @@ -0,0 +1,31 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.testutils.mockers; + +import org.openstreetmap.josm.tools.bugreport.BugReport; +import org.openstreetmap.josm.tools.bugreport.ReportedException; + +import mockit.Invocation; +import mockit.Mock; +import mockit.MockUp; + +/** + * Intercept bug report interceptions. This is useful if you have some code paths that should call {@link BugReport#intercept(Throwable)}, + * and you want to verify that the "right" exception was thrown. + */ +public class BugReportMock extends MockUp { + private Throwable throwable; + + @Mock + public ReportedException intercept(Invocation invocation, Throwable t) { + this.throwable = t; + return invocation.proceed(t); + } + + /** + * Get the throwable that was intercepted + * @return The intercepted throwable + */ + public Throwable throwable() { + return this.throwable; + } +} Index: /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/ImageProviderMock.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/ImageProviderMock.java (revision 18805) +++ /trunk/test/unit/org/openstreetmap/josm/testutils/mockers/ImageProviderMock.java (revision 18805) @@ -0,0 +1,24 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.testutils.mockers; + +import java.awt.Image; + +import org.openstreetmap.josm.tools.ImageProvider; +import org.openstreetmap.josm.tools.ImageResource; + +import mockit.Mock; +import mockit.MockUp; + +/** + * Mock out {@link ImageProvider#getResource()} calls, which can be very expensive for SVGs. + * Mostly useful for speeding up tests. If you are testing rendering, do not call this mock! + */ +public class ImageProviderMock extends MockUp { + + private static final Image EMPTY_IMAGE = ImageProvider.getEmpty(ImageProvider.ImageSizes.DEFAULT).getImage(); + + @Mock + public ImageResource getResource() { + return new ImageResource(EMPTY_IMAGE); + } +}