Modify

Opened 19 months ago

Last modified 9 months ago

#21139 new enhancement

[PATCH] BasicPreferences Test Isolation

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: Longterm
Component: Unit tests Version:
Keywords: junit5 Cc:

Description (last modified by 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.

Attachments (3)

21139.patch (4.5 KB) - added by taylor.smock 19 months ago.
Reset all static AbstractProperty preferences fields to the new Config.
21139.2.patch (4.8 KB) - added by taylor.smock 19 months ago.
Additionally reset static AbstractProperty preference fields to null after each test (this may cause other test failures)
21139.4.patch (272.4 KB) - added by taylor.smock 17 months ago.

Download all attachments as: .zip

Change History (27)

Changed 19 months ago by taylor.smock

Attachment: 21139.patch added

Reset all static AbstractProperty preferences fields to the new Config.

Changed 19 months ago by taylor.smock

Attachment: 21139.2.patch added

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

comment:1 Changed 19 months ago by taylor.smock

Description: modified (diff)

comment:2 Changed 19 months ago by taylor.smock

Component: CoreUnit tests
Keywords: junit5 added
Type: defectenhancement

comment:3 Changed 19 months ago by 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?

comment:4 Changed 19 months ago by Don-vip

Milestone: 21.07

comment:5 in reply to:  3 ; Changed 19 months ago by 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

comment:6 in reply to:  5 Changed 19 months ago by 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.

comment:7 Changed 19 months ago by 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.

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

comment:8 Changed 19 months ago by 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

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

comment:9 Changed 18 months ago by 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.

comment:10 Changed 18 months ago by 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.

comment:11 Changed 18 months ago by Don-vip

Milestone: 21.0721.08

OK, thanks.

comment:12 Changed 17 months ago by Don-vip

Milestone: 21.0821.09

Changed 17 months ago by taylor.smock

Attachment: 21139.4.patch added

comment:13 Changed 17 months ago by 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. :)

EDIT: And failed. :(
On the plus side, ImageryPreferenceTestIT appears to work. And that was my primary worry about having to debug integration tests (the test run directly prior had the @BasicPreferences annotation).

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

comment:14 Changed 16 months ago by Don-vip

Milestone: 21.0921.10

Milestone renamed

comment:15 Changed 16 months ago by Don-vip

Milestone: 21.1021.11

comment:16 Changed 14 months ago by Don-vip

Milestone: 21.1121.12

Milestone renamed

comment:17 Changed 14 months ago by Don-vip

Milestone: 21.1222.01

comment:18 Changed 12 months ago by stoecker

Milestone: 22.0122.02

Milestone renamed

comment:19 Changed 12 months ago by stoecker

@taylor: You can handle this yourself?

comment:20 in reply to:  19 Changed 12 months ago by 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).

comment:21 Changed 12 months ago by Don-vip

Milestone: 22.0222.03

comment:22 Changed 10 months ago by stoecker

Milestone: 22.0322.04

comment:23 Changed 9 months ago by stoecker

Milestone: 22.0422.05

Milestone renamed

comment:24 Changed 9 months ago by taylor.smock

Milestone: 22.05Longterm

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to taylor.smock
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


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

 
Note: See TracTickets for help on using tickets.