Modify

Opened 3 years ago

Last modified 16 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 3 years ago.
Reset all static AbstractProperty preferences fields to the new Config.
21139.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)
21139.4.patch (272.4 KB ) - added by taylor.smock 3 years ago.

Download all attachments as: .zip

Change History (29)

by taylor.smock, 3 years ago

Attachment: 21139.patch added

Reset all static AbstractProperty preferences fields to the new Config.

by taylor.smock, 3 years ago

Attachment: 21139.2.patch added

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

comment:1 by taylor.smock, 3 years ago

Description: modified (diff)

comment:2 by taylor.smock, 3 years ago

Component: CoreUnit tests
Keywords: junit5 added
Type: defectenhancement

comment:3 by Don-vip, 3 years ago

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

Milestone: 21.07

in reply to:  3 ; comment:5 by taylor.smock, 3 years ago

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

in reply to:  5 comment:6 by Don-vip, 3 years ago

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 by taylor.smock, 3 years ago

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 years ago by taylor.smock (previous) (diff)

comment:8 by taylor.smock, 3 years ago

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 years ago by taylor.smock (previous) (diff)

comment:9 by Don-vip, 3 years ago

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 by taylor.smock, 3 years ago

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

Milestone: 21.0721.08

OK, thanks.

comment:12 by Don-vip, 3 years ago

Milestone: 21.0821.09

by taylor.smock, 3 years ago

Attachment: 21139.4.patch added

comment:13 by taylor.smock, 3 years ago

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 3 years ago by taylor.smock (previous) (diff)

comment:14 by Don-vip, 3 years ago

Milestone: 21.0921.10

Milestone renamed

comment:15 by Don-vip, 3 years ago

Milestone: 21.1021.11

comment:16 by Don-vip, 2 years ago

Milestone: 21.1121.12

Milestone renamed

comment:17 by Don-vip, 2 years ago

Milestone: 21.1222.01

comment:18 by stoecker, 2 years ago

Milestone: 22.0122.02

Milestone renamed

comment:19 by stoecker, 2 years ago

@taylor: You can handle this yourself?

in reply to:  19 comment:20 by taylor.smock, 2 years ago

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

Milestone: 22.0222.03

comment:22 by stoecker, 2 years ago

Milestone: 22.0322.04

comment:23 by stoecker, 2 years ago

Milestone: 22.0422.05

Milestone renamed

comment:24 by taylor.smock, 2 years ago

Milestone: 22.05Longterm

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

in reply to:  24 comment:25 by gaben, 16 months ago

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

comment:26 by taylor.smock, 16 months ago

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.

Modify Ticket

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

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.