Modify

Opened 6 years ago

Closed 4 months ago

Last modified 4 months ago

#16567 closed enhancement (fixed)

[PATCH] Upgrade to JUnit 5

Reported by: Don-vip Owned by: team
Priority: major Milestone: Longterm
Component: Unit tests Version:
Keywords: junit5 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 (28)

16567.patch (2.4 KB ) - added by taylor.smock 4 years ago.
Allow JUnit 5 tests to use JOSMTestRules
16567.1.patch (10.6 KB ) - added by taylor.smock 4 years 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 4 years ago.
Add script to get jar files (for easy downloading and testing)
16567.4.patch (10.6 KB ) - added by taylor.smock 4 years 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 4 years 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 4 years ago.
Show tests that have different numbers of passed/failed tests
16567.6.patch (10.7 KB ) - added by taylor.smock 4 years ago.
Update to ensure patch applies cleanly (see r15703)
16567.9.patch (13.6 KB ) - added by taylor.smock 4 years ago.
Update for Ivy dependencies
16567.11.patch (12.9 KB ) - added by taylor.smock 4 years 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 4 years ago.
This patch causes plenty of deprecation warnings…
16567.12.patch (12.7 KB ) - added by taylor.smock 4 years ago.
Remove unnecessary modification from JOSMTestRules
16567.deprecated.1.patch (52.1 KB ) - added by taylor.smock 4 years ago.
Fix (most) deprecation warnings. There is still one in ExpectedRootException and OptionParserTest.
16567.deprecated.2.patch (58.6 KB ) - added by taylor.smock 4 years 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 4 years 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 4 years ago.
Update for testMultipleShort changes
16567.fixup1.patch (1.5 KB ) - added by taylor.smock 4 years ago.
Ensure that after is not run until after the tests complete
16567.fixup1.1.patch (2.3 KB ) - added by taylor.smock 4 years 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 4 years 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 4 years 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 4 years ago.
16567.separate_all_each.patch (19.3 KB ) - added by taylor.smock 3 years ago.
Initial work on using {After,Before}{Each,All}Callbacks properly. Some tests are failing in Eclipse.
16567.separate_all_each.1.patch (21.5 KB ) - added by taylor.smock 3 years ago.
Use @ExtendWith for Override annotations
16567.initial_extensions.patch (16.6 KB ) - added by taylor.smock 3 years ago.
Initial extensions replacing JOSMTestRules#preferences and JOSMTestRules#assumeRevision (Override should probably be dropped from filenames)
16567.initial_extensions.1.patch (37.5 KB ) - added by taylor.smock 3 years ago.
Modify two tests to use the annotations, add i18n, Main, ApiType, Presets, Projection, ProjectionNadGrids, and Territories. Territories will probably be modified so that there can be different initialization types (e.g., full initialization or partial initialization).
16567.initial_extensions.2.patch (39.2 KB ) - added by taylor.smock 3 years ago.
Modify Territories.Extension so that there are various initialization states, and doesn't reinitialize when not needed
16567.initial_extensions.3.patch (189.1 KB ) - added by taylor.smock 3 years ago.
Modify Territories to have an uninitialize function (java-docs indicate that calling it outside of a test environment is a very bad idea (tm)), modify several tests to use the extensions (largely focused on longer tests, most taking >10s on my machine), add a utils class to find appropriate annotations and reset static classes, add replacements for assertionsInEDT, http, https, a new specific annotation for DeleteCommandCallback, setting up and cleaning up the layer environment, timezone usage, annotations for integration and slow tests
josm16567-junit5_assertions.patch (350.6 KB ) - added by gaben 2 years ago.
Replace Junit4 imports and assertions with the new Junit5 style code according to https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4-tips, plus minor simplifications
16567.2.patch (333.2 KB ) - added by taylor.smock 5 months ago.
More @Annotations, convert most tests to use them

Download all attachments as: .zip

Change History (188)

comment:1 by Don-vip, 6 years ago

Milestone: 18.08

comment:2 by Don-vip, 6 years ago

by taylor.smock, 4 years ago

Attachment: 16567.patch added

Allow JUnit 5 tests to use JOSMTestRules

comment:3 by taylor.smock, 4 years ago

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 4 years ago by taylor.smock (previous) (diff)

in reply to:  2 comment:4 by taylor.smock, 4 years ago

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 )

by taylor.smock, 4 years ago

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 by taylor.smock, 4 years ago

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 by taylor.smock, 4 years ago

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 by Don-vip, 4 years ago

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

comment:8 by taylor.smock, 4 years ago

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/)

by taylor.smock, 4 years ago

Attachment: junit_jars.sh added

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

in reply to:  8 comment:9 by taylor.smock, 4 years ago

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 by Don-vip, 4 years ago

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 by taylor.smock, 4 years ago

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?

in reply to:  11 ; comment:12 by Don-vip, 4 years ago

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.

in reply to:  12 comment:13 by taylor.smock, 4 years ago

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.

by taylor.smock, 4 years ago

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 by taylor.smock, 4 years ago

@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 by taylor.smock, 4 years ago

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 4 years ago by taylor.smock (previous) (diff)

by taylor.smock, 4 years ago

Attachment: 16567.5.patch added

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

by taylor.smock, 4 years ago

Attachment: junit_difference.py added

Show tests that have different numbers of passed/failed tests

comment:16 by taylor.smock, 4 years ago

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

comment:17 by Don-vip, 4 years ago

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

comment:18 by Don-vip, 4 years ago

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 by Don-vip, 4 years ago

Milestone: 19.1220.01

comment:20 by Don-vip, 4 years ago

In 15703/josm:

see #16567 - generate Jacoco CSV and XML reports

by taylor.smock, 4 years ago

Attachment: 16567.6.patch added

Update to ensure patch applies cleanly (see r15703)

comment:21 by Don-vip, 4 years ago

Milestone: 20.0120.02

comment:22 by taylor.smock, 4 years ago

Is there anything I need to do?

in reply to:  22 comment:23 by Don-vip, 4 years ago

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 by taylor.smock, 4 years ago

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 by Don-vip, 4 years ago

Milestone: 20.0220.03

comment:26 by Don-vip, 4 years ago

Milestone: 20.0320.04

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

by taylor.smock, 4 years ago

Attachment: 16567.9.patch added

Update for Ivy dependencies

comment:27 by Klumbumbus, 4 years ago

Milestone: 20.0420.05

Milestone renamed

comment:28 by simon04, 4 years ago

Milestone: 20.05Longterm

comment:29 by taylor.smock, 4 years ago

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 years ago by taylor.smock (previous) (diff)

by taylor.smock, 4 years ago

Attachment: 16567.11.patch added

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

by simon04, 4 years ago

Attachment: assertThat-warning.txt added

This patch causes plenty of deprecation warnings…

comment:30 by taylor.smock, 4 years ago

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.

by taylor.smock, 4 years ago

Attachment: 16567.12.patch added

Remove unnecessary modification from JOSMTestRules

by taylor.smock, 4 years ago

Attachment: 16567.deprecated.1.patch added

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

comment:31 by taylor.smock, 4 years ago

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 4 years ago by taylor.smock (previous) (diff)

by taylor.smock, 4 years ago

Attachment: 16567.deprecated.2.patch added

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

by taylor.smock, 4 years ago

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 by taylor.smock, 4 years ago

@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 by Don-vip, 4 years ago

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

comment:34 by simon04, 4 years ago

In 16608/josm:

see #16567 - Fix OptionParserTest.testMultipleShort

by taylor.smock, 4 years ago

Attachment: 16567.deprecated.4.patch added

Update for testMultipleShort changes

comment:35 by simon04, 4 years ago

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 4 years ago by simon04 (previous) (diff)

comment:36 by simon04, 4 years ago

In 16618/josm:

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

I applied attachment:16567.deprecated.4.patch

Last edited 4 years ago by simon04 (previous) (diff)

comment:37 by simon04, 4 years ago

Milestone: Longterm20.06

comment:38 by simon04, 4 years ago

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 by simon04, 4 years ago

Resolution: fixed
Status: closedreopened

comment:40 by simon04, 4 years ago

In 16633/josm:

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

by taylor.smock, 4 years ago

Attachment: 16567.fixup1.patch added

Ensure that after is not run until after the tests complete

comment:41 by taylor.smock, 4 years ago

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.

by taylor.smock, 4 years ago

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 by simon04, 4 years ago

In 16658/josm:

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

comment:43 by simon04, 4 years ago

In 16659/josm:

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

in reply to:  43 comment:44 by taylor.smock, 4 years ago

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 by simon04, 4 years ago

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 :-(

by taylor.smock, 4 years ago

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 by taylor.smock, 4 years ago

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).

in reply to:  48 comment:49 by taylor.smock, 4 years ago

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 4 years ago by taylor.smock (previous) (diff)

comment:50 by simon04, 4 years ago

@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 by taylor.smock, 4 years ago

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 by simon04, 4 years ago

Resolution: fixed
Status: reopenedclosed

In 16727/josm:

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

comment:53 by simon04, 4 years ago

@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 by Klumbumbus, 4 years ago

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/

in reply to:  54 comment:55 by taylor.smock, 4 years ago

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.

by taylor.smock, 4 years 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.

by taylor.smock, 4 years ago

comment:56 by Klumbumbus, 4 years ago

Milestone: 20.0620.07
Resolution: fixed
Status: closedreopened

in reply to:  52 ; comment:58 by Klumbumbus, 4 years ago

Replying to simon04:

In 16727/josm:

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

It is not yet fully fixed. The overview at https://josm.openstreetmap.de/jenkins/job/JOSM-Imagery-Integration/lastCompletedBuild/testReport/ should display the imagery name instead of "String, ImageryInfo".

in reply to:  58 comment:59 by taylor.smock, 4 years ago

Replying to Klumbumbus:

Replying to simon04:

In 16727/josm:

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

It is not yet fully fixed. The overview at https://josm.openstreetmap.de/jenkins/job/JOSM-Imagery-Integration/lastCompletedBuild/testReport/ should display the imagery name instead of "String, ImageryInfo".

That would be ideal. Unfortunately, that is (currently) the way that JUnit5 reports the test (see comment:53). That particular bug hasn't had any further activity on the ant bug tracker, but I have no idea how long it usually takes them to look at a bug and respond.

My patch, TBH, is a workaround so that we can at least figure out what the failing imagery was, although we have to actually read the logs. I, quite frankly, didn't expect JUnit5 to have a regression like this, and I didn't look at the parameterized tests outside of Eclipse (which does show parameters, albeit not the same way that JUnit4 shows parameters).

comment:60 by simon04, 4 years ago

In 16765/josm:

see #16567 - Fix DomainValidatorTestIT for JUnit 5 (patch by taylor.smock)

comment:61 by simon04, 4 years ago

Milestone: 20.07Longterm

in reply to:  53 comment:62 by simon04, 4 years ago

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)

I found a pull request resolving this issue: https://github.com/apache/ant/pull/121 Unfortunately it is pending since 2020-01…

comment:63 by simon04, 3 years ago

In 17135/josm:

see #16567 - Update to JUnit 5.7.0

CHANGELOG: https://junit.org/junit5/docs/5.7.0/release-notes/

comment:64 by Don-vip, 3 years ago

@Taylor I'm writing a simple test:

// License: GPL. For details, see LICENSE file.
package org.openstreetmap.josm.gui.dialogs;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.Date;

import javax.swing.JLabel;
import javax.swing.JList;

import org.junit.Rule;
import org.junit.jupiter.api.Test;
import org.openstreetmap.josm.data.coor.LatLon;
import org.openstreetmap.josm.data.notes.Note;
import org.openstreetmap.josm.data.notes.NoteComment;
import org.openstreetmap.josm.data.osm.User;
import org.openstreetmap.josm.gui.dialogs.NotesDialog.NoteRenderer;
import org.openstreetmap.josm.testutils.JOSMTestRules;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
 * Unit tests of {@link NotesDialog}
 */
class NotesDialogTest {

    /**
     * Setup tests
     */
    @Rule
    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
    public JOSMTestRules josmTestRules = new JOSMTestRules().preferences();

    @Test
    void testMultiLineNoteRendering() {
        Note note = new Note(LatLon.ZERO);
        note.setCreatedAt(new Date());
        note.addComment(new NoteComment(new Date(), User.createLocalUser(null), "foo\nbar\n\nbaz", null, false));
        assertEquals("foo; bar; baz",
                ((JLabel) new NoteRenderer().getListCellRendererComponent(new JList<>(), note, 0, false, false)).getText());
    }
}

which fails because the return value of "Config.getPref()" is null

java.lang.ExceptionInInitializerError
	at org.openstreetmap.josm.gui.dialogs.NotesDialog$NoteRenderer.getListCellRendererComponent(NotesDialog.java:262)
	at org.openstreetmap.josm.gui.dialogs.NotesDialogTest.testMultiLineNoteRendering(NotesDialogTest.java:40)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:564)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:686)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
	at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:205)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:201)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:137)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:71)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:248)
	at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$5(DefaultLauncher.java:211)
	at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:226)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:199)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:141)
	at org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(JUnit5TestReference.java:98)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:542)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:770)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:464)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
Caused by: java.lang.NullPointerException: Cannot invoke "org.openstreetmap.josm.spi.preferences.IPreferences.getInt(String, int)" because the return value of "org.openstreetmap.josm.spi.preferences.Config.getPref()" is null
	at org.openstreetmap.josm.tools.ImageProvider$ImageSizes.<clinit>(ImageProvider.java:121)
	... 67 more

In debug I can see that JOSMTestRules.preferences() is called, but not JOSMTestRules.before().

Any idea what's wrong?

in reply to:  64 ; comment:65 by taylor.smock, 3 years ago

Replying to Don-vip:

@Taylor I'm writing a simple test:

I don't (currently) have an idea as to what is wrong. I'll have to step through it tomorrow.

When I converted JOSMTestRules to work with JUnit5, I took care to not modify any existing code, IIRC. I think the only thing I really changed was adding a junit5 boolean, which conditionally executed after(), and the junit5 boolean shouldn't be touched by your code. Just out of curiosity, what happens when you use the JUnit 5 equivalents?

in reply to:  65 ; comment:66 by Don-vip, 3 years ago

Replying to taylor.smock:

Just out of curiosity, what happens when you use the JUnit 5 equivalents?

I didn't try yet, what's the JUnit5 syntax to use?

in reply to:  66 comment:67 by simon04, 3 years ago

Replying to Don-vip:

I didn't try yet, what's the JUnit5 syntax to use?

org.junit.jupiter.api.extension.RegisterExtension, see org.openstreetmap.josm.gui.preferences.imagery.ImageryPreferenceTestIT for instance.

in reply to:  66 ; comment:68 by taylor.smock, 3 years ago

Replying to Don-vip:

Replying to taylor.smock:

Just out of curiosity, what happens when you use the JUnit 5 equivalents?

I didn't try yet, what's the JUnit5 syntax to use?

// License: GPL. For details, see LICENSE file.
package org.openstreetmap.josm.gui.dialogs;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.Date;

import javax.swing.JLabel;
import javax.swing.JList;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.openstreetmap.josm.data.coor.LatLon;
import org.openstreetmap.josm.data.notes.Note;
import org.openstreetmap.josm.data.notes.NoteComment;
import org.openstreetmap.josm.data.osm.User;
import org.openstreetmap.josm.gui.dialogs.NotesDialog.NoteRenderer;
import org.openstreetmap.josm.testutils.JOSMTestRules;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
 * Unit tests of {@link NotesDialog}
 */
class NotesDialogTest {

    /**
     * Setup tests
     */
    @RegisterExtension
    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
    public JOSMTestRules josmTestRules = new JOSMTestRules().preferences();

    @Test
    void testMultiLineNoteRendering() {
        Note note = new Note(LatLon.ZERO);
        note.setCreatedAt(new Date());
        note.addComment(new NoteComment(new Date(), User.createLocalUser(null), "foo\nbar\n\nbaz", null, false));
        assertEquals("foo; bar; baz",
                ((JLabel) new NoteRenderer().getListCellRendererComponent(new JList<>(), note, 0, false, false)).getText());
    }
}

OK. I think I see what the problem was. @Rule isn't used by JUnit5, which means the code was never executed. I skimmed the code last night and missed that you were using the @Test annotation from JUnit5.

in reply to:  68 ; comment:69 by Don-vip, 3 years ago

Replying to taylor.smock:

OK. I think I see what the problem was. @Rule isn't used by JUnit5, which means the code was never executed. I skimmed the code last night and missed that you were using the @Test annotation from JUnit5.

Ah ok we must either use only JUnit4 annotations or JUnit 5 ones, not a mix of them, right?

in reply to:  69 comment:70 by taylor.smock, 3 years ago

Replying to Don-vip:

Replying to taylor.smock:

OK. I think I see what the problem was. @Rule isn't used by JUnit5, which means the code was never executed. I skimmed the code last night and missed that you were using the @Test annotation from JUnit5.

Ah ok we must either use only JUnit4 annotations or JUnit 5 ones, not a mix of them, right?

I'd stick with that. You can probably do something like this:

...
import org.junit.Rule;
import org.junit.jupiter.api.extension.RegisterExtension;
...
@Rule
@RegisterExtension
public JOSM TestRules josmTestRules = new JOSMTestRules().preferences();
@org.junit.Test
public void test4() {
...
}
@org.junit.jupiter.api.Test
void test5() {
...
}

But I think we should probably stick with one or the other per test file.

comment:71 by Don-vip, 3 years ago

In 17275/josm:

see #16567 - upgrade almost all tests to JUnit 5, except those depending on WiremockRule

See https://github.com/tomakehurst/wiremock/issues/684

comment:72 by Don-vip, 3 years ago

In 17276/josm:

see #16567 - fix obvious test errors + upgrade tests dependencies

comment:73 by Don-vip, 3 years ago

In 17277/josm:

see #16567 - fix more obvious test errors

comment:74 by Don-vip, 3 years ago

I don't understand the errors in tests using JMockit @Injectable and @Mocked annotations. In Eclipse, the tests run OK if I add the -javaagent parameter, as required by JMockit. I can reproduce the error when I remove the -javaagent argument, but we already have it in the build.xml, so I'm kind of lost here... Any clue someone?

comment:75 by Don-vip, 3 years ago

In 17279/josm:

see #16567 - revert r17275 for CycleLayerActionTest / MinimapDialogTest

They depend on fakeImagery, which does not work yet with JUnit 5

comment:76 by Don-vip, 3 years ago

In 17280/josm:

see #16567 - fix assertEquals parameters order

comment:77 by GerdP, 3 years ago

What do I have to change in Eclipse to run unit tests?

in reply to:  74 ; comment:78 by taylor.smock, 3 years ago

Replying to Don-vip:

I don't understand the errors in tests using JMockit @Injectable and @Mocked annotations. In Eclipse, the tests run OK if I add the -javaagent parameter, as required by JMockit. I can reproduce the error when I remove the -javaagent argument, but we already have it in the build.xml, so I'm kind of lost here... Any clue someone?

Which tests are failing?
I just ran ant test-html on Mac (took a little while), and the following tests had issues:

  • MapCSSRendererTest (looks like UI differences, probably not an issue)
  • ImageryPreferenceTestIT (I'm ignoring these)
  • AuthenticationPreferenceTest (java.lang.NoClassDefFoundError, didn't see any mocks, from test/unit/org/openstreetmap/josm/gui/preferences/server/ServerAccessPreferenceTest.java)
  • OverpassServerPreferenceTest (NoClassDefFoundError again, didn't see any mocks, from test/unit/org/openstreetmap/josm/gui/preferences/server/ServerAccessPreferenceTest.java)

in reply to:  77 comment:79 by taylor.smock, 3 years ago

Replying to GerdP:

What do I have to change in Eclipse to run unit tests?

1) Change the test launcher to JUnit5

a) Down arrow next to Debug/Run/Coverage
b) Select {Debug,Run,Coverage} Configurations
c) Select the JUnit test(s) that you want to run under JUnit5
d) Change "Test runner" from "Junit 4" to "JUnit 5"
e) Apply

2) Add JUnit 5 to the classpath (may already be done)

a) Right click the JOSM project
b) Build Path -> Configure Build Path
c) Libraries
d) Remove JUnit 4
e) Add Library
f) JUnit
g) Set "JUnit 5" as the version
h) Finish and apply

Hopefully I didn't miss anything. :)

comment:80 by GerdP, 3 years ago

My Eclipse doesn't know JUnit 5. It offers 3 and 4.

in reply to:  80 comment:81 by taylor.smock, 3 years ago

Replying to GerdP:

My Eclipse doesn't know JUnit 5. It offers 3 and 4.

What version of Eclipse are you running? (Also, you can ping me on IRC if you want)

comment:82 by GerdP, 3 years ago

Version: Neon.3 Release (4.6.3)
Build id: 20170314-1500

Don't know how to use IRC. Sorry for my ignorance.

in reply to:  82 comment:83 by taylor.smock, 3 years ago

Replying to GerdP:

Version: Neon.3 Release (4.6.3)
Build id: 20170314-1500

Don't know how to use IRC. Sorry for my ignorance.

Don't worry about not knowing how to use IRC. I was just putting it out there to avoid spamming other people's inboxes.

You should update Eclipse. It looks like JUnit5 support was introduced in 4.7.1a ( see https://www.eclipse.org/community/eclipse_newsletter/2017/october/article5.php ).

comment:84 by GerdP, 3 years ago

OK, let's see where that takes me...

comment:85 by Don-vip, 3 years ago

@Gerd I waited three years after initial Eclipse support, it's time to update :D There's a new version of Eclipse every three months. Once you have a recent version, there is nothing special to do. Eclipse selects the correct JUnit version by itself.

in reply to:  78 ; comment:86 by Don-vip, 3 years ago

Replying to taylor.smock:

Which tests are failing?

You can check on Jenkins:

  • With Java 11, everything's fine. No error
  • With Java 8, all tests involving JMockit injected arguments fail with message "Failed to resolve parameter..." I guess we should open a bug report to JMockit, but it's difficult to do so with the library author :(

in reply to:  85 ; comment:87 by GerdP, 3 years ago

Replying to Don-vip:

@Gerd I waited three years after initial Eclipse support, it's time to update :D There's a new version of Eclipse every three months. Once you have a recent version, there is nothing special to do. Eclipse selects the correct JUnit version by itself.

Well, the latest Eclipse version requires JDK 11. Not sure how to set up everything without risking to produce binaries which don't work on JRE8. On my laptop I use a newer version of Eclipse that supports JUnit5, but it it not able to compile JOSM several tests.
No I have to configure my new Eclipse Version: 2020-09 (4.17.0) ...

in reply to:  86 ; comment:88 by taylor.smock, 3 years ago

Replying to Don-vip:

You can check on Jenkins:

  • With Java 11, everything's fine. No error
  • With Java 8, all tests involving JMockit injected arguments fail with message "Failed to resolve parameter..." I guess we should open a bug report to JMockit, but it's difficult to do so with the library author :(

OK. I ran the tests locally with Java 11. I'll start looking at Java 8. I thought I had some tests in MapWithAI that were using mockers, and I do, but I haven't switched them over to JUnit 5 yet.

Anyway, on Mac I just ran JAVA_HOME=$(/usr/libexec/java_home -v 1.8) ant test-clean test '-Ddefault-junit-includes=**/PlatformHookWindowsTest.class' -Dglass.platform=Monocle -Dmonocle.platform=Headless -Dprism.order=sw; cat test/report/TEST-org.openstreetmap.josm.tools.PlatformHookWindowsTest.txt, which output

Testcase: org.openstreetmap.josm.tools.PlatformHookWindowsTest
Test: testGetRootKeystore() took 1 sec(s)
Test: testStartupHook() took 494 milli sec(s)
Test: testAfterPrefStartupHook() took 642 milli sec(s)
Test: testGetDefaultPrefDirectory() took 492 milli sec(s)
Test: testOpenUrlSuccess(Desktop) took 788 milli sec(s)
Test: testGetInstalledFonts() took 510 milli sec(s)
Test: testInitSystemShortcuts() took 598 milli sec(s)
Test: testGetDefaultCacheDirectory() took 656 milli sec(s)
Test: testOpenUrlFallback(Desktop, Runtime) took 917 milli sec(s)
Test: testGetDefaultStyle() took 536 milli sec(s)
Test: testGetOSDescription() took 593 milli sec(s)
Test: testGetAdditionalFonts() took 508 milli sec(s)
Tests run: 12, Failures: 0, Skipped: 0, Aborted: 0, Time elapsed: 8 sec(s)

Are we using Ubuntu 18.04, or have we updated to Ubuntu 20.04 for CI?

in reply to:  88 ; comment:89 by Don-vip, 3 years ago

Replying to taylor.smock:

Are we using Ubuntu 18.04, or have we updated to Ubuntu 20.04 for CI?

We're running Ubuntu 18.04.5 LTS up-to-date. Yesterday I updated these packages and needed to commit r17278:

  • openjdk-11-jre:amd64 (11.0.9+11-0ubuntu1~18.04.1) over (11.0.8+10-0ubuntu1~18.04.1)
  • openjdk-8-jre:amd64 (8u272-b10-0ubuntu1~18.04) over (8u265-b01-0ubuntu2~18.04)

in reply to:  87 comment:90 by Don-vip, 3 years ago

Replying to GerdP:

Well, the latest Eclipse version requires JDK 11. Not sure how to set up everything without risking to produce binaries which don't work on JRE8.

You can setup as many JDK as you want. You need Java 11 to run Eclipse but you can setup Java 8 as default JDK in compiler settings.

in reply to:  89 comment:91 by taylor.smock, 3 years ago

Replying to Don-vip:

We're running Ubuntu 18.04.5 LTS up-to-date. Yesterday I updated these packages and needed to commit r17278:

  • openjdk-11-jre:amd64 (11.0.9+11-0ubuntu1~18.04.1) over (11.0.8+10-0ubuntu1~18.04.1)
  • openjdk-8-jre:amd64 (8u272-b10-0ubuntu1~18.04) over (8u265-b01-0ubuntu2~18.04)

Well, it doesn't happen on 20.04, so I'll have to make a VM Of 18.04 for testing.

comment:92 by Don-vip, 3 years ago

In 17288/josm:

see #16567 - fix integration tests

comment:93 by Don-vip, 3 years ago

In 17304/josm:

see #16567 - use our own patched version of JMockit to see if it solves the JUnit 5/Java 8 test failures

https://github.com/JOSM/jmockit1/releases/tag/v1.49.a manually uploaded to https://josm.openstreetmap.de/nexus/

comment:94 by Don-vip, 3 years ago

More details in text file:

Test: testSortData(Collections) took 121 milli sec(s) FAILED: Failed to resolve parameter [java.util.Collections arg0] in method [void org.openstreetmap.josm.data.ImageDataTest.testSortData(java.util.Collections)]: call site initialization exception
org.junit.jupiter.api.extension.ParameterResolutionException: Failed to resolve parameter [java.util.Collections arg0] in method [void org.openstreetmap.josm.data.ImageDataTest.testSortData(java.util.Collections)]: call site initialization exception
        at org.apache.tools.ant.taskdefs.optional.junitlauncher.LauncherSupport.launch(LauncherSupport.java:141)
        at org.apache.tools.ant.taskdefs.optional.junitlauncher.StandaloneLauncher.main(StandaloneLauncher.java:114)
Caused by: java.lang.BootstrapMethodError: call site initialization exception
        at java.lang.invoke.CallSite.makeSite(CallSite.java:341)
        at java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:307)
        at java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:297)
        ... 2 more
Caused by: java.lang.VerifyError: Bad local variable type
Exception Details:
  Location:
    org/junit/jupiter/engine/execution/ExecutableInvoker$$Lambda$1475.get$Lambda()Ljava/util/function/Predicate; @4: aload_0
  Reason:
    Type top (current frame, locals[0]) is not assignable to reference type
  Current Frame:
    bci: @4
    flags: { }
    locals: { }
    stack: { uninitialized 0, uninitialized 0 }
  Bytecode:
    0x0000000: bb00 0259 2a2b b700 15b0

        at sun.misc.Unsafe.defineAnonymousClass(Native Method)
        at java.lang.invoke.InnerClassLambdaMetafactory.spinInnerClass(InnerClassLambdaMetafactory.java:326)
        at java.lang.invoke.InnerClassLambdaMetafactory.buildCallSite(InnerClassLambdaMetafactory.java:194)
        at java.lang.invoke.LambdaMetafactory.metafactory(LambdaMetafactory.java:304)
        at java.lang.invoke.CallSite.makeSite(CallSite.java:302)
        ... 4 more

comment:95 by Don-vip, 3 years ago

In Eclipse I can reproduce with a similar error, but not exactly the same:

Caused by: java.lang.VerifyError: Bad local variable type
Exception Details:
  Location:
    org/junit/jupiter/engine/descriptor/TestMethodTestDescriptor$$Lambda$280.<init>()V @5: aload_1
  Reason:
    Type top (current frame, locals[1]) is not assignable to reference type
  Current Frame:
    bci: @5
    flags: { }
    locals: { 'org/junit/jupiter/engine/descriptor/TestMethodTestDescriptor$$Lambda$280' }
    stack: { 'org/junit/jupiter/engine/descriptor/TestMethodTestDescriptor$$Lambda$280' }
  Bytecode:
    0x0000000: 2ab7 0010 2a2b b500 122a 2cb5 0014 2a2d
    0x0000010: b500 16b1                              

Stacktrace:

Thread [main] (Suspended (exception java.lang.VerifyError))	
	java.lang.invoke.InnerClassLambdaMetafactory.spinInnerClass() line: 326 [local variables unavailable]	
	java.lang.invoke.InnerClassLambdaMetafactory.buildCallSite() line: 194	
	java.lang.invoke.LambdaMetafactory.metafactory(java.lang.invoke.MethodHandles$Lookup, java.lang.String, java.lang.invoke.MethodType, java.lang.invoke.MethodType, java.lang.invoke.MethodHandle, java.lang.invoke.MethodType) line: 304	
	java.lang.invoke.LambdaForm$DMH.1514322932.invokeStatic_L6_L(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object) line: not available	
	java.lang.invoke.LambdaForm$BMH.1121453612.reinvoke(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object) line: not available	
	java.lang.invoke.LambdaForm$MH.128526626.invoke_MT(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object) line: not available	
	java.lang.invoke.CallSite.makeSite(java.lang.invoke.MethodHandle, java.lang.String, java.lang.invoke.MethodType, java.lang.Object, java.lang.Class<?>) line: 302	
	java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(java.lang.Class<?>, java.lang.invoke.MethodHandle, java.lang.String, java.lang.invoke.MethodType, java.lang.Object, java.lang.Object[]) line: 307	
	java.lang.invoke.MethodHandleNatives.linkCallSite(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object[]) line: 297	
	org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(org.junit.jupiter.engine.execution.JupiterEngineExecutionContext, org.junit.platform.engine.support.hierarchical.Node$DynamicTestExecutor) line: 206	
	org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(org.junit.jupiter.engine.execution.JupiterEngineExecutionContext, org.junit.platform.engine.support.hierarchical.Node$DynamicTestExecutor) line: 131	
	org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(org.junit.platform.engine.support.hierarchical.EngineExecutionContext, org.junit.platform.engine.support.hierarchical.Node$DynamicTestExecutor) line: 65	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$5() line: 139	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$214.103536485.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$7(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: 129	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$213.572191680.invoke(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: not available	
	org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor(org.junit.platform.engine.support.hierarchical.Node<C>).around(C, org.junit.platform.engine.support.hierarchical.Node.Invocation<C>) line: 137	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$8() line: 127	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$212.1007309018.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.executeRecursively() line: 126	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.execute() line: 84	
	org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService$$Lambda$218.2076287037.accept(java.lang.Object) line: not available	
	java.util.ArrayList<E>.forEach(java.util.function.Consumer<? super E>) line: 1259	
	org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(java.util.List<? extends org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService.TestTask>) line: 38	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$5() line: 143	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$214.103536485.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$7(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: 129	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$213.572191680.invoke(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: not available	
	org.junit.jupiter.engine.descriptor.ClassTestDescriptor(org.junit.platform.engine.support.hierarchical.Node<C>).around(C, org.junit.platform.engine.support.hierarchical.Node.Invocation<C>) line: 137	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$8() line: 127	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$212.1007309018.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.executeRecursively() line: 126	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.execute() line: 84	
	org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService$$Lambda$218.2076287037.accept(java.lang.Object) line: not available	
	java.util.ArrayList<E>.forEach(java.util.function.Consumer<? super E>) line: 1259	
	org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(java.util.List<? extends org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService.TestTask>) line: 38	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$5() line: 143	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$214.103536485.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$7(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: 129	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$213.572191680.invoke(org.junit.platform.engine.support.hierarchical.EngineExecutionContext) line: not available	
	org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor(org.junit.platform.engine.support.hierarchical.Node<C>).around(C, org.junit.platform.engine.support.hierarchical.Node.Invocation<C>) line: 137	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.lambda$executeRecursively$8() line: 127	
	org.junit.platform.engine.support.hierarchical.NodeTestTask$$Lambda$212.1007309018.execute() line: not available	
	org.junit.jupiter.engine.support.OpenTest4JAndJUnit4AwareThrowableCollector(org.junit.platform.engine.support.hierarchical.ThrowableCollector).execute(org.junit.platform.engine.support.hierarchical.ThrowableCollector$Executable) line: 73	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.executeRecursively() line: 126	
	org.junit.platform.engine.support.hierarchical.NodeTestTask<C>.execute() line: 84	
	org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService.TestTask) line: 32	
	org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor<C>.execute() line: 57	
	org.junit.jupiter.engine.JupiterTestEngine(org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine<C>).execute(org.junit.platform.engine.ExecutionRequest) line: 51	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(org.junit.platform.engine.TestDescriptor, org.junit.platform.engine.EngineExecutionListener, org.junit.platform.engine.ConfigurationParameters, org.junit.platform.engine.TestEngine) line: 108	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(org.junit.platform.launcher.core.LauncherDiscoveryResult, org.junit.platform.engine.EngineExecutionListener) line: 88	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(org.junit.platform.launcher.core.InternalTestPlan, org.junit.platform.launcher.core.LauncherDiscoveryResult, org.junit.platform.launcher.TestExecutionListener) line: 54	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator$$Lambda$170.1920387277.accept(java.lang.Object) line: not available	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(org.junit.platform.engine.ConfigurationParameters, org.junit.platform.launcher.core.TestExecutionListenerRegistry, java.util.function.Consumer<org.junit.platform.launcher.TestExecutionListener>) line: 67	
	org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(org.junit.platform.launcher.core.InternalTestPlan, org.junit.platform.launcher.TestExecutionListener...) line: 52	
	org.junit.platform.launcher.core.DefaultLauncher.execute(org.junit.platform.launcher.core.InternalTestPlan, org.junit.platform.launcher.TestExecutionListener[]) line: 96	
	org.junit.platform.launcher.core.DefaultLauncher.execute(org.junit.platform.launcher.TestPlan, org.junit.platform.launcher.TestExecutionListener...) line: 84	
	org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(org.eclipse.jdt.internal.junit.runner.TestExecution) line: 98	
	org.eclipse.jdt.internal.junit.runner.TestExecution.run(org.eclipse.jdt.internal.junit.runner.ITestReference[]) line: 41	
	org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(java.lang.String[], java.lang.String, org.eclipse.jdt.internal.junit.runner.TestExecution) line: 542	
	org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(org.eclipse.jdt.internal.junit.runner.TestExecution) line: 770	
	org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run() line: 464	
	org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(java.lang.String[]) line: 210	

comment:96 by Don-vip, 3 years ago

In 17310/josm:

see #16567 - rewrite ImageDataTest to get rid of problematic JMockit annotation

comment:97 by Don-vip, 3 years ago

Interestingly, RenderingCLIAreaTest fails without using JMockit...

comment:98 by Don-vip, 3 years ago

WTF, after Ubuntu upgrade that contains openjdk 8u275, everything's fine again. So it was likely a weird Java bug...

comment:99 by Stereo, 3 years ago

The CI at https://github.com/openstreetmap/josm/actions use Ubuntu 18.04, macOS Catalina 10.15 and Windows Server 2019, with java 8, 11, 15 and 16-ea.

We can run the tests on anything that's listed on https://docs.github.com/en/free-pro-team@latest/actions/reference/specifications-for-github-hosted-runners#supported-runners-and-hardware-resources

16-ea builds seem to be failing right now.

in reply to:  99 comment:100 by Don-vip, 3 years ago

Replying to Stereo:

16-ea builds seem to be failing right now.

See ticket:19724#comment:3

comment:101 by Don-vip, 3 years ago

In 35642/osm:

see #16567 - see #20085 - JUnit to JUnitLauncher for plugins (patch by taylor.smock)

comment:103 by simon04, 3 years ago

Nice, our commit is linked to the Apache Ant GitHub issue: https://github.com/apache/ant/pull/121#ref-commit-dfd4e3b

in reply to:  103 comment:104 by Don-vip, 3 years ago

Replying to simon04:

Nice, our commit is linked to the Apache Ant GitHub issue: https://github.com/apache/ant/pull/121#ref-commit-dfd4e3b

This is a GitHub feature I love :)

comment:105 by Don-vip, 3 years ago

Following my comment the feature has been implemented in Ant master:

                    <listener type="legacy-plain" useLegacyReportingName="false" />
                    <listener type="legacy-xml" useLegacyReportingName="false" />

Unfortunately I can't commit the new attributes right now as older versions of Ant fail on unknown tag:

BUILD FAILED
C:\SVN\josm\core\build.xml:505: The following error occurred while executing this line:
C:\SVN\josm\core\build.xml:434: listener doesn't support the "useLegacyReportingName" attribute

So until ant 1.10.10 is released and widely used in IDEs and Linux distributions, I'll tell Jenkins to patch build.xml on the fly, so that we can get our old reports back.

comment:106 by Don-vip, 3 years ago

Done: ant 1.10.10alpha is now used for JOSM integration tests, and we have nice errors again:

org.openstreetmap.josm.gui.preferences.map.TaggingPresetPreferenceTestIT.Persian Presets - https://github.com/DearRude/IranianPresets/blob/master/zip/latast.zip

@Klumbumbus it will be easy again to find which extension is broken.

comment:107 by Klumbumbus, 3 years ago

Great.

comment:108 by Klumbumbus, 3 years ago

see https://josm.openstreetmap.de/jenkins/job/JOSM-Integration/5463/testReport/
For Rules, Plugins and possibly Styles there is still no information which one is the culprit. However I'm not sure if it was the same before the switch to JUnit 5.

comment:109 by Don-vip, 3 years ago

In 17383/josm:

see #16567 - better assertion error messages for rules/plugins integration tests

comment:110 by GerdP, 3 years ago

I've noticed that some unit tests call JOSMFixture.createUnitTestFixture().init(); in @BeforeEach, others do this only in @BeforeAll. The latter is much faster, so I wonder if the former is needed or not and if needed, what are the criteria?

in reply to:  110 comment:111 by taylor.smock, 3 years ago

Replying to GerdP:

I've noticed that some unit tests call JOSMFixture.createUnitTestFixture().init(); in @BeforeEach, others do this only in @BeforeAll. The latter is much faster, so I wonder if the former is needed or not and if needed, what are the criteria?

It pretty much comes down to how the JUnit extension is registered (JOSMTestRules). If it is static, then it is essentially called in @BeforeAll. If it is not static, then it is essentially called in @BeforeEach. This is largely due to how I implemented JUnit5 support in JOSMTestRules -- I specifically wanted to keep current behavior w.r.t. JUnit4 to make it easier for people to port tests (especially the plugin users, and I don't anticipate being able to remove the JUnit4 support until mid to late 2021, at the earliest).

I suspect we could technically make modifications such that JOSMFixture is only called in the @BeforeAll, but that requires:
a) Making all JOSMTestRules static in our source code
b) Waiting on plugin authors to update their tests so that all of their JOSMTestRules are static

I'm pretty certain that we can have static JOSMTestRules have a beforeAll and a beforeEach behavior, and an afterAll and afterEach behavior. We just have to refactor the current code. Which I didn't do specifically to allow plugin authors a chance to update their unit tests to JUnit 5.

Plugins that I know have moved to JUnit 5:

  • Mapillary
  • MapWithAI (partial, mostly there)

Docs for @RegisterExtension:
https://junit.org/junit5/docs/5.1.1/api/org/junit/jupiter/api/extension/RegisterExtension.html

It actually looks like, if JOSMTestRules is static, that beforeEach and beforeAll are both called. I'll see if that bears out with a debugger. :(

comment:112 by GerdP, 3 years ago

Ok, sounds complicated. What's the difference between e.g. SelfIntersectingWayTest which uses @BeforeALl and SharpAnglesTest which uses @BeforeEach?

in reply to:  112 comment:113 by taylor.smock, 3 years ago

Replying to GerdP:

Ok, sounds complicated. What's the difference between e.g. SelfIntersectingWayTest which uses @BeforeALl and SharpAnglesTest which uses @BeforeEach?

Um... I don't remember why I wasn't using JOSMTestRules in SharpAnglesTest. But the fixture should probably be in a @BeforeAll method. I haven't actually looked at the fixture, but it is also initialized in JOSMTestRules.

comment:114 by Don-vip, 3 years ago

JOSMFixture is the old mechanism, you should avoid it for new tests. Everything should be doable with JOSMTestRules only.

comment:115 by GerdP, 3 years ago

I try to understand the output of ImageryPreferenceTestIT. What is the purpose of this test? What are the expected actions to fix a problem reported by this test?

comment:116 by Klumbumbus, 3 years ago

It tests if the source actually delivers an image and if the urls (attribution url, icon url,...) are working. In the last test https://josm.openstreetmap.de/jenkins/job/JOSM-Imagery-Integration/lastCompletedBuild/testReport/ 98 failed. Some might be false positives and actually work in JOSM, these could be put on ignorelist at IntegrationTestIgnores. Vincent also somtimes did report to the providers if there are incomplete ssl certificate chains or things like that.

comment:117 by Klumbumbus, 3 years ago

If it is an real error it should be fixed on the subpages of wiki:/Maps

in reply to:  117 comment:118 by GerdP, 3 years ago

Replying to Klumbumbus:

If it is an real error it should be fixed on the subpages of wiki:/Maps

OK, but how does one find out that it is a real error? Is this something that can be done by anybody?

comment:119 by Klumbumbus, 3 years ago

Trying if the imagery works in JOSM / checking if an URL is not dead/404?

comment:120 by Don-vip, 3 years ago

Can we please keep this ticket for tracking JUnit 5 migration?

comment:121 by Don-vip, 3 years ago

In 17403/josm:

see #16567 - JUnit 5: enable parallel execution mode with command line parameters

See https://junit.org/junit5/docs/snapshot/user-guide/index.html#writing-tests-parallel-execution

comment:122 by michael2402, 3 years ago

Does anyone have a list of which patches in this ticket are applied and which are not?

by taylor.smock, 3 years ago

Initial work on using {After,Before}{Each,All}Callbacks properly. Some tests are failing in Eclipse.

in reply to:  122 comment:123 by taylor.smock, 3 years ago

Replying to michael2402:

Does anyone have a list of which patches in this ticket are applied and which are not?

I don't know about anything else, but all attachments prior to attachment:16567.fixup1.1.patch is either in the above list, or was not applied.

comment:124 by michael2402, 3 years ago

@taylor Only eclipse? Or also Jenkins/ANT?

About your patch:
Classes passed to ExtendWith should not have a global state. You should store all state in the ExtensionContext.

Checking the annotations in the JOSM Test rules is prone to errors, because users always have to remember to add JOSMTestRules to the class. And you should get the annotation from Junit, not by hand. What you should do is something like:

public class OverrideAssumeRevisionExtension extends BeforeEachCallback {
  public void beforeEach(ExtensionContext context) {
    // Get annotation from context.getElement()
    // Then recurse to context.getParent() if not available
  }
}

@ExtendWith(OverrideAssumeRevisionExtension.class)
@Retention(RetentionPolicy.RUNTIME)
@Target(value={TYPE,METHOD})
@interface OverrideAssumeRevision{
int value();
}

in reply to:  124 ; comment:125 by taylor.smock, 3 years ago

Replying to michael2402:

@taylor Only eclipse? Or also Jenkins/ANT?

I've only run tests in eclipse so far. I'd expect at least one of them to pass properly in ant (which is why I called out Eclipse -- that specific test case failed with java.awt.HeadlessException, and I don't think I set eclipse up for headless tests).

About your patch:
Classes passed to ExtendWith should not have a global state. You should store all state in the ExtensionContext.

Checking the annotations in the JOSM Test rules is prone to errors, because users always have to remember to add JOSMTestRules to the class. And you should get the annotation from Junit, not by hand. What you should do is something like:

I thought the annotations having any effect were dependent upon users adding JOSMTestRules to the class (via @RegisterExtension).

public class OverrideAssumeRevisionExtension extends BeforeEachCallback {
  public void beforeEach(ExtensionContext context) {
    // Get annotation from context.getElement()
    // Then recurse to context.getParent() if not available
  }
}

@ExtendWith(OverrideAssumeRevisionExtension.class)
@Retention(RetentionPolicy.RUNTIME)
@Target(value={TYPE,METHOD})
@interface OverrideAssumeRevision{
int value();
}

I'll upload the changes I think you were trying to get at.

by taylor.smock, 3 years ago

Use @ExtendWith for Override annotations

in reply to:  125 comment:126 by michael2402, 3 years ago

Replying to taylor.smock:

I'll upload the changes I think you were trying to get at.

The JOSMTestRules stuff is Junit4. I used it as an an ugly hack because what we acutally need was not possible with Junit4. See #12949. I did some more refactoring so that test rules did not always include everything but only the parts needed in #12977 #13005 #13017 #13018 #12974 #13033 #13037 #13045 #13047 #13048

When moving to Junit 5, we should get rid of JOSMTestRules all together. We should create new annotations for all the things the test rules changed, e.g.:

@UseI18n("en")
@UseJosmHome()
@UsePreferences()
@UseApi(APIType)
@UsePlatform()
@UseProjection("EPSG:3857")

You can put @ExtendWith on the annotation. Junit searches recursively, so if you mark a test with @OverrideAssumeRevision, it will automatically apply the @ExtendWith. So for each annotation, you create your own extension. You can also add @ExtendWith twice to use other dependend annotations (in the order in which they should be loaded), e.g. if Preferences requires JosmHome, then use:

@ExtendWith(UseJosmHomeExtension.class)
@ExtendWith(UsePreferences.class)
…
@interface UsePreferences {…}

You can then even pass parameters into the tests. For example, this would then work:

@Test
public void myTest(MainLayerManager ml) {
}

If you need more help, we can have a hangout / other video call, most of this is easier explained that way.

comment:127 by taylor.smock, 3 years ago

OK. That makes a lot more sense than what I thought you were saying.

I've been trying to keep some backwards compatibility, so plugins with tests don't have to update from JUnit{3,4} to JUnit5, and then have to spend time figuring out how to fix their tests.

Splitting things out will definitely be clearer and better.

in reply to:  127 comment:128 by michael2402, 3 years ago

Replying to taylor.smock:

I've been trying to keep some backwards compatibility, so plugins with tests don't have to update from JUnit{3,4} to JUnit5, and then have to spend time figuring out how to fix their tests.

Keep the old JOSMTestRules, don't touch them. Keep the JOSMFixture, don't touch it. Mark them as deprecated

And then write your extensions from scratch. We will have duplicate code for some time, but we won't maintain the old one for long, so it is not an issue. That way, we can start with clean extensions and clean up all the tests, plugin can then update whenever they like.

by taylor.smock, 3 years ago

Initial extensions replacing JOSMTestRules#preferences and JOSMTestRules#assumeRevision (Override should probably be dropped from filenames)

by taylor.smock, 3 years ago

Modify two tests to use the annotations, add i18n, Main, ApiType, Presets, Projection, ProjectionNadGrids, and Territories. Territories will probably be modified so that there can be different initialization types (e.g., full initialization or partial initialization).

comment:129 by taylor.smock, 3 years ago

I probably won't get back to this for awhile (probably a month or so), so this post is largely to jog my memory. Or to help someone else if they decide to work on it in the meantime.

Unused annotations (with the two modified tests):

  • OsmApiType
  • AssumeRevision
  • I18n
  • Presets
  • ProjectionNadGrids
  • Territories

Behavior that I need to check:

  • What happens if i18n (for example) can be used on both methods and types, and the extension implements BeforeAllCallback and BeforeEachCallback. Is BeforeEachCallback called prior to every method (my suspicion), or is it just called prior to annotated methods (probably not). So I need to figure out a good way to check and see if the most recent annotation is for a method or a class, and then either run the BeforeEachCallback or not based off of that.

Use case: Test localization in English and Spanish in the same test class.

in reply to:  129 ; comment:130 by michael2402, 3 years ago

Replying to taylor.smock:

Behavior that I need to check:

  • What happens if i18n (for example) can be used on both methods and types, and the extension implements BeforeAllCallback and BeforeEachCallback. Is BeforeEachCallback called prior to every method (my suspicion), or is it just called prior to annotated methods (probably not). So I need to figure out a good way to check and see if the most recent annotation is for a method or a class, and then either run the BeforeEachCallback or not based off of that.

Use case: Test localization in English and Spanish in the same test class.

No problem there if you only use beforeEach and afterEach. You can use:

@I18n("en") // This will get extended to @ExtendWith(I18nExtension)
public class MyTest {
  // BeforeAllCallback would called before all tests with the context of the class => sees only @I18n("en")

  @Test
  public void testEn() {
    // This test is extended with I18nExtension
    // I18nExtension#beforeEach is called with the context of the method
    // I18nExtension looks for @I18n on the method (=> not found) and then on the class (=> en)
  }

  @Test
  @I18n("de") // This will get add another @ExtendWith(I18nExtension)
  public void testDe() {
    // This test is extended with I18nExtension
    // The extension is only applied once (Junit removes duplicates)
    // I18nExtension#beforeEach is called with the context of the method
    // I18nExtension looks for @I18n on the method (=> de)
  }
}

by taylor.smock, 3 years ago

Modify Territories.Extension so that there are various initialization states, and doesn't reinitialize when not needed

in reply to:  130 ; comment:131 by taylor.smock, 3 years ago

Replying to michael2402:

...

I was actually thinking more about items that really don't need to be initialized more than once (Territories is one such item), but may need to be overridden for whatever reason (maybe only one test needs the entirety of Territories to be initialized, but every other test in the class only needs the basic initialization done).

There are probably some other items that shouldn't be called more than once, but I don't know them off the top of my head.

in reply to:  131 comment:132 by michael2402, 3 years ago

Replying to taylor.smock:

There are probably some other items that shouldn't be called more than once, but I don't know them off the top of my head.

Then add a uninitalize to them.

Before the annotated test => initialize. After the annotated test => uninitialize.

That way, we can be sure that every test works in the environment it defines. Otherwise, we would e.g. have hard to track problems when the order of tests changes.

by taylor.smock, 3 years ago

Modify Territories to have an uninitialize function (java-docs indicate that calling it outside of a test environment is a very bad idea (tm)), modify several tests to use the extensions (largely focused on longer tests, most taking >10s on my machine), add a utils class to find appropriate annotations and reset static classes, add replacements for assertionsInEDT, http, https, a new specific annotation for DeleteCommandCallback, setting up and cleaning up the layer environment, timezone usage, annotations for integration and slow tests

comment:133 by michael2402, 3 years ago

@taylor.smock

TimeZoneAnnotation:

You should either mark this as only @Target({ElementType.TYPE}) to prevent use at the method level or you should use afterEach instead of afterAll.

When Extension Annotations are added at a method level, only the beforeEach and afterEach callbacks are executed, but the afterAll callback is never executed then (it is just silently ignored)

comment:134 by Don-vip, 3 years ago

In 17478/josm:

see #16567 - see #20433 - restore parallel execution of imagery integration test

comment:135 by Don-vip, 3 years ago

In 17531/josm:

see #16567 - consistency between extended source entry integration tests

comment:136 by Don-vip, 3 years ago

In 17532/josm:

see #16567 - ignore IAE that can happen when running concurrent tests

comment:137 by Don-vip, 3 years ago

In 17536/josm:

see #16567 - revert parallel execution of TaggingPresetPreferenceTestIT - huge instability

comment:138 by Don-vip, 3 years ago

In 17541/josm:

see #16567 - better synchronization of Preferences.settingsMap

comment:139 by simon04, 3 years ago

In 17677/josm:

see #16567 - JUnit 5: use org.junit.jupiter.api.Assumptions

comment:140 by Don-vip, 3 years ago

In 17966/josm:

see #16567 - hide stdout/stderr of unit tests from Ant output

comment:141 by Don-vip, 3 years ago

Keywords: junit5 added; junit removed

in reply to:  138 comment:142 by Don-vip, 2 years ago

Replying to Don-vip:

In 17541/josm:

see #16567 - better synchronization of Preferences.settingsMap

Ah, my bad, this is the cause of the deadlock in #21462

comment:143 by gaben, 2 years ago

Hmm, just found this ticket. I started fixing the remaining assertions/imports in tests, about 90 test files modified so far. I can provide a patch in a few days if needed.

in reply to:  143 comment:144 by taylor.smock, 2 years ago

Replying to gaben:

Hmm, just found this ticket. I started fixing the remaining assertions/imports in tests, about 90 test files modified so far. I can provide a patch in a few days if needed.

I think I've already done that. I just need to get all the tests working, so I should probably dust off that patch series. See #21139 (git diffs available at https://gitlab.com/smocktaylor/josm/-/merge_requests/8 ).

comment:145 by gaben, 2 years ago

Thanks, didn't know that. I changed org.junit.Assert to the new org.junit.jupiter.api.Assertions import path and fixed some minor code issues alongside. I see you changed some, but not all.

by gaben, 2 years ago

Replace Junit4 imports and assertions with the new Junit5 style code according to https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4-tips, plus minor simplifications

comment:146 by taylor.smock, 18 months ago

Just a heads up, it looks like JMockit isn't properly cleaning up mocks in JUnit4 tests when run with the vintage runner.

See https://josm.openstreetmap.de/jenkins/job/JOSM/jdk=JDK8/lastCompletedBuild/testReport/org.openstreetmap.josm.tools.bugreport/BugReportTest/testSuppressedExceptions_String__Consumer__2_/ .

I'm trying to figure out what the smallest patch is to fix the issue, and then I think I'll rebase the patch I have. I had been planning on waiting for a fix in ant before I picked this up again, but I think it might be better to move off of JUnit4 sooner rather than later.

@gaben: I'll go ahead and pull your patch into my branch, just so I can fix conflicts at the same time.

comment:147 by gaben, 12 months ago

@taylor, it seems your repo isn't available (at least publicly). Have you made progress since the last comment?

comment:148 by taylor.smock, 12 months ago

Funny story that...

GitLab decided to make it so OSS projects had to jump through hoops to have more than 5 GB of storage, and I had to clean up my personal space by a bit. So I cleaned out most of the "large" repos I had.

I thought I had a copy on my hard drive, but I wasn't able to find it. I should have forked it onto GitHub and then gone from there.

Anyway, one of the blockers was ant -- I fixed a bug in it (probably 6 months to a year ago) which was released a month or so ago.

I'll go ahead and merge your changes as soon as I track down why tests are suddenly failing (I suspect state pollution, and I fixed one bug I found while debugging in my IDE, which fixed the tests in my IDE but not via ant test).

If I forget, don't feel bad about pinging this ticket again.

comment:149 by taylor.smock, 12 months ago

In 18690/josm:

See #16567: Convert all assertion calls to JUnit 5 (patch by gaben, modified)

The modifications are as follows:

  • Merge DomainValidatorTest.testIDN and DomainValidatorTest.testIDNJava6OrLater
  • Update some tests to use @ParameterizedTest (DomainValidatorTest)
  • Replace various exception blocks with assertThrows. These typically looked like
        try {
            // Something that should throw an exception here
            fail("An exception should have been thrown");
        } catch (Exception e) {
            // Verify the exception matches expectations here
        }
    
  • Replace assertTrue(val instanceof Clazz) with assertInstanceOf
  • Replace JUnit 4 @Suite with JUnit 5 @Suite

Both the original patch and the modified patch fix various lint issues.

comment:150 by taylor.smock, 12 months ago

In 36063/osm:

See #16567: Convert all assertion calls to JUnit 5 (patch by gaben, modified)

The modifications are as follows:

  • Merge DomainValidatorTest.testIDN and DomainValidatorTest.testIDNJava6OrLater
  • Update some tests to use @ParameterizedTest (DomainValidatorTest)
  • Replace various exception blocks with assertThrows. These typically looked like
        try {
            // Something that should throw an exception here
            fail("An exception should have been thrown");
        } catch (Exception e) {
            // Verify the exception matches expectations here
        }
    
  • Replace assertTrue(val instanceof Clazz) with assertInstanceOf
  • Replace JUnit 4 @Suite with JUnit 5 @Suite

Both the original patch and the modified patch fix various lint issues.

comment:151 by gaben, 12 months ago

I see the ticket is still open, I'd take this opportunity to add another patch :)

  • test/functional/org/openstreetmap/josm/data/BoundariesTestIT.java

     
    2828    private static final List<String> RETIRED_ISO3166_1_CODES = Arrays.asList(
    2929            "AN", "BU", "CS", "NT", "TP", "YU", "ZR");
    3030
    31     private static final List<String> EXCEPTIONNALY_RESERVED_ISO3166_1_CODES = Arrays.asList(
     31    private static final List<String> EXCEPTIONALLY_RESERVED_ISO3166_1_CODES = Arrays.asList(
    3232            "AC", "CP", "DG", "EA", "EU", "EZ", "FX", "IC", "SU", "TA", "UK", "UN", "XK");
    3333
    3434    private static final List<String> ISO3166_2_CODES = Arrays.asList(
     
    7272            // Check for unknown ISO-3166-1 alpha 2 codes
    7373            for (OsmPrimitive p : tagged.stream().filter(SearchCompiler.compile("ISO3166-1\\:alpha2")).collect(Collectors.toList())) {
    7474                String code = p.get("ISO3166-1:alpha2");
    75                 assertTrue(iso31661a2.contains(code) || EXCEPTIONNALY_RESERVED_ISO3166_1_CODES.contains(code), code);
     75                assertTrue(iso31661a2.contains(code) || EXCEPTIONALLY_RESERVED_ISO3166_1_CODES.contains(code), code);
    7676            }
    7777            // Check presence of all ISO-3166-2 codes for USA, Canada, Australia (for speed limits)
    7878            for (String code : ISO3166_2_CODES) {

in reply to:  151 comment:152 by taylor.smock, 12 months ago

Replying to gaben:

I see the ticket is still open, I'd take this opportunity to add another patch :)

The following files still use JUnit 4. Some are for backwards compatibility reasons, but others are due to needing WireMock server support (note: wiremock now supports JUnit 5, so I should spend some time upgrading the rest of the tests). At that point, the only things which have JUnit 4 code should be compatibility code in testutils.

I'll do a pass for JUnit 4 tests in the plugins that we can maintain. External plugins are on their own.

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

comment:153 by taylor.smock, 12 months ago

In 18694/josm:

Fix #22381: Try to automatically install newly required plugins on plugin update

This additionally updates the plugin tests to JUnit 5 (see #16567). Some tests
required fixing (for example, MinimapDialogTest fails if the system of
measurement is metric).

comment:154 by taylor.smock, 12 months ago

In 36064/osm:

See #16567: Convert most plugin tests to JUnit 5

comment:155 by taylor.smock, 7 months ago

In 18798/josm:

See #16567: Add @Territories annotation for JUnit 5

This reduces memory allocations during test runs by ~20% since we are no longer
running Territories.initialize() on every test where JOSMTestRules.territories()
was called. This reduced overall test time by ~10%, mostly due to less time spent
in JVM internals.

comment:156 by taylor.smock, 5 months ago

In 18853/josm:

See #16567: Update to JUnit 5

This removes new JOSMTestRules() with no additional setup and most
JOSMFixture calls.

Removing the bare JOSMTestRules speeds up the test suite since there are two
fewer System.gc() calls per test.

by taylor.smock, 5 months ago

Attachment: 16567.2.patch added

More @Annotations, convert most tests to use them

comment:157 by taylor.smock, 5 months ago

In 18867/josm:

See #16567: Update to JUnit 5

This adds the @TaggingPresets annotation used by r18866 (see #23196). The
annotation tries to only initialize the presets when the current preset list does
not have the same hashCode as the default preset list. In order for this to work,
TaggingPresets#getTaggingPresets had to return a List (the method contract
has not changed), which calculates the hashCode in a deterministic manner.

comment:158 by taylor.smock, 5 months ago

In 18870/josm:

See #16567: Update to JUnit 5

This converts most tests to use @Annotations. There are also some performance
improvements as it relates to tests.

comment:159 by taylor.smock, 4 months ago

Resolution: fixed
Status: reopenedclosed

In 18893/josm:

Fix #16567: Upgrade to JUnit 5

JOSMTestRules and JOSMTestFixture can reset the default JOSM profile, which can
be unexpected for new contributors. This updates all tests to use JUnit 5 and
the new JUnit 5 annotations.

This also renames MapCSSStyleSourceFilterTest to MapCSSStyleSourceFilterPerformanceTest
to match the naming convention for performance tests and fixes some lint issues.

This was tested by running all tests individually and together.

comment:160 by taylor.smock, 4 months ago

In 36193/osm:

See #16567: Upgrade to JUnit 5

This updates plugins to use the JUnit 5 annotations

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.