Modify

Opened 3 months ago

Last modified 26 hours ago

#21139 new enhancement

[PATCH] BasicPreferences Test Isolation

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 21.11
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 3 months ago.
Reset all static AbstractProperty preferences fields to the new Config.
21139.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)
21139.4.patch (272.4 KB) - added by taylor.smock 6 weeks ago.

Download all attachments as: .zip

Change History (18)

Changed 3 months ago by taylor.smock

Attachment: 21139.patch added

Reset all static AbstractProperty preferences fields to the new Config.

Changed 3 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 3 months ago by taylor.smock

Description: modified (diff)

comment:2 Changed 3 months ago by taylor.smock

Component: CoreUnit tests
Keywords: junit5 added
Type: defectenhancement

comment:3 Changed 3 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 3 months ago by Don-vip

Milestone: 21.07

comment:5 in reply to:  3 ; Changed 3 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 3 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 3 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 3 months ago by taylor.smock (previous) (diff)

comment:8 Changed 3 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 3 months ago by taylor.smock (previous) (diff)

comment:9 Changed 3 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 3 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 3 months ago by Don-vip

Milestone: 21.0721.08

OK, thanks.

comment:12 Changed 7 weeks ago by Don-vip

Milestone: 21.0821.09

Changed 6 weeks ago by taylor.smock

Attachment: 21139.4.patch added

comment:13 Changed 6 weeks 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 6 weeks ago by taylor.smock (previous) (diff)

comment:14 Changed 9 days ago by Don-vip

Milestone: 21.0921.10

Milestone renamed

comment:15 Changed 26 hours ago by Don-vip

Milestone: 21.1021.11

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.