Modify

Opened 22 months ago

Last modified 2 weeks ago

#16567 assigned enhancement

[PATCH] Upgrade to JUnit 5

Reported by: Don-vip Owned by: Don-vip
Priority: major Milestone: Longterm
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 (8)

16567.patch (2.4 KB) - added by taylor.smock 7 months ago.
Allow JUnit 5 tests to use JOSMTestRules
16567.1.patch (10.6 KB) - added by taylor.smock 7 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 7 months ago.
Add script to get jar files (for easy downloading and testing)
16567.4.patch (10.6 KB) - added by taylor.smock 6 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 6 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 6 months ago.
Show tests that have different numbers of passed/failed tests
16567.6.patch (10.7 KB) - added by taylor.smock 4 months ago.
Update to ensure patch applies cleanly (see r15703)
16567.9.patch (13.6 KB) - added by taylor.smock 6 weeks ago.
Update for Ivy dependencies

Download all attachments as: .zip

Change History (36)

comment:1 Changed 22 months ago by Don-vip

Milestone: 18.08

comment:2 Changed 22 months ago by Don-vip

Changed 7 months ago by taylor.smock

Attachment: 16567.patch added

Allow JUnit 5 tests to use JOSMTestRules

comment:3 Changed 7 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 7 months ago by taylor.smock (previous) (diff)

comment:4 in reply to:  2 Changed 7 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 7 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 7 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 7 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 7 months ago by Don-vip

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

comment:8 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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 6 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 6 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 6 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~.

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

Differences

TEST-org.openstreetmap.josm.gui.dialogs.relation.actions.AbstractRelationEditorActionTest.xml: ['org.openstreetmap.josm.gui.dialogs.relation.actions.AbstractRelationEditorActionTest']
TEST-org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.xml: ['testRender[way-repeat-image]', 'testRender[node-shapes-default]', 'testRender[eval]', 'testRender[node-shapes]', 'testRender[way-color]', 'testRender[way-dashes]', 'testRender[way-dashes-clamp]', 'testRender[relation-linkselector]', 'testRender[area-text]', 'testRender[way-width]', 'testRender[way-text]', 'testRender[area-fill-image]', 'testRender[area-fill-color]', 'testRender[way-repeat-image-clamp]', 'testRender[area-icon]', 'testRender[node-text]', 'testRender[node-shapes2]', 'testRender[way-dashes2]', 'testRender[order]', 'testRender[node-text2]', 'testRender[relation-parentselector]', 'testRender[node-shapes-combined]']
TEST-org.openstreetmap.josm.io.audio.AudioPlayerTest.xml: ['org.openstreetmap.josm.io.audio.AudioPlayerTest']
TEST-org.openstreetmap.josm.gui.mappaint.RenderingCLIAreaTest.xml: ['testDetermineRenderingArea[3]', 'testDetermineRenderingArea[2]', 'testDetermineRenderingArea[15]', 'testDetermineRenderingArea[6]', 'testDetermineRenderingArea[11]', 'testDetermineRenderingArea[5]', 'testDetermineRenderingArea[14]', 'testDetermineRenderingArea[13]', 'testDetermineRenderingArea[10]', 'testDetermineRenderingArea[8]', 'testDetermineRenderingArea[1]', 'testDetermineRenderingArea[9]', 'testDetermineRenderingArea[12]', 'testDetermineRenderingArea[4]', 'testDetermineRenderingArea[7]', 'testDetermineRenderingArea[0]']
TEST-org.openstreetmap.josm.io.UploadStrategySelectionPanelTest.xml: ['org.openstreetmap.josm.io.UploadStrategySelectionPanelTest']
Version 1, edited 6 months ago by taylor.smock (previous) (next) (diff)

Changed 6 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 6 months ago by taylor.smock

Attachment: junit_difference.py added

Show tests that have different numbers of passed/failed tests

comment:16 Changed 6 months ago by taylor.smock

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

comment:17 Changed 6 months ago by Don-vip

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

comment:18 Changed 5 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 5 months ago by Don-vip

Milestone: 19.1220.01

comment:20 Changed 5 months ago by Don-vip

In 15703/josm:

see #16567 - generate Jacoco CSV and XML reports

Changed 4 months ago by taylor.smock

Attachment: 16567.6.patch added

Update to ensure patch applies cleanly (see r15703)

comment:21 Changed 4 months ago by Don-vip

Milestone: 20.0120.02

comment:22 Changed 4 months ago by taylor.smock

Is there anything I need to do?

comment:23 in reply to:  22 Changed 4 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 4 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 3 months ago by Don-vip

Milestone: 20.0220.03

comment:26 Changed 3 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 6 weeks ago by taylor.smock

Attachment: 16567.9.patch added

Update for Ivy dependencies

comment:27 Changed 3 weeks ago by Klumbumbus

Milestone: 20.0420.05

Milestone renamed

comment:28 Changed 2 weeks ago by simon04

Milestone: 20.05Longterm

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain Don-vip.
as The resolution will be set.
to The owner will be changed from Don-vip 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.