[PATCH] BasicPreferences Test Isolation

[taylor.smock]

This is a followup of comment:13:ticket:21064 (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.

As in comment:12:ticket:21064, the two patches attached to this ticket do the following:

• attachment:21139.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:21139.2.patch​​ is the same as attachment:21139.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.

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

This is likely to break some tests, can you please create a PR to ​https://github.com/openstreetmap/josm/pulls to see how the test suite behaves?

[taylor.smock]

Replying to Don-vip:

This is likely to break some tests, can you please create a PR to ​https://github.com/openstreetmap/josm/pulls to see how the test suite behaves?

I've opened ​https://github.com/openstreetmap/josm/pull/72. Note that the workflow hasn't started yet ("First-time contributors need a maintainer to approve running workflows.") I've attached links to the two commits in the fork repo, where the workflow has started.

​https://github.com/tsmock/josm/commits/josm-21139-test-isolation

• attachment:21139.patch​: ​https://github.com/tsmock/josm/commit/6530ad5b8628bde02ec008668dd4c26e87fd4ead
• attachment:21139.2.patch​: ​https://github.com/tsmock/josm/commit/eeaac2b9500850458625aa082cef2ba6fc49f307

[Don-vip]

Replying to taylor.smock:

I've opened ​https://github.com/openstreetmap/josm/pull/72. Note that the workflow hasn't started yet ("First-time contributors need a maintainer to approve running workflows.")

Nice security feature, I like it.

[taylor.smock]

ell, it looks like it failed (Java 11):

java.lang.NoSuchFieldException: classes
	at java.base/java.lang.Class.getDeclaredField(Class.java:2549)
	at org.openstreetmap.josm.testutils.annotations.BasicPreferences$BasicPreferencesExtension.resetConfigVariables(BasicPreferences.java:92)
	at org.openstreetmap.josm.testutils.annotations.BasicPreferences$BasicPreferencesExtension.beforeAll(BasicPreferences.java:76)
	at org.openstreetmap.josm.testutils.annotations.BasicPreferences$BasicPreferencesExtension.beforeEach(BasicPreferences.java:82)

Specific code: final Field classField = ClassLoader.class.getDeclaredField("classes");. I'm not certain why this is failing -- Java 11 appears to have that field in ClassLoader.java.

[taylor.smock]

Well, I've got it mostly working. There is one test that just doesn't seem to like JUnit5 though (MinimapDialogTest.java, see ​86565c689dacd523e18891a95e5ac8647b0b3496 for Assert -> Assertions changes -- the test is still run under JUnit4, as there are issues running it under JUnit5).

I'll work on getting some other annotations pushed so I can try it with only JUnit annotations

Current annotations in my working directory (relevant for MinimapDialogTest):

• @Main
  • Needs @HTTP (otherwise MOTD throws -- "HTTP factory has not been set")
• @Projection
• @FakeImagery
  • Needs a @Wiremock rule, which I haven't written yet, probably why it fails under JUnit5 in the first place

EDIT:
@Wiremock and @HTTP are part of the patch for #21150

[Don-vip]

I've merged #21150 (thanks!). Can you please rebase ​https://github.com/openstreetmap/josm/pull/72? I'll approve the Github actions run.

[taylor.smock]

I've gone ahead and rebased, but I do have to implement the @FakeImagery annotation as noted above, so don't bother starting a Github actions run immediately (it will probably fail). I may have to add @Main and @Projection as well, but those are already written, although I might need to make some changes.

Additional notes:
I've rebased onto my gitlab-ci branch -- I'll rebase off it once everything is passing.

[Don-vip]

OK, thanks.

[taylor.smock]

I've rebased off of the gitlab-ci branch (all unit/functional tests were passing except for those dependent upon platform pixel colouring in MapCSSRendererTest).

My current test run includes integration tests, and I'm waiting for that to finish. Hopefully it is done some time tomorrow. :)

[Don-vip]

Milestone renamed

[Don-vip]

Milestone renamed

[stoecker]

Milestone renamed

[stoecker]

@taylor: You can handle this yourself?

[taylor.smock]

Replying to stoecker:

@taylor: You can handle this yourself?

Yes. I've got it mostly working -- there are a few tests that are failing.

Something I did discover is that the ant JUnit5 xml formatter doesn't escape ]]> in CDATA (see ​https://github.com/apache/ant/pull/175 -- it is about due for a "was this forgotten?" ping). I want to say Jenkins uses the xml output, but I'm not 100% certain on that.

Other than a few failing tests, its pretty much done, and I haven't gotten around to troubleshooting said failing tests just so I can double-check that the CDATA is properly written with the next ant version (the ant tests for junitlauncher don't appear to be run in their CI).

[stoecker]

Milestone renamed

[taylor.smock]

I'm waiting on ant to release 1.10.13 which will fix an issue where xml files are improperly written.

[gaben]

Replying to taylor.smock:

I'm waiting on ant to release 1.10.13 which will fix an issue where xml files are improperly written.

Ant 1.10.13 has been released on 10th January 2023. ​https://ant.apache.org/bindownload.cgi

[taylor.smock]

I know. I've been slowly introducing the annotations from my patch as I make other test modifications.

With that said, one of the major issues is related to the @BasicPreferences annotation -- there are classes that store the current preferences object in a final field, so what I'm currently looking at doing (instead of trying to use reflection to work around it as I did in the original patch) is creating a class that delegates everything to the primary preferences instance (also static final), but has a check prior to calling the preferences instance.

It does require jmockit in order to change the return of the Preferences.main() method call, but I think that it will be more maintainable long-term.