[PATCH] Add JUnit 5 extension for preferences

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

• ​https://gitlab.com/smocktaylor/josm/-/merge_requests/7
• ​https://gitlab.com/smocktaylor/josm/-/merge_requests/7/pipelines

[taylor.smock]

@BasicPreferences extension + porting of tests to use it

[taylor.smock]

Two week patch ping

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

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

[taylor.smock]

Update for recent test changes

[Don-vip]

In 18037/josm:

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

[Don-vip]

thanks!

[Don-vip]

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

OverpassDownloadReaderTest is not happy about the change.

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

[taylor.smock]

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

[taylor.smock]

• attachment:21064.NameFinderAnnotation.patch​ just modifies NameFinderTest to have a @BasicPreferences annotation. While I don't think it is ideal, it does avoid the pitfalls of the other option.
• attachment:21064.BasicPreferencesConfigReset.patch​ iterates through all loaded classes (I make an assumption that we only have one classloader), and sets all AbstractProperty preferences fields to the new preferences. This uses reflection to fiddle with final fields, which has some pitfalls.
• attachment:21064.BasicPreferencesConfigReset.2.patch​ is the same as attachment:21064.BasicPreferencesConfigReset.patch​, except it additionally resets the preferences fields to null. This is better from a test isolation view, but I can almost guarantee that additional tests will fail.

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

[taylor.smock]

Reset all static AbstractProperty preferences fields to the new Config.

[taylor.smock]

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

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

[Don-vip]

In 18041/josm:

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

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