Modify

Opened 2 years ago

Last modified 4 days ago

#16567 reopened enhancement

[PATCH] Upgrade to JUnit 5

Reported by: Don-vip Owned by: team
Priority: major Milestone: 20.07
Component: Unit tests Version:
Keywords: junit Cc:

Description

JUnit 5 is now supported by all major IDEs and quite stable. JUnit 4 has not seen any release in the last 4 years, so it's time to upgrade.

As a first step I'll try to run our tests with the "vintage" engine (JUnit 5 running in JUnit 4 compatibility mode), before upgrading the tests.

https://ant.apache.org/manual/Tasks/junitlauncher.html

Attachments (20)

16567.patch (2.4 KB) - added by taylor.smock 8 months ago.
Allow JUnit 5 tests to use JOSMTestRules
16567.1.patch (10.6 KB) - added by taylor.smock 8 months ago.
Modify build.xml to use junitlauncher instead of junit. (jacoco output not working, looks like it is a problem with the destfile syntax).
junit_jars.sh (1.2 KB) - added by taylor.smock 8 months ago.
Add script to get jar files (for easy downloading and testing)
16567.4.patch (10.6 KB) - added by taylor.smock 7 months ago.
Output jacoco.exec files (if:set needed to be fixed). jacoco.exec files appear to have information (secondary testing would be nice, since my version of eclipse is refusing to load them). The reports appear to work properly, but the jacoco.exec file is not loading in Eclipse (this hasn't changed from the previous version, AFAICT). Since the jacoco.exec files are used in report generation, the data is there.
16567.5.patch (10.7 KB) - added by taylor.smock 7 months ago.
Fix an issue where non-unit tests (i.e., functional tests) didn't have JOSMTestRules in their classpath.
junit_difference.py (1.3 KB) - added by taylor.smock 7 months ago.
Show tests that have different numbers of passed/failed tests
16567.6.patch (10.7 KB) - added by taylor.smock 6 months ago.
Update to ensure patch applies cleanly (see r15703)
16567.9.patch (13.6 KB) - added by taylor.smock 2 months ago.
Update for Ivy dependencies
16567.11.patch (12.9 KB) - added by taylor.smock 4 weeks ago.
Fix ant test-perf (failing since the jacoco agent was added, even when coverage was disabled)
assertThat-warning.txt (35.3 KB) - added by simon04 3 weeks ago.
This patch causes plenty of deprecation warnings…
16567.12.patch (12.7 KB) - added by taylor.smock 3 weeks ago.
Remove unnecessary modification from JOSMTestRules
16567.deprecated.1.patch (52.1 KB) - added by taylor.smock 3 weeks ago.
Fix (most) deprecation warnings. There is still one in ExpectedRootException and OptionParserTest.
16567.deprecated.2.patch (58.6 KB) - added by taylor.smock 3 weeks ago.
ExpectedRootException has been deprecated, and LayerManagerTest has been modified to account for that
16567.deprecated.3.patch (59.0 KB) - added by taylor.smock 3 weeks ago.
All deprecation warnings are fixed, but there is a new failing test. The failing test is OptionParserTest#testMultipleShort. The reason it is failing is that the first parser.parseOpts was throwing an exception. This ensured that the rest of the test was never used.
16567.deprecated.4.patch (56.9 KB) - added by taylor.smock 3 weeks ago.
Update for testMultipleShort changes
16567.fixup1.patch (1.5 KB) - added by taylor.smock 3 weeks ago.
Ensure that after is not run until after the tests complete
16567.fixup1.1.patch (2.3 KB) - added by taylor.smock 3 weeks ago.
Add -Djunit.jupiter.extensions.autodetection.enabled=true as a JVM argument. This automatically registers the JMockit JUnit5 extension, which clears mocks between tests (otherwise, you can have a ton of mocks for the same code)
16567.imageryit.patch (8.3 KB) - added by taylor.smock 2 weeks ago.
Implement AfterAllCallback and BeforeAllCallback for instances where there is a static JOSMTestRules, modify ImageryPreferenceTestIT to be a full JUnit5 test, add junit-jupiter-params as an Ivy dependency
16567.domainvalidatortestit.patch (3.2 KB) - added by taylor.smock 7 days ago.
Use Logging.error + Logging.getLastErrorAndWarnings() to show output when JUnit 5 doesn't show system errors. Also use JUnit 5 Assertions instead of JUnit 4 Assert.
16567.domainvalidatortestit.2.patch (6.5 KB) - added by taylor.smock 7 days ago.

Download all attachments as: .zip

Change History (77)

comment:1 Changed 2 years ago by Don-vip

Milestone: 18.08

comment:2 Changed 2 years ago by Don-vip

Changed 8 months ago by taylor.smock

Attachment: 16567.patch added

Allow JUnit 5 tests to use JOSMTestRules

comment:3 Changed 8 months ago by taylor.smock

I'd like to be able to start using JUnit 5 in plugin tests. The above patch should not have any affect with current tests (since those methods are never called by JUnit 4).

Example usage:

    @RegisterExtension
    public JOSMTestRules test = new JOSMTestRules().projection();

EDIT:
JUnit 5 jars will need to be downloaded to test/lib/junit for ant test

$ curl -O "https://search.maven.org/remotecontent?filepath=org/junit/jupiter/junit-jupiter/5.5.2/junit-jupiter-5.5.2.jar"
$ curl -O "https://search.maven.org/remotecontent?filepath=org/junit/jupiter/junit-jupiter-api/5.5.2/junit-jupiter-api-5.5.2.jar"
Last edited 8 months ago by taylor.smock (previous) (diff)

comment:4 in reply to:  2 Changed 8 months ago by taylor.smock

Replying to Don-vip:

We need this in Ant: https://github.com/apache/ant/pull/67

https://ant.apache.org/manual/Tasks/junitlauncher.html#fork -- Since Ant 1.10.6 (May 7, 2019)

Replying to Don-vip:

First problem: ​https://github.com/jacoco/jacoco/issues/673

Apparently not going to be fixed (they really want to keep Java 5 compatibility):

Ant 1.10.x depends on Java 8, while the baseline for JaCoCo is Java 5. So currently we cannot add implementation and tests for it without major hacks in our build. To manage expectations I close this with WONTFIX.

It does look like there is a workaround though (see https://github.com/jacoco/jacoco/issues/673#issuecomment-499738017 )

Changed 8 months ago by taylor.smock

Attachment: 16567.1.patch added

Modify build.xml to use junitlauncher instead of junit. (jacoco output not working, looks like it is a problem with the destfile syntax).

comment:5 Changed 8 months ago by taylor.smock

Script to get the required libraries for junitlauncher:

#!/usr/bin/env sh

libraries=(
        # Common
        'https://search.maven.org/remotecontent?filepath=org/junit/platform/junit-platform-commons/1.5.2/junit-platform-commons-1.5.2.jar'
        'https://search.maven.org/remotecontent?filepath=org/junit/platform/junit-platform-engine/1.5.2/junit-platform-engine-1.5.2.jar'
        'https://search.maven.org/remotecontent?filepath=org/junit/platform/junit-platform-launcher/1.5.2/junit-platform-launcher-1.5.2.jar'
        'https://search.maven.org/remotecontent?filepath=org/opentest4j/opentest4j/1.2.0/opentest4j-1.2.0.jar'
        # JUnit 5
        'https://search.maven.org/remotecontent?filepath=org/junit/jupiter/junit-jupiter-api/5.5.2/junit-jupiter-api-5.5.2.jar'
        'https://search.maven.org/remotecontent?filepath=org/junit/jupiter/junit-jupiter-engine/5.5.2/junit-jupiter-engine-5.5.2.jar'
        # Junit 4 (also needs junit.jar from junit4, already there)
        'https://search.maven.org/remotecontent?filepath=org/junit/vintage/junit-vintage-engine/5.5.2/junit-vintage-engine-5.5.2.jar'
        )

for lib in ${libraries[@]}; do curl -O "${lib}"; done

comment:6 Changed 8 months ago by taylor.smock

Summary: Upgrade to JUnit 5[WIP PATCH] Upgrade to JUnit 5

When I said "(jacoco output not working, looks like it is a problem with the destfile syntax)", I was wrong -- I was running the tests under Java 13, where jacoco is explicitly disabled.

That being said, is the jacoco.exec supposed to have coverage information? It doesn't look like it is actually working, with the original build.xml (both the original and modified build.xml give me a coverage of two instructions in all of JOSM with jacoco.exec, and neither produce any other jacoco*.exec files).

comment:7 Changed 8 months ago by Don-vip

Yes, jacoco coverage works with Java 8: https://josm.openstreetmap.de/sonar/dashboard?id=josm

comment:8 Changed 8 months ago by taylor.smock

I haven't been able to get jacoco coverage working properly in a fresh VM (Fedora 31 with openjdk 1.8.0) with a fresh copy of JOSM (from svn)

What OS/OS version is the test server running?

I've been using ant test-unit -Dosm.username=<+TEST_USER+> -Dosm.password=<+TEST_PASSWORD+> (with a real user/password on the osm sandbox server).

If that isn't the command that leads to the appropriate junit coverage output, please let me know (based off of <echo> statements, testITsuffix is never set, which means that the jacoco.exec file is overwritten by unit/functional/performance tests, but none of them should have 2 lines of coverage).

To test the coverage, I've been loading it into Eclipse, using the JOSM/src as the target directory (I didn't see anything that indicated that it should be a lower directory, e.g. src/org/openstreetmap/josm/)

Changed 8 months ago by taylor.smock

Attachment: junit_jars.sh added

Add script to get jar files (for easy downloading and testing)

comment:9 in reply to:  8 Changed 8 months ago by taylor.smock

Replying to taylor.smock:

I haven't been able to get jacoco coverage working properly in a fresh VM (Fedora 31 with openjdk 1.8.0) with a fresh copy of JOSM (from svn)

I ran ant test-html -Dosm.username=<+TEST_USER+> -Dosm.password=<+TEST_PASSWORD+> and I got coverage information (using the html instead of jacoco.exec). I also just took a look at the docs for SonarQube coverage ( https://docs.sonarqube.org/latest/analysis/coverage/ ), and it looks like sonar.coverage.jacoco.xmlReportPaths is the supported way to get coverage information of java instead of parsing the jacoco.exec file.

It looks like jacoco.exec and jacocoIT.exec are generated on Debian with the original build.xml.

comment:10 Changed 8 months ago by Don-vip

I'm pretty sure the jacoco.exec files work as expected.

The SonarQube project configuration is as follows:

Key: sonar.jacoco.reportPaths:
- target/jacoco.exec
- target/jacoco-it.exec

Server is running current Ubuntu LTS.

JOSM Jenkins job is:

ant clean test-clean test-html dist-optimized-report distmac distwin spotbugs checkstyle pmd javadoc taginfo

with JDK8 (from Ubuntu: /usr/lib/jvm/java-8-openjdk-amd64), Ant 1.10.5 (downloaded; I just upgraded manually to Ant 1.10.7) and following properties:

test-it.notRequired=true
test-perf.notRequired=true
osm.username=xxx
osm.password=yyy
glass.platform=Monocle
monocle.platform=Headless
prism.order=sw

Jenkins jacoco configuration is:

path to exec: **/**.exec
inclusions: org/openstreetmap/josm/**/*.class
path to class: build
path to source: src

Then the Jenkins Sonar job runs SonarQube Scanner 3.2.0.1227 (downloaded; I just upgraded manually to 4.2.0.1873) with the same JDK8 and following properties:

# required metadata
sonar.projectKey=josm
sonar.projectName=JOSM
sonar.projectVersion=$SVN_REVISION

# path to source directories (required)
sonar.sources=src,data,images,resources,styles,linux,windows,macosx,scripts
sonar.sourceEncoding=UTF-8
sonar.java.source=1.8

# path to test source directories (optional)
sonar.tests=test/functional,test/performance,test/unit

# path to project binaries (optional), for example directory of Java bytecode
sonar.java.binaries=build
sonar.java.test.binaries=build,test/build/unit,test/build/functional,test/build/performance
sonar.java.test.libraries=test/lib/**/*.jar

sonar.scm.url=scm:svn:http://josm.openstreetmap.de/svn/trunk

# Exclude all COTS and classes in the 'org/openstreetmap/josm/gui/mappaint/mapcss/parsergen' package
sonar.inclusions=src/org/openstreetmap/josm/**/*.java,**/*.groovy,**/*.xml,**/*.xsd,**/*.css,**/*.osm,**/*.mapcss,**/*.properties,**/*.json,**/*.js,**/*.cfg
sonar.exclusions=src/org/openstreetmap/josm/gui/mappaint/mapcss/parsergen/**/*.java,src/org/openstreetmap/josm/data/imagery/types/*.java,data/overpass-turbo-ffs.js,data/overpass-wizard.js,data/validator/opening_hours.js,macosx/JOSM.app/Contents/Info.plist_template.xml

# Plugins
sonar.findbugs.timeout=1200000
sonar.jacoco.reportPath=test/jacoco.exec
sonar.jacoco.itReportPath=test/jacocoIT.exec
sonar.junit.reportsPath=test/report
sonar.scm-stats.period1=365
sonar.scm-stats.period2=90
sonar.scm-stats.period3=7
sonar.trac.url=https://josm.openstreetmap.de
sonar.trac.username.secured=xxx
sonar.trac.password.secured=yyy

This setup is working fine for years.

comment:11 Changed 8 months ago by taylor.smock

It seems like something would be broken:

sonar.jacoco.itReportPath=test/jacocoIT.exec
target/jacoco-it.exec

Anyway, thank you for the information. I'll set up a VM for testing with Ubuntu 18.04 LTS, and once I can reproduce the jacoco{,IT}.exec files, I'll apply the patch and see if it works.

If I don't figure out what is going on with the junitlauncher + jacoco:agent ant configuration before 19.12, is there any chance of merging the first patch that added the JUnit 5 compatibility for plugin tests?

comment:12 in reply to:  11 ; Changed 8 months ago by Don-vip

Replying to taylor.smock:

If I don't figure out what is going on with the junitlauncher + jacoco:agent ant configuration before 19.12, is there any chance of merging the first patch that added the JUnit 5 compatibility for plugin tests?

Sure, I just want to test it first but I have very little time this week.

comment:13 in reply to:  12 Changed 8 months ago by taylor.smock

Replying to Don-vip:

Sure, I just want to test it first but I have very little time this week.

That's fair.

Have a good (and hopefully enjoyable) week.

Changed 7 months ago by taylor.smock

Attachment: 16567.4.patch added

Output jacoco.exec files (if:set needed to be fixed). jacoco.exec files appear to have information (secondary testing would be nice, since my version of eclipse is refusing to load them). The reports appear to work properly, but the jacoco.exec file is not loading in Eclipse (this hasn't changed from the previous version, AFAICT). Since the jacoco.exec files are used in report generation, the data is there.

comment:14 Changed 7 months ago by taylor.smock

@Don-vip: I would appreciate a second opinion on the jacoco.exec files -- the data appears to be there, but I haven't been able to get Eclipse to load it (as a sanity check). The report generation works using the same files, and the reports look OK.

comment:15 Changed 7 months ago by taylor.smock

OK. I just ran a comparison between the report from junit 4 and junit 5, and I got the following differences (see the junit_difference.py file, junit4/junit5 reports have to be copied to specific directories):

TEST-org.openstreetmap.josm.gui.mappaint.mapcss.ChildOrParentSelectorTest.xml
report-junit4: {'tests': '7', 'skipped': '4'}
report-junit5: {'tests': '3', 'skipped': '4'}
TEST-org.openstreetmap.josm.actions.JoinAreasActionTest.xml
report-junit4: {'tests': '3', 'skipped': '1'}
report-junit5: {'tests': '2', 'skipped': '1'}
TEST-org.openstreetmap.josm.tools.PlatformHookOsxTest.xml
report-junit4: {'tests': '8', 'skipped': '1'}
report-junit5: {'tests': '8', 'aborted': '1'}
TEST-org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.xml
report-junit4: {'tests': '17', 'skipped': '1'}
report-junit5: {'tests': '16', 'skipped': '1'}
TEST-org.openstreetmap.josm.gui.dialogs.relation.actions.AbstractRelationEditorActionTest.xml
report-junit4: {'tests': '1', 'skipped': '1'}
report-junit5: {}
TEST-org.openstreetmap.josm.gui.layer.gpx.ChooseTrackVisibilityActionTest.xml
report-junit4: {'tests': '1', 'skipped': '1'}
report-junit5: {'skipped': '1'}
TEST-org.openstreetmap.josm.io.MultiFetchServerObjectReaderTest.xml
report-junit4: {'tests': '5', 'skipped': '5'}
report-junit5: {'tests': '5', 'aborted': '5'}
TEST-org.openstreetmap.josm.io.OsmServerBackreferenceReaderTest.xml
report-junit4: {'tests': '6', 'skipped': '6'}
report-junit5: {'tests': '6', 'aborted': '6'}
TEST-org.openstreetmap.josm.tools.OverpassTurboQueryWizardTest.xml
report-junit4: {'tests': '11', 'skipped': '1'}
report-junit5: {'tests': '10', 'skipped': '1'}
TEST-org.openstreetmap.josm.io.audio.AudioPlayerTest.xml
report-junit4: {'tests': '1', 'skipped': '1'}
report-junit5: {'skipped': '1'}
TEST-org.openstreetmap.josm.io.UploadStrategySelectionPanelTest.xml
report-junit4: {'tests': '1', 'skipped': '1'}
report-junit5: {'skipped': '1'}
TEST-org.openstreetmap.josm.data.validation.routines.EmailValidatorTest.xml
report-junit4: {'tests': '22', 'skipped': '1'}
report-junit5: {'tests': '21', 'skipped': '1'}

Beyond that, it looks like JUnit5 removes the "skipped" tests from the "tests" count, so there appears to be no real difference as far as the test outputs go.

That being said, the HTML outputs do have differences, and I'm not certain why probably due to differences in how they handle skipped tests.

JUnit Tests Failures Errors Skipped Success rate Time
4 2168 0 0 24 100.00% 1392.075
5 2156 0 0 11 100.00% 1378.424

So JUnit5 appears to have 2167 tests while JUnit4 has 2168 tests. The single difference is likely from org.openstreetmap.josm.gui.dialogs.relation.actions.AbstractRelationEditorActionTest, which only has skipped tests in JUnit4, which means JUnit5 is not counting them.

Last edited 7 months ago by taylor.smock (previous) (diff)

Changed 7 months ago by taylor.smock

Attachment: 16567.5.patch added

Fix an issue where non-unit tests (i.e., functional tests) didn't have JOSMTestRules in their classpath.

Changed 7 months ago by taylor.smock

Attachment: junit_difference.py added

Show tests that have different numbers of passed/failed tests

comment:16 Changed 7 months ago by taylor.smock

Summary: [WIP PATCH] Upgrade to JUnit 5[PATCH] Upgrade to JUnit 5

comment:17 Changed 7 months ago by Don-vip

Milestone: 19.12
Owner: changed from team to Don-vip
Priority: normalmajor
Status: newassigned

comment:18 Changed 6 months ago by Don-vip

Jacoco *.exec file support has just been removed from SonarQube: https://jira.sonarsource.com/browse/MMF-1651
I will probably need to change something to load XML reports instead.

comment:19 Changed 6 months ago by Don-vip

Milestone: 19.1220.01

comment:20 Changed 6 months ago by Don-vip

In 15703/josm:

see #16567 - generate Jacoco CSV and XML reports

Changed 6 months ago by taylor.smock

Attachment: 16567.6.patch added

Update to ensure patch applies cleanly (see r15703)

comment:21 Changed 5 months ago by Don-vip

Milestone: 20.0120.02

comment:22 Changed 5 months ago by taylor.smock

Is there anything I need to do?

comment:23 in reply to:  22 Changed 5 months ago by Don-vip

Replying to taylor.smock:

Is there anything I need to do?

No? I still need to find enough time to complete my testing and commit the changes. Sorry, I'm a bit overloaded right now.

comment:24 Changed 5 months ago by taylor.smock

Fair enough. I figured I would double check -- I haven't seen any issues in my own testing, but different systems are different, and I'm usually testing on a heavily customized JOSM codebase. Its worked for me(tm) on a clean codebase (with just the patch applied), but sometimes software works differently on different systems.

And I completely understand waiting to find a solid chunk of time for testing this (it is kind of important for the test suite to keep working).

comment:25 Changed 4 months ago by Don-vip

Milestone: 20.0220.03

comment:26 Changed 4 months ago by Don-vip

Milestone: 20.0320.04

Descoping to release 20.03 early to address #18798, as we receive a lot of duplicates.

Changed 2 months ago by taylor.smock

Attachment: 16567.9.patch added

Update for Ivy dependencies

comment:27 Changed 2 months ago by Klumbumbus

Milestone: 20.0420.05

Milestone renamed

comment:28 Changed 7 weeks ago by simon04

Milestone: 20.05Longterm

comment:29 Changed 5 weeks ago by taylor.smock

I've been running svn up; cd core; ant clean dist test for the past month or two on weekdays (in my morning). The only consistent test failure I've seen with this patch (on Mac) is this:

ShortcutTest    testMakeTooltip Failure expected:<[<html>Foo Bar <font size='-2'>(Shift+J)</font>&nbsp;</html>]> but was:<[Foo Bar (⇧+J)]>

And that isn't a true failure, just a case where Shift is replaced by .

EDIT: I was always checking the html output, which wasn't being regenerated (I needed to be calling ant test-html).

Last edited 4 weeks ago by taylor.smock (previous) (diff)

Changed 4 weeks ago by taylor.smock

Attachment: 16567.11.patch added

Fix ant test-perf (failing since the jacoco agent was added, even when coverage was disabled)

Changed 3 weeks ago by simon04

Attachment: assertThat-warning.txt added

This patch causes plenty of deprecation warnings…

comment:30 Changed 3 weeks ago by taylor.smock

The deprecation warnings are fairly new. At the time of the original patch, they did not exist.

My original plan was to do a in-situ upgrade to JUnit5, then start switching tests over to using JUnit5 (to remove most dependencies on JUnit4).

In light of the deprecation notices, I was going to still do the "simple" upgrade to JUnit5, but then fix deprecation warnings, and then start switching tests over to using JUnit5.

Note: I was intending on leaving the dependency on JUnit4 (JOSMTestRules) in for a little while, to give plugin authors some time to switch to JUnit5. This is to avoid a situation where multiple plugins have to upgrade all at the same time.

Changed 3 weeks ago by taylor.smock

Attachment: 16567.12.patch added

Remove unnecessary modification from JOSMTestRules

Changed 3 weeks ago by taylor.smock

Attachment: 16567.deprecated.1.patch added

Fix (most) deprecation warnings. There is still one in ExpectedRootException and OptionParserTest.

comment:31 Changed 3 weeks ago by taylor.smock

I've gone through and fixed most deprecation warnings.

The 16567.deprecated.1.patch does depend on the 16567.12.patch (where I added new assert methods, I used the JUnit5 API, not the JUnit4 API).

I'm going to have to do more work on ExpectedRootException -- currently only used in LayerManagerTest.java, and maybe some plugins. [EDIT: Not used in any plugins in the `plugins` directory]

I'm also going to have to do some work on OptionParserTest#testMultipleShort -- the current (in-tree) implementation stops at the first parser.parseOptions(Arrays.asList("-ft=arg", "x")), so I have to figure out how to actually run the remainder of the test (note: fairly easy to avoid stopping due to an expected exception, but the other assertions are failing).

Last edited 3 weeks ago by taylor.smock (previous) (diff)

Changed 3 weeks ago by taylor.smock

Attachment: 16567.deprecated.2.patch added

ExpectedRootException has been deprecated, and LayerManagerTest has been modified to account for that

Changed 3 weeks ago by taylor.smock

Attachment: 16567.deprecated.3.patch added

All deprecation warnings are fixed, but there is a new failing test. The failing test is OptionParserTest#testMultipleShort. The reason it is failing is that the first parser.parseOpts was throwing an exception. This ensured that the rest of the test was never used.

comment:32 Changed 3 weeks ago by taylor.smock

@simon04: Any advice on how to handle the failing test? I'm inclined to either remove everything after the point where the test initially was stopping, or rewriting the test to pass today using the current code to indicate expected results.

comment:33 Changed 3 weeks ago by Don-vip

Owner: changed from Don-vip to team
Status: assignednew

comment:34 Changed 3 weeks ago by simon04

In 16608/josm:

see #16567 - Fix OptionParserTest.testMultipleShort

Changed 3 weeks ago by taylor.smock

Attachment: 16567.deprecated.4.patch added

Update for testMultipleShort changes

comment:35 Changed 3 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 16617/josm:

fix #16567 - Upgrade to JUnit 5 (patch by taylor.smock)

I applied attachment:16567.12.patch

Last edited 3 weeks ago by simon04 (previous) (diff)

comment:36 Changed 3 weeks ago by simon04

In 16618/josm:

see #16567 - Fix deprecations related to JUnit 5 (patch by taylor.smock)

I applied attachment:16567.deprecated.4.patch

Last edited 3 weeks ago by simon04 (previous) (diff)

comment:37 Changed 3 weeks ago by simon04

Milestone: Longterm20.06

comment:38 Changed 3 weeks ago by simon04

Jenkins needs to be updated – https://josm.openstreetmap.de/jenkins/job/JOSM/6529/jdk=JDK8/testReport/org.openstreetmap.josm.gui.layer/TMSLayerTest/testTMSLayer/ only reports

java.lang.reflect.InvocationTargetException

when it should report

ReportedException [thread=Thread[Timeout runner,5,], exception=java.lang.reflect.InvocationTargetException
, methodWarningFrom=null]
	at org.openstreetmap.josm.tools.bugreport.BugReport.intercept(BugReport.java:173)
	at org.openstreetmap.josm.gui.util.GuiHelper.runInEDTAndWaitWithException(GuiHelper.java:245)
	at org.openstreetmap.josm.gui.layer.LayerManager.addLayer(LayerManager.java:217)
	at org.openstreetmap.josm.gui.layer.LayerManager.addLayer(LayerManager.java:206)
	at org.openstreetmap.josm.gui.layer.TMSLayerTest.test(TMSLayerTest.java:53)
	at org.openstreetmap.josm.gui.layer.TMSLayerTest.testTMSLayer(TMSLayerTest.java:66)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:756)
Caused by: java.lang.reflect.InvocationTargetException
	at java.desktop/java.awt.EventQueue.invokeAndWait(EventQueue.java:1367)
	at java.desktop/java.awt.EventQueue.invokeAndWait(EventQueue.java:1342)
	at java.desktop/javax.swing.SwingUtilities.invokeAndWait(SwingUtilities.java:1480)
	at org.openstreetmap.josm.gui.util.GuiHelper.runInEDTAndWaitWithException(GuiHelper.java:243)
	... 5 more
Caused by: java.lang.NullPointerException
	at org.openstreetmap.josm.gui.layer.imagery.TileCoordinateConverter.getScaleFactor(TileCoordinateConverter.java:186)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.getScaleFactor(AbstractTileSourceLayer.java:359)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.getBestZoom(AbstractTileSourceLayer.java:370)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.initTileSource(AbstractTileSourceLayer.java:281)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.initializeIfRequired(AbstractTileSourceLayer.java:579)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer.attachToMapView(AbstractTileSourceLayer.java:555)
	at org.openstreetmap.josm.gui.MapView.layerAdded(MapView.java:342)
	at org.openstreetmap.josm.gui.layer.LayerManager.fireLayerAdded(LayerManager.java:458)
	at org.openstreetmap.josm.gui.layer.LayerManager.realAddLayer(LayerManager.java:233)
	at org.openstreetmap.josm.gui.layer.MainLayerManager.realAddLayer(MainLayerManager.java:284)
	at org.openstreetmap.josm.gui.layer.LayerManager.lambda$addLayer$0(LayerManager.java:217)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:306)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:770)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:704)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:740)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

comment:39 Changed 3 weeks ago by simon04

Resolution: fixed
Status: closedreopened

comment:40 Changed 3 weeks ago by simon04

In 16633/josm:

see #16567 - Fix deprecations related to JUnit 5 (in HttpClientTest)

Changed 3 weeks ago by taylor.smock

Attachment: 16567.fixup1.patch added

Ensure that after is not run until after the tests complete

comment:41 Changed 3 weeks ago by taylor.smock

I just started porting over the MapWithAI unit tests over to JUnit5. In the process, I discovered that after was being called before the tests were actually run.

I'm currently tracking down a StackOverflow bug as well, but that might just be code that I'm using in the plugin.

Changed 3 weeks ago by taylor.smock

Attachment: 16567.fixup1.1.patch added

Add -Djunit.jupiter.extensions.autodetection.enabled=true as a JVM argument. This automatically registers the JMockit JUnit5 extension, which clears mocks between tests (otherwise, you can have a ton of mocks for the same code)

comment:42 Changed 3 weeks ago by simon04

In 16658/josm:

see #16567 - JUnit 5: call JOSMTestRules.after (patch by taylor.smock)

comment:43 Changed 3 weeks ago by simon04

In 16659/josm:

see #16567 - JUnit 5: set -Djunit.jupiter.extensions.autodetection.enabled=true to clear JMockit mocks (patch by taylor.smock)

comment:44 in reply to:  43 Changed 2 weeks ago by taylor.smock

Replying to simon04:

In 16659/josm:

see #16567 - JUnit 5: set -Djunit.jupiter.extensions.autodetection.enabled=true to clear JMockit mocks (patch by taylor.smock)

#18200 is going to be a blocker for moving completely to JUnit5. The -Djunit.jupiter.extensions.autodetection.enabled=true is only relevant for v1.46+ of JMockit, and we are currently using v1.44. The JMockit changelog claims that it Added support for JUnit 5.4.0. However, in my testing, I found that 1.47+ worked, while 1.46 did not actually remove the mockers when Djunit.jupiter.extensions.autodetection.enabled=true.

comment:46 Changed 2 weeks ago by simon04

Yes, very likely: Running …

$ ant -Dtest-perf.notRequired=true -Dtest.notRequired=true '-Ddefault-junitIT-includes=**/ImageryPreferenceTestIT.class' test-clean test-html

yields

$ cat test/report/TEST-org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT.txt
Testcase: org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT
Test: testImageryEntryValidity[cyclosm] took 1 sec(s)
Test: testImageryEntryValidity[osm-cambodia_laos_thailand_vietnam-bilingual] took 1 sec(s)
Test: testImageryEntryValidity[Bing] took 2 sec(s)
Tests run: 3, Failures: 0, Skipped: 0, Aborted: 0, Time elapsed: 4 sec(s)

but an empty

$ cat test/report/TEST-org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT.xml
<?xml version="1.0"?>
<testsuite name="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" time="4.466" timestamp="2020-06-21T08:14:17" tests="3" failures="0" skipped="0" aborted="0">
  <properties><!-- ... --></properties>
  <!-- here should be the test results!!! -->
</testsuite>

For testing, I restricted the imagery tests to 3:

  • test/unit/org/openstreetmap/josm/gui/preferences/imagery/ImageryPreferenceTestIT.java

    diff --git a/test/unit/org/openstreetmap/josm/gui/preferences/imagery/ImageryPreferenceTestIT.java b/test/unit/org/openstreetmap/josm/gui/preferences/imagery/ImageryPreferenceTestIT.java
    index 24fc65551..b1950c09b 100644
    a b public static void afterClass() { 
    128128        ImageryLayerInfo.instance.load(false);
    129129        return ImageryLayerInfo.instance.getDefaultLayers()
    130130                .stream()
     131                .limit(3)
    131132                .map(x -> new Object[] {x.getId(), x})
    132133                .collect(Collectors.toList());
    133134    }

The error might be related to org.apache.tools.ant.taskdefs.optional.junitlauncher.LegacyXmlResultFormatter not correctly reporting parameterized tests…
Now we're in the middle of JUnit 4 tests, JUnit 5 test runner, Ant test reporter :-(

Changed 2 weeks ago by taylor.smock

Attachment: 16567.imageryit.patch added

Implement AfterAllCallback and BeforeAllCallback for instances where there is a static JOSMTestRules, modify ImageryPreferenceTestIT to be a full JUnit5 test, add junit-jupiter-params as an Ivy dependency

comment:48 Changed 2 weeks ago by taylor.smock

There is some odd behavior when running the parameterized test in Eclipse. The test likes to be collapsed except when running another parameterized test, or one of the parameters has caused a failure. So for the first x tests, it is collapsing/expanding on every parameter.

In any case, I think I've fixed the issue, but I am waiting on the test to actually complete (I didn't limit it to 3 imagery layers). It will probably finish in 30-80 minutes (full test runs on my home machine takes ~86 minutes).

comment:49 in reply to:  48 Changed 2 weeks ago by taylor.smock

Replying to taylor.smock:

In any case, I think I've fixed the issue, but I am waiting on the test to actually complete (I didn't limit it to 3 imagery layers). It will probably finish in 30-80 minutes (full test runs on my home machine takes ~86 minutes).

OK. It took 90 minutes (I think I may need to look into @Timeout if we want to decrease test run time, but we already allow it to take 40 minutes), but there is data in the xml file, and there is data in the txt file. The xml looks good (note: I ran it through xmllint --format and edited for brevity)

<?xml version="1.0" ?><testsuite name="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" time="5397.281" timestamp="2020-06-21T12:41:33" tests="887" failures="57" skipped="0" aborted="29">
<.../>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[193]" time="0.533"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[700]" time="8.179"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[833]" time="3.198"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[506]" time="0.831"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[639]" time="0.971"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[748]" time="2.493"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[615]" time="0.631">
    <aborted message="Assumption failed: {IE={OSMIE Townlands[IE] ('Ireland) - https://tile.openstreetmap.ie/townland/{zoom}/{x}/{y}.png - TMS=[https://tile.openstreetmap.ie/townland/7/61/41.png -&gt; java.net.ConnectException: Connection refused (Connection refused), https://tile.openstreetmap.ie/townland/12/1956/1326.png -&gt; java.net.ConnectException: Connection refused (Connection refused)]}}" type="org.opentest4j.TestAbortedException"><![CDATA[org.opentest4j.TestAbortedException: Assumption failed: {IE={OSMIE Townlands[IE] ('Ireland) - https://tile.openstreetmap.ie/townland/{zoom}/{x}/{y}.png - TMS=[https://tile.openstreetmap.ie/townland/7/61/41.png -> java.net.ConnectException: Connection refused (Connection refused), https://tile.openstreetmap.ie/townland/12/1956/1326.png -> java.net.ConnectException: Connection refused (Connection refused)]}}
    ...stacktrace...
    </aborted>
  </testcase>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[91]" time="5.099"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[724]" time="8.458"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[857]" time="0.854"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[192]" time="1.305"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[723]" time="7.297"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[832]" time="0.947"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[505]" time="0.825"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[529]" time="0.82"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[638]" time="0.807"/>
  <testcase classname="org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT" name="testImageryEntryValidity(String, ImageryInfo)[747]" time="120.349">
    <failure message="{PL={Przemy&#x15B;l: Ortophotomap (aerial image)[PL] ('Poland) - http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&amp;VERSION=1.1.1&amp;SERVICE=WMS&amp;REQUEST=GetMap&amp;LAYERS=ortofotomapa&amp;STYLES=&amp;SRS={proj}&amp;WIDTH={width}&amp;HEIGHT={height}&amp;BBOX={bbox} - WMS=[http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&amp;VERSION=1.1.1&amp;SERVICE=WMS&amp;REQUEST=GetMap&amp;LAYERS=ortofotomapa&amp;STYLES=&amp;SRS=EPSG:2179&amp;WIDTH=512&amp;HEIGHT=512&amp;BBOX=8390252.4918224,-33995034.6879584,48465269.1774009,6079981.9976201 -&gt; java.net.SocketTimeoutException: connect timed out, http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&amp;VERSION=1.1.1&amp;SERVICE=WMS&amp;REQUEST=GetMap&amp;LAYERS=ortofotomapa&amp;STYLES=&amp;SRS=EPSG:2179&amp;WIDTH=512&amp;HEIGHT=512&amp;BBOX=8409820.3710634,5512513.4996309,8429388.2503044,5532081.3788719 -&gt; java.net.SocketTimeoutException: connect timed out]}} ==&gt; expected: &lt;true&gt; but was: &lt;false&gt;" type="org.opentest4j.AssertionFailedError"><![CDATA[org.opentest4j.AssertionFailedError: {PL={Przemy�~[l: Ortophotomap (aerial image)[PL] ('Poland) - http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=ortofotomapa&STYLES=&SRS={proj}&WIDTH={width}&HEIGHT={height}&BBOX={bbox} - WMS=[http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=ortofotomapa&STYLES=&SRS=EPSG:2179&WIDTH=512&HEIGHT=512&BBOX=8390252.4918224,-33995034.6879584,48465269.1774009,6079981.9976201 -> java.net.SocketTimeoutException: connect timed out, http://przemysl.geoportal2.pl/map/geoportal/wms.php?FORMAT=image/jpeg&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=ortofotomapa&STYLES=&SRS=EPSG:2179&WIDTH=512&HEIGHT=512&BBOX=8409820.3710634,5512513.4996309,8429388.2503044,5532081.3788719 -> java.net.SocketTimeoutException: connect timed out]}} ==> expected: <true> but was: <false>
... More output ...
Last edited 2 weeks ago by taylor.smock (previous) (diff)

comment:50 Changed 2 weeks ago by simon04

@taylor.smock, thanks for looking into this issue!

The test names are not yet correct – tests are reported as name="testImageryEntryValidity(String, ImageryInfo)[193]" when they used to be name="testImageryEntryValidity[Berlin-2018]", see https://josm.openstreetmap.de/jenkins/job/JOSM-Imagery-Integration/1699/testReport/

comment:51 Changed 2 weeks ago by taylor.smock

It turns out this is not supported (yet). Apparently the JUnit team has been planning to create a new test format that supports all of the new JUnit5 features. I would have expected parameterized tests to continue working as they had in JUnit4. I have a parameterized test in MapWithAI, and it works ok in Eclipse (like this one). I had never bothered looking at the xml output, so I didn't realize that it was (effectively) useless.

We could (theoretically) write our own test writer, but I really don't want to do that. Or extend the XML writer to write what we actually want it to write.

Links, so I don't forget them:

https://github.com/junit-team/junit5/issues/373
https://github.com/ota4j-team/opentest4j/issues/9
https://junit.org/junit5/docs/current/user-guide/#launcher-api-listeners-custom (if we want to roll our own...)

comment:52 Changed 7 days ago by simon04

Resolution: fixed
Status: reopenedclosed

In 16727/josm:

fix #16567 - Fix ImageryPreferenceTestIT for JUnit 5 (patch by taylor.smock)

comment:53 Changed 7 days ago by simon04

@taylor.smock, thank you. I'm a bit puzzled that there are so many pitfalls (considering that JUnit has been released in 2017-09)

I've submitted a bug in the Ant Bugzilla: Bug 64564 – JUnitLauncher Task should support @ParameterizedTest (custom display name)

comment:54 Changed 7 days ago by Klumbumbus

In the last time I already had the impression that the output of tests on jenkins is reduced. Now there is one test without any output at all, which is bad: https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/lastCompletedBuild/jdk=JDK8/testReport/org.openstreetmap.josm.data.validation.routines/DomainValidatorTestIT/testIanaTldList/

comment:55 in reply to:  54 Changed 7 days ago by taylor.smock

Replying to simon04:

@taylor.smock, thank you. I'm a bit puzzled that there are so many pitfalls (considering that JUnit has been released in ​2017-09)

I've submitted a bug in the Ant Bugzilla: Bug ​64564 – JUnitLauncher Task should support @ParameterizedTest (custom display name)

It has surprised me too. And not in a good way. I had been hoping that no major issues would crop up, but I was (a) not testing properly (i.e., not looking at the xml and txt files) and (b) assuming that JUnit 5 would have feature parity (at least!).

Replying to Klumbumbus:

In the last time I already had the impression that the output of tests on jenkins is reduced. Now there is one test without any output at all, which is bad: https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/lastCompletedBuild/jdk=JDK8/testReport/org.openstreetmap.josm.data.validation.routines/DomainValidatorTestIT/testIanaTldList/

I've got a fix for this specific issue. I'll post that here in a minute or two.

Changed 7 days ago by taylor.smock

Use Logging.error + Logging.getLastErrorAndWarnings() to show output when JUnit 5 doesn't show system errors. Also use JUnit 5 Assertions instead of JUnit 4 Assert.

Changed 7 days ago by taylor.smock

comment:56 Changed 4 days ago by Klumbumbus

Milestone: 20.0620.07
Resolution: fixed
Status: closedreopened

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to Don-vip
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.