Modify

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#21064 closed enhancement (fixed)

[PATCH] Add JUnit 5 extension for preferences

Reported by: taylor.smock Owned by: Don-vip
Priority: normal Milestone: 21.07
Component: Unit tests Version:
Keywords: junit5 Cc:

Description (last modified by taylor.smock)

#16567 is getting a bit big, so I'm going to make separate tickets for different patches/functionality.

The attached patch does the following:

JUnit5 Extension: Add utilities for basic preferences


Also, move many tests that rely solely on

  • JOSMTestRules#preferences
  • JOSMTestRules#timeout
  • JOSMTestRules#i18n
  • JOSMTestRules with no further calls

See:

Attachments (5)

21064.patch (256.6 KB) - added by taylor.smock 4 months ago.
@BasicPreferences extension + porting of tests to use it
21064.2.patch (259.9 KB) - added by taylor.smock 3 months ago.
Update for recent test changes
21064.NameFinderAnnotation.patch (610 bytes) - added by taylor.smock 3 months ago.
NameFinder initializes NameFinder.NOMINATIM_URL_PROP before OsmDownloadReaderTest (depends upon ordering)
21064.BasicPreferencesConfigReset.patch (4.5 KB) - added by taylor.smock 3 months ago.
Reset all static AbstractProperty preferences fields to the new Config.
21064.BasicPreferencesConfigReset.2.patch (4.8 KB) - added by taylor.smock 3 months ago.
Additionally reset static AbstractProperty preference fields to null after each test (this may cause other test failures)

Download all attachments as: .zip

Change History (20)

Changed 4 months ago by taylor.smock

Attachment: 21064.patch added

@BasicPreferences extension + porting of tests to use it

comment:1 Changed 4 months ago by taylor.smock

Description: modified (diff)

comment:2 Changed 3 months ago by taylor.smock

Two week patch ping

comment:3 Changed 3 months ago by Don-vip

Milestone: 21.07
Owner: changed from team to Don-vip
Status: newassigned

comment:4 Changed 3 months ago by Don-vip

Can you please update the patch? I've changed some tests in the past two weeks. I'll apply it first thing tomorrow morning.

comment:5 Changed 3 months ago by Don-vip

Component: CoreUnit tests

comment:6 Changed 3 months ago by Don-vip

Keywords: junit5 added; junit 5 removed

comment:7 Changed 3 months ago by taylor.smock

I just did that (on GitLab). Hopefully the CI test stage doesn't time out again (just so that we have a confirmation from a "clean" environment that it doesn't break anything). I'll also upload an updated patch here.

Changed 3 months ago by taylor.smock

Attachment: 21064.2.patch added

Update for recent test changes

comment:8 Changed 3 months ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 18037/josm:

fix #21064 - Add JUnit 5 extension for preferences (patch by taylor.smock)

comment:9 Changed 3 months ago by Don-vip

thanks!

comment:10 Changed 3 months ago by Don-vip

Resolution: fixed
Status: closedreopened

https://github.com/openstreetmap/josm/runs/3084630956?check_suite_focus=true

OverpassDownloadReaderTest is not happy about the change.

comment:11 Changed 3 months ago by taylor.smock

Awesome. That is one of the tests I didn't modify, since it is still on JUnit4.

It does look like an ordering issue. What is happening is NameFinder.NOMINATIM_URL_PROP is getting initialized elsewhere, and since AbstractProperty sets the preference variable on first load, we have a null preference stored, even when preferences are initialized.

The previous behavior of JOSMTestRules was to avoid cleaning up the preferences class, so this was never a real issue.

Solutions:

  • Add @BasicPreferences annotation to NameFinderTest. This is easy, but it doesn't help us keep tests isolated, and this may crop up again in the future.
  • Modify the @BasicPreferences annotation to crawl through loaded classes, looking for static and static final AbstractProperties, and use reflection to set the preferences field to the current Config.getPref() variable. This is harder, may be significantly slower (but may not be as well -- @BasicPreferences typically only runs once per test class).

Changed 3 months ago by taylor.smock

NameFinder initializes NameFinder.NOMINATIM_URL_PROP before OsmDownloadReaderTest (depends upon ordering)

comment:12 Changed 3 months ago by taylor.smock

I'm running a test run with attachment:21064.BasicPreferencesConfigReset.2.patch right now, but it is going to take a little while.

Changed 3 months ago by taylor.smock

Reset all static AbstractProperty preferences fields to the new Config.

Changed 3 months ago by taylor.smock

Additionally reset static AbstractProperty preference fields to null after each test (this may cause other test failures)

comment:13 Changed 3 months ago by Don-vip

Thanks! For this ticket I'll stick with the simple solution, even if not ideal. Isolating unit tests is a long-term task.

comment:14 Changed 3 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 18041/josm:

fix #21064 - NameFinder initializes NameFinder.NOMINATIM_URL_PROP before OsmDownloadReaderTest (depends upon ordering) (patch by taylor.smock)

comment:15 in reply to:  13 Changed 3 months ago by taylor.smock

Replying to Don-vip:

Thanks! For this ticket I'll stick with the simple solution, even if not ideal. Isolating unit tests is a long-term task.

I'll open a new ticket with both of the BasicPreferencesConfigReset patches (see #21139). That way they aren't forgotten. :)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.