Unit Test MarkerLayerTest fails in Germany

[GerdP]

I run the test with

ant -Ddefault-junit-includes="**/**/Marker*.class" test

and it seems to fail because my Windows System is configured to German.

test:
     [echo] Running unit tests with JUnit
[junitlauncher]
[junitlauncher] Test run finished after 5873 ms
[junitlauncher] [         4 containers found      ]
[junitlauncher] [         0 containers skipped    ]
[junitlauncher] [         4 containers started    ]
[junitlauncher] [         0 containers aborted    ]
[junitlauncher] [         4 containers successful ]
[junitlauncher] [         0 containers failed     ]
[junitlauncher] [         3 tests found           ]
[junitlauncher] [         0 tests skipped         ]
[junitlauncher] [         3 tests started         ]
[junitlauncher] [         0 tests aborted         ]
[junitlauncher] [         2 tests successful      ]
[junitlauncher] [         1 tests failed          ]
[junitlauncher]
     [echo] Running functional tests with JUnit

BUILD SUCCESSFUL

[GerdP]

output in test\report

[GerdP]

It would be good if the unit tests would ignore the local language configuration, or maybe I can set an envirentment variable like

LANG=en_US.UTF-8
LANGUAGE=en_US

So far nothing seemed to help.

[taylor.smock]

I was able to reproduce on mac with spanish set as the language.

I think the problem is in the @I18n code. I think I know how to fix it.

[GerdP]

Sorry, didn't see the patch. It seems to work well.

[GerdP]

Or maybe not? This looks like another i18n problem:

Test: testRegionKey() took 31 milli sec(s) FAILED: expected: <Preset Pedestrian Crossing should not have the key crossing_ref> but was: <Preset Fußgängerüberweg should not have the key crossing_ref>
org.opentest4j.AssertionFailedError: expected: <Preset Pedestrian Crossing should not have the key crossing_ref> but was: <Preset Fußgängerüberweg should not have the key crossing_ref>
	at org.openstreetmap.josm.data.validation.tests.TagCheckerTest.testRegionKey(TagCheckerTest.java:236)

Or is this caused by the fact that I am executing the unit test in Germany and not in the UK?

[taylor.smock]

Replying to GerdP:

Or maybe not? This looks like another i18n problem:

We can probably fix that by adding @I18n to the JosmDefaults class or (preferentially) adding @I18n to the test class or methods that are failing.

I suspect part of the problem is that most of the tests were written by people using English as their primary language or with their computer set to English as the default.

When I was writing the annotations, I was trying to keep them as close as possible to the JOSMTestRules class behavior while attempting to reduce or remove test pollution sources.

The problem here being I want to reset the i18n language after tests which require the i18n to be english so that a test run individually should have the same behavior as a test run with other tests.

[GerdP]

I tried adding @I18n like this, but that didn't help on the comamnd line.

Index: test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java
===================================================================
--- test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java  (revision 18957)
+++ test/unit/org/openstreetmap/josm/data/validation/tests/TagCheckerTest.java  (working copy)
@@ -19,11 +19,13 @@
 import org.openstreetmap.josm.data.osm.Tag;
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.testutils.annotations.I18n;
 import org.openstreetmap.josm.testutils.annotations.TaggingPresets;

 /**
  * JUnit Test of {@link TagChecker}.
  */
+@I18n
 @TaggingPresets
 class TagCheckerTest {
     List<TestError> test(OsmPrimitive primitive) throws IOException {

The command I used here:

ant -Ddefault-junit-includes=org/openstreetmap/josm/data/validation/tests/*.class test

The log for the (only) failing test:

Testcase: org.openstreetmap.josm.data.validation.tests.TagCheckerTest
Test: testValueDifferentCase() took 32 milli sec(s)
Test: testMisspelledTag() took 24 milli sec(s)
Test: testObjectTypeNotSupportedByPreset() took 35 milli sec(s)
Test: testUpperCaseInKeyIgnoredTag() took 21 milli sec(s)
Test: testContainsRemoveUnwantedNonprintingControlCharacters() took 10 milli sec(s)
Test: testMisspelledKeyButAlternativeInUse() took 22 milli sec(s)
Test: testShortValNotInPreset2() took 19 milli sec(s)
Test: testRegression17246() took 17 milli sec(s)
Test: testTicket17667() took 6 milli sec(s)
Test: testTicket18322() took 6 milli sec(s)
Test: testTicket18449() took 6 milli sec(s)
Test: testTicket18740() took 5 milli sec(s)
Test: testTicket19519() took 0 milli sec(s) SKIPPED: broken, see #19519
Test: testTicket20437() took 5 milli sec(s)
Test: testTicket20754() took 5 milli sec(s)
Test: testTicket21348() took 19 milli sec(s)
Test: testTranslatedNameKey() took 17 milli sec(s)
Test: testIgnoredTagsNotInPresets() took 17 milli sec(s)
Test: testRegionKey() took 29 milli sec(s) FAILED: expected: <Preset Pedestrian Crossing should not have the key crossing_ref> but was: <Preset Fußgängerüberweg should not have the key crossing_ref>
org.opentest4j.AssertionFailedError: expected: <Preset Pedestrian Crossing should not have the key crossing_ref> but was: <Preset Fußgängerüberweg should not have the key crossing_ref>
	at org.openstreetmap.josm.data.validation.tests.TagCheckerTest.testRegionKey(TagCheckerTest.java:238)

Test: testRegionTag() took 19 milli sec(s) FAILED: expected: <Preset Waterway should not have the key ref:gnis> but was: <Preset Gewässer should not have the key ref:gnis>
org.opentest4j.AssertionFailedError: expected: <Preset Waterway should not have the key ref:gnis> but was: <Preset Gewässer should not have the key ref:gnis>
	at org.openstreetmap.josm.data.validation.tests.TagCheckerTest.testRegionTag(TagCheckerTest.java:249)

Test: testTooShortToFix() took 18 milli sec(s)
Test: testUpperCaseIgnoredKey() took 17 milli sec(s)
Test: testMisspelledKey1() took 17 milli sec(s)
Test: testMisspelledKey2() took 17 milli sec(s)
Test: testMisspelledTag2() took 17 milli sec(s)
Test: testMisspelledTag3() took 17 milli sec(s)
Tests run: 25, Failures: 2, Skipped: 1, Aborted: 0, Time elapsed: 492 milli sec(s)

[taylor.smock]

I wasn't able to repro the failing test on my machine.

EDIT: NVM. I misread the terminal output.

[GerdP]

BTW: In this case the test works in Eclipse, even with the additional @I18n

[GerdP]

Replying to GerdP:

BTW: In this case the test works in Eclipse, even without the additional @I18n

[taylor.smock]

It looks like this is due to caching in @TaggingPresets which was done to speed up the test suite. I've got a fix, but I'm going to wait for the full test suite to run this time.

[taylor.smock]

Modify @TaggingPresets to reread presets when locale changes

[taylor.smock]

There are a couple of other tests that seem to be failing due to I18n issues. But I think those might be slightly different (example: DateUtilsTest failed since the date format didn't match).

I'll try running those with @I18n and see what happens. If they pass, I'll commit the changes.

[taylor.smock]

In 18958/josm:

Fix #23418: Improve unit test consistency when run with non-English locales

This also fixes some tests that fail when run individually.

[skyper]

I am not sure if the last changes are to blame but the i18n test seem to be run constantly (every 10-15 minutes), now.

[taylor.smock]

Replying to skyper:

I am not sure if the last changes are to blame but the i18n test seem to be run constantly (every 10-15 minutes), now.

It doesn't look like it. Somehow the i18n-launcher job is set to run every 15 minutes. I didn't change it (maybe stoecker did?). I'll give him an opportunity to respond (maybe he is debugging something), and he probably knows what the original setting was.

[stoecker]

Replying to taylor.smock:

Replying to skyper:

I am not sure if the last changes are to blame but the i18n test seem to be run constantly (every 10-15 minutes), now.

It doesn't look like it. Somehow the i18n-launcher job is set to run every 15 minutes. I didn't change it (maybe stoecker did?). I'll give him an opportunity to respond (maybe he is debugging something), and he probably knows what the original setting was.

Nope. Didn't change anything, but I updated Jenkins and plugins due to security fixes. Maybe that causes different behaviour.

[GerdP]

Maybe something happened between Oct. 19 and Oct. 22? The number of issues reported on Sonarqube mor than doubled around that time. I think it runs with defaults now instead of JOSM specific settings?

[stoecker]

Replying to GerdP:

Maybe something happened between Oct. 19 and Oct. 22? The number of issues reported on Sonarqube mor than doubled around that time. I think it runs with defaults now instead of JOSM specific settings?

That was a major update and cleanup of Sonar ;-) A lot of new checks appeared.

[GerdP]

I am pretty sure that e.g. "Refactor this method to reduce its Cognitive Complexity from 49 to the 15 allowed" isn't a new thing. I've seen this years before in my mkgmap project but not in JOSM. My understandig was that the limit was increased for JOSM.

[stoecker]

Replying to GerdP:

I am pretty sure that e.g. "Refactor this method to reduce its Cognitive Complexity from 49 to the 15 allowed" isn't a new thing. I've seen this years before in my mkgmap project but not in JOSM. My understandig was that the limit was increased for JOSM.

Yes. There were so many changes necessary, that I basically dropped the JOSM config. If adaptions are useful, please tell me and I'll change it.

[GerdP]

Well, that's a different thing. I always wondered why JOSM has its own settings.
I don't mind to use them also for JOSM, but maybe the test that produces strings like Define a constant instead of duplicating this literal "active" 3 times. should be reviewed. I think it was disabled before because the i18n strings often require the duplication.

[taylor.smock]

I think it was disabled before because the i18n strings often require the duplication

Maybe that was true in the past; now we just have to wrap the first definition in marktr. Example:

private static final String DESCRIPTION = marktr("This is a description");
private static final String DESCRIPTION_TRANSLATED = tr(DESCRIPTION);

As far as having our own settings go, I'd much rather keep the default settings and clean up the issues that can easily be corrected as I touch files (example: defining a constant instead of duplicating the literal is an "easy" one to fix, and can easily be proven to be correct, unlike reducing cognitive complexity).

[GerdP]

@taylor: Please have a look at https://josm.openstreetmap.de/jenkins/job/JOSM/8209/
Three unit tests fail only with JDK17. Could this also be an error in the junit setup?

[taylor.smock]

Nope; that would be due to a test (no clue which one) not cleaning up after itself. The only "real" way to debug it is to (a) hope it wasn't due to a race or ordering condition and (b) set up an AfterAllCallback that checks to ensure that the listeners are cleaned up. I've got that setup locally, and it wasn't failing with Java 21. I'll do a run with Java 17 and see what happens.

[taylor.smock]

My test runs with Java 17 did not reproduce it locally, and the code that was supposed to catch it didn't find anything.

But I have a lot of uncommitted changes locally. I'll have to run the tests in a clean directory to see if anything comes up. Beyond the standard "you are running on macOS" test failures.

[GerdP]

My understanding of the traceback in the failed tests is that under special conditions ImageViewerDialog.destroy() may be called twice.

[taylor.smock]

The problem is figuring out what the special conditions are. Especially since the tests don't stay broken.

[stoecker]

Replying to taylor.smock:

The problem is figuring out what the special conditions are. Especially since the tests don't stay broken.

After some time this type of error is the mainly remaining one in any mature software ;-)

P.S. You know that Jenkins keeps the whole build files and you should have access to them (for the few cases where that helps :-)?

[taylor.smock]

Replying to stoecker:

After some time this type of error is the mainly remaining one in any mature software ;-)

I'm working concurrently on flaky tests for the HOT Tasking Manager, and it has been frustrating (it doesn't help that it seems like the failing tests may be caused due to available system resources). I'd rather fix flaky tests now than have them become a problem later. Especially since I think the tests are flaky due to bad test isolation.

P.S. You know that Jenkins keeps the whole build files and you should have access to them (for the few cases where that helps :-)?

I don't think that would help in this case; I'm fairly confident that the order of the tests is important, and JUnit deliberately doesn't make the order obvious (and doesn't make it easy to run the tests in a specific order anyway). So I'm tried a run of the failing versions locally without modifications, and that just passed. So now I need to try a run in a VM, although I'm now leaning towards rerunning the test next time to see if it is consistent (and something I can debug easily).

[GerdP]

What's going wrong when this teardown code is missing?

    @AfterEach
    void tearDown() {
        if (ImageViewerDialog.hasInstance()) {
            ImageViewerDialog.getInstance().destroy();
        }
    }

BTW: When I try to run unit test ImageMarkerTest in Eclipse with clean r18966 it always fails:

java.lang.NullPointerException
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.getDefaultDetachedSize(ToggleDialog.java:910)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog$DetachedDialog.<init>(ToggleDialog.java:748)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.detach(ToggleDialog.java:427)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.showDialog(ToggleDialog.java:346)
	at org.openstreetmap.josm.gui.dialogs.ToggleDialog.unfurlDialog(ToggleDialog.java:371)
	at org.openstreetmap.josm.gui.layer.geoimage.ImageViewerDialog.displayImages(ImageViewerDialog.java:937)
	at org.openstreetmap.josm.gui.layer.markerlayer.ImageMarker.actionPerformed(ImageMarker.java:47)
	at org.openstreetmap.josm.gui.layer.markerlayer.ImageMarkerTest.testImageMarker(ImageMarkerTest.java:44)

The console log shows

2024-01-31 09:16:35.325 INFO: GET https://josm.openstreetmap.de/wiki/StartupPage -> HTTP/1.1 200 (515 ms)
System property josm.test.data is not set, using 'test/data'
System property josm.test.data is not set, using 'test/data'
2024-01-31 09:16:36.076 INFO: Loading file:/C:/josm/core/test/data/regress/12255/G0016941.JPG
2024-01-31 09:16:36.135 INFO: Attempting to close 2 windows left open by tests: [javax.swing.SwingUtilities$SharedOwnerFrame[frame0,0,0,0x0,invalid,hidden,layout=java.awt.BorderLayout,title=,resizable,normal], org.openstreetmap.josm.gui.dialogs.ToggleDialog$DetachedDialog[dialog0,0,0,0x0,invalid,hidden,layout=java.awt.BorderLayout,MODELESS,title=,defaultCloseOperation=HIDE_ON_CLOSE,rootPane=javax.swing.JRootPane[,0,0,0x0,invalid,layout=javax.swing.JRootPane$RootLayout,alignmentX=0.0,alignmentY=0.0,border=,flags=16777673,maximumSize=,minimumSize=,preferredSize=],rootPaneCheckingEnabled=true]]
2024-01-31 09:16:36.262 SCHWERWIEGEND: java.io.IOException: closed
java.io.IOException: closed
	at javax.imageio.stream.ImageInputStreamImpl.checkClosed(ImageInputStreamImpl.java:110)
	at javax.imageio.stream.ImageInputStreamImpl.close(ImageInputStreamImpl.java:857)
	at javax.imageio.stream.FileCacheImageInputStream.close(FileCacheImageInputStream.java:250)
	at org.openstreetmap.josm.tools.ImageProvider.read(ImageProvider.java:1740)
	at org.openstreetmap.josm.tools.ImageProvider.read(ImageProvider.java:1665)
	at org.openstreetmap.josm.data.imagery.street_level.IImageEntry.read(IImageEntry.java:153)
	at org.openstreetmap.josm.gui.layer.geoimage.ImageDisplay$LoadImageRunnable.run(ImageDisplay.java:286)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

[taylor.smock]

Replying to GerdP:

What's going wrong when this teardown code is missing?

That is what I found last time the tests were randomly failing.

BTW: When I try to run unit test ImageMarkerTest in Eclipse with clean r18966 it always fails:

It looks like this is due to not running the tests with -Djava.awt.headless=true -- there is a headless check prior to that call. We ought to make the tests less dependent upon whether or not they are run in a headless environment, but that is what the CI server is, so that is where I've focused on making the tests work.

We could try working around it by getting the mapview initialized (example: MainApplication.getLayerManager().addLayer(new GpxLayer(new GpxData())) -- this requires @Projection which can be done on the method level).

[GerdP]

As these test failed again: I think I saw the image dialog pop up recently when I saved my session file. I was not yet able to reproduce that but I think it might be the trigger for these random failures.

[GerdP]

No idea if this is related to the failing unit tests but I think I should share the information:
I set a breakpoint in Eclipse and loaded a session file with several image layers. Sometimes when I do this an empty popup for the image viewer is openend, sometimes not. The is the traceback when it happens (taken in Eclipse)

Thread [AWT-EventQueue-0] (Suspended (breakpoint at line 190 in ImageViewerDialog))	
	owns: Class<T> (org.openstreetmap.josm.gui.layer.geoimage.ImageViewerDialog) (id=72)	
	owns: Component$AWTTreeLock  (id=197)	
	ImageViewerDialog.showNotify() line: 190	
	DialogsPanel.add(ToggleDialog, boolean) line: 104	
	DialogsPanel.add(ToggleDialog) line: 75	
	MapFrame.addToggleDialog(ToggleDialog, boolean) line: 417	
	MapFrame.addToggleDialog(ToggleDialog) line: 398	
	ImageViewerDialog.getInstance() line: 128	
	ImageViewerDialog.getCurrentImage() line: 1082	
	GeoImageLayer.paint(Graphics2D, MapView, Bounds) line: 520	
	GeoImageLayer$2(AbstractMapViewPaintable$CompatibilityModeLayerPainter).paint(MapViewGraphics) line: 27	
	MapView.paintLayer(Layer, Graphics2D) line: 475	
	MapView.drawMapContent(Graphics2D) line: 590	
	MapView.paint(Graphics) line: 497	
	JPanel(JComponent).paintChildren(Graphics) line: 952	
	JPanel(JComponent).paint(Graphics) line: 1128	
	JSplitPane(JComponent).paintChildren(Graphics) line: 952	
	JSplitPane.paintChildren(Graphics) line: 1030	
	JSplitPane(JComponent).paint(Graphics) line: 1128	
	MapFrame(JComponent).paintChildren(Graphics) line: 952	
	MapFrame(JComponent).paint(Graphics) line: 1128	
	MainPanel(JComponent).paintChildren(Graphics) line: 952	
	MainPanel(JComponent).paint(Graphics) line: 1128	
	JPanel(JComponent).paintChildren(Graphics) line: 952	
	JPanel(JComponent).paint(Graphics) line: 1128	
	JPanel(JComponent).paintToOffscreen(Graphics, int, int, int, int, int, int) line: 5311	
	RepaintManager$PaintManager.paintDoubleBufferedImpl(JComponent, Image, Graphics, int, int, int, int) line: 1657	
	RepaintManager$PaintManager.paintDoubleBuffered(JComponent, Image, Graphics, int, int, int, int) line: 1632	
	RepaintManager$PaintManager.paint(JComponent, JComponent, Graphics, int, int, int, int) line: 1570	
	CheckThreadViolationRepaintManager(RepaintManager).paint(JComponent, JComponent, Graphics, int, int, int, int) line: 1337	
	JPanel(JComponent)._paintImmediately(int, int, int, int) line: 5259	
	JPanel(JComponent).paintImmediately(int, int, int, int) line: 5069	
	RepaintManager$4.run() line: 879	
	RepaintManager$4.run() line: 862	
	AccessController.executePrivileged(PrivilegedAction<T>, AccessControlContext, Class<?>) line: 776	
	AccessController.doPrivileged(PrivilegedAction<T>, AccessControlContext) line: 399	
	ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(PrivilegedAction<T>, AccessControlContext, AccessControlContext) line: 86	
	CheckThreadViolationRepaintManager(RepaintManager).paintDirtyRegions(Map<Component,Rectangle>) line: 862	
	CheckThreadViolationRepaintManager(RepaintManager).paintDirtyRegions() line: 835	
	CheckThreadViolationRepaintManager(RepaintManager).prePaintDirtyRegions() line: 784	
	RepaintManager$ProcessingRunnable.run() line: 1898	
	InvocationEvent.dispatch() line: 318	
	EventQueue.dispatchEventImpl(AWTEvent, Object) line: 771	
	EventQueue$4.run() line: 722	
	EventQueue$4.run() line: 716	
	AccessController.executePrivileged(PrivilegedAction<T>, AccessControlContext, Class<?>) line: 776	
	AccessController.doPrivileged(PrivilegedAction<T>, AccessControlContext) line: 399	
	ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(PrivilegedAction<T>, AccessControlContext, AccessControlContext) line: 86	
	EventQueue.dispatchEvent(AWTEvent) line: 741	
	EventDispatchThread.pumpOneEventForFilters(int) line: 203	
	EventDispatchThread.pumpEventsForFilter(int, Conditional, EventFilter) line: 124	
	EventDispatchThread.pumpEventsForHierarchy(int, Conditional, Component) line: 113	
	EventDispatchThread.pumpEvents(int, Conditional) line: 109	
	EventDispatchThread.pumpEvents(Conditional) line: 101	
	EventDispatchThread.run() line: 90