Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years 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 3 years ago.
@BasicPreferences extension + porting of tests to use it
21064.2.patch (259.9 KB ) - added by taylor.smock 3 years ago.
Update for recent test changes
21064.NameFinderAnnotation.patch (610 bytes ) - added by taylor.smock 3 years ago.
NameFinder initializes NameFinder.NOMINATIM_URL_PROP before OsmDownloadReaderTest (depends upon ordering)
21064.BasicPreferencesConfigReset.patch (4.5 KB ) - added by taylor.smock 3 years ago.
Reset all static AbstractProperty preferences fields to the new Config.
21064.BasicPreferencesConfigReset.2.patch (4.8 KB ) - added by taylor.smock 3 years 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)

by taylor.smock, 3 years ago

Attachment: 21064.patch added

@BasicPreferences extension + porting of tests to use it

comment:1 by taylor.smock, 3 years ago

Description: modified (diff)

comment:2 by taylor.smock, 3 years ago

Two week patch ping

comment:3 by Don-vip, 3 years ago

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

comment:4 by Don-vip, 3 years ago

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

Component: CoreUnit tests

comment:6 by Don-vip, 3 years ago

Keywords: junit5 added; junit 5 removed

comment:7 by taylor.smock, 3 years ago

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.

by taylor.smock, 3 years ago

Attachment: 21064.2.patch added

Update for recent test changes

comment:8 by Don-vip, 3 years ago

Resolution: fixed
Status: assignedclosed

In 18037/josm:

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

comment:9 by Don-vip, 3 years ago

thanks!

comment:10 by Don-vip, 3 years ago

Resolution: fixed
Status: closedreopened

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

OverpassDownloadReaderTest is not happy about the change.

comment:11 by taylor.smock, 3 years ago

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

by taylor.smock, 3 years ago

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

comment:12 by taylor.smock, 3 years ago

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

by taylor.smock, 3 years ago

Reset all static AbstractProperty preferences fields to the new Config.

by taylor.smock, 3 years ago

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

comment:13 by Don-vip, 3 years ago

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

Resolution: fixed
Status: reopenedclosed

In 18041/josm:

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

in reply to:  13 comment:15 by taylor.smock, 3 years ago

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