Modify

Opened 20 months ago

Closed 15 months ago

Last modified 13 months ago

#16010 closed enhancement (fixed)

Tests: use JMockit to enable more extensive test coverage

Reported by: ris Owned by: team
Priority: normal Milestone: 18.07
Component: Unit tests Version:
Keywords: mock jmockit window extendeddialog Cc:

Description

Here's something I've been working on...

There are a number of places in the tests where our lack of ability to mock out deeply embedded code feels like it has stunted the growth of the test suite. An example of this is our avoidance of anything that creates a Window because its constructor will raise a HeadlessException when the tests are run in headless mode. This also means that anything inheriting from JDialog can't really be tested - and that's a shame, because there are an awful lot of dialog-flows in JOSM and from what I can tell, there aren't really test for any of them. On the other hand, the headless restriction has caused the growth in a number of places around the code of "alternate flows" for "test mode", which usually enable themselves by detecting isHeadless. The presence of these alternate flows makes the regular-mode behaviour of the code a bit harder to understand and it's not always particularly easy to piece together where all of these magic only-for-test-mode switches are and what their combined effect is.

Also, headless mode is not totally synonymous with "this is a test". Developers run tests in non-headless mode and there (hypothetically at least?) could be a non-test use for some JOSM code in a headless context. On top of that, a test being run in non-headless mode doesn't automatically mean that no window-related mocking is required - generally though Windows are available we often don't create them as that can add some unwanted complexity. (I refer to this as Windowless mode in my code)

I've had a little bit of experience working with mocking libraries in java and so I decided to dig into this path. There are a few different java mocking libraries, and they all seem to work in slightly different ways. I've personally had most success with JMockit's MockUp class, which is a class that intercepts things at a class loader level in java. What it essentially allows the user to do is to temporarily, at runtime, globally, switch out the implementation of an arbitrary class's methods. The original implementations are replaced automatically at the end of the JUnit test ends.

In the posted patch set, one of the first things I've done is replace a couple of JOSM's isHeadless() switches with mock methods that get switched out during JOSMTestRules initialization. The advantage of this is that the mock behaviour it creates will apply in headless and non-headless mode. I take advantage of this by re-adding an old bugbear test of mine which previously failed in non-headless mode.

Then I go on and develop a mechanism for allowing ExtendedDialog-driven flows to be tested. The nastiest part of this is where I actually patch out the constructor of java.awt.Window. It's considered bad practise to mock out code you "don't own", but really, the isHeadless() check existing in Window's constructor, I don't see any other possible way a Window-derived class could ever be constructed in headless mode. Now this does mean that we shouldn't expect these partially uninitialized Windows to behave perfectly correctly, but fortunately, often we don't need them to. Most uses of ExtendedDialog only interact with the instance on a very simple level, not actually relying on any of its window-like behaviour.

And then to demo this I add a test exercising the updatePlugins flow, overriding the "your JOSM version is too low" warnings. And I also fix a bug I noticed in JOSMTestRules whereby the effects of switching to a new-mock-home-directory per-test never take hold.

There are a couple of downsides to using a mocking library like JMockit, the main one is the the mocking is not thread-safe and so this could limit our ability to execute tests in parallel in future. However, from what I can see, we are a long way off from that ability right now and there are also alternatives to thread-based parallel tests (process based parallel tests perhaps?).

And now I've written an essay it would seem. Will post the patches (based on r13450), but the changes are easily browsable in the following PR: https://github.com/openstreetmap/josm/pull/24

I may think of more things to say, but I've got to go out now.

Attachments (39)

v1-0001-unit-tests-replace-isHeadless-checks-in-Navigatab.patch (958.1 KB) - added by ris 20 months ago.
v1-0002-MinimapDialogTest-add-tests-covering-show-downloa.patch (18.1 KB) - added by ris 20 months ago.
v1-0003-add-ExtendedDialogMocker-WindowMocker.patch (8.9 KB) - added by ris 20 months ago.
v1-0004-JOSMTestRules-clear-JosmBaseDirectories-memo-ized.patch (3.0 KB) - added by ris 20 months ago.
v1-0005-tests-add-PluginHandlerUpdatePluginsTest.patch (10.0 KB) - added by ris 20 months ago.
v2-0001-unit-tests-replace-isHeadless-checks-in-Navigatab.patch (959.1 KB) - added by ris 15 months ago.
v2-0002-MinimapDialogTest-add-tests-covering-show-downloa.patch (18.1 KB) - added by ris 15 months ago.
v2-0003-testing-add-ExtendedDialogMocker-WindowMocker.patch (11.4 KB) - added by ris 15 months ago.
v2-0004-testing-add-JOptionPaneSimpleMocker.patch (12.7 KB) - added by ris 15 months ago.
v2-0005-testing-add-HelpAwareOptionPaneMocker.patch (7.8 KB) - added by ris 15 months ago.
v2-0006-JOSMTestRules-clear-JosmBaseDirectories-memo-ized.patch (3.0 KB) - added by ris 15 months ago.
v2-0007-JOSMTestRules-add-EDTAssertionMocker-and-.asserti.patch (5.3 KB) - added by ris 15 months ago.
v2-0008-JOSMTestRules-attempt-to-close-all-windows-left-o.patch (1.9 KB) - added by ris 15 months ago.
v2-0009-AsynchronousUploadPrimitivesTaskTest-convert-to-u.patch (2.1 KB) - added by ris 15 months ago.
v2-0010-ExportProfileActionTest-convert-to-use-of-JOSMTes.patch (2.3 KB) - added by ris 15 months ago.
v2-0011-PreferencesTableTest-convert-to-use-of-JOSMTestRu.patch (3.0 KB) - added by ris 15 months ago.
v2-0012-PluginPreferenceTest-convert-to-use-of-HelpAwareO.patch (2.7 KB) - added by ris 15 months ago.
v2-0013-JOSMTestRules-don-t-use-setProperty-to-unreliably.patch (1.3 KB) - added by ris 15 months ago.
v2-0014-remove-some-isHeadless-checks-which-were-preventi.patch (7.7 KB) - added by ris 15 months ago.
v2-0015-unit-test-headlessness-allow-this-to-be-set-throu.patch (2.5 KB) - added by ris 15 months ago.
v2-0016-logging-add-ReacquiringConsoleHandler-which-shoul.patch (6.7 KB) - added by ris 15 months ago.
v2-0017-testutils-add-PluginServer.patch (9.9 KB) - added by ris 15 months ago.
v2-0018-TestUtils-add-assertFileContentsEqual-use-in-Plug.patch (5.0 KB) - added by ris 15 months ago.
v2-0019-tests-add-PluginHandlerJOSMTooOldTest.patch (27.0 KB) - added by ris 15 months ago.
v2-0020-PluginPreferenceTest-add-.platform-option-to-JOSM.patch (1.3 KB) - added by ris 15 months ago.
v2-0021-JOSMTestRules-preferences-handling-clear-defaults.patch (3.0 KB) - added by ris 15 months ago.
v2-0022-plugin-preferences-small-additions-to-ease-testin.patch (4.3 KB) - added by ris 15 months ago.
v2-0023-JOSMTestRules-synchronize-worker-thread-after-tes.patch (2.1 KB) - added by ris 15 months ago.
v2-0024-ProgressMonitorExecutor-log-exceptions-raised-by-.patch (2.1 KB) - added by ris 15 months ago.
v2-0025-MainApplication-shutdown-don-t-use-isHeadless-to-.patch (4.8 KB) - added by ris 15 months ago.
v2-0026-TestUtils-add-syncEDTAndWorkerThreads.patch (3.1 KB) - added by ris 15 months ago.
v2-0027-ExtendedDialogMocker-HelpAwareOptionPaneMocker-JO.patch (17.8 KB) - added by ris 15 months ago.
v2-0028-tests-add-PluginPreferenceHighLevelTest.patch (37.8 KB) - added by ris 15 months ago.
v1-0001-HelpAwareOptionPaneMocker-remove-idea-of-result-o.patch (3.1 KB) - added by ris 15 months ago.
v1-0002-tests-DownloadAlongTrackActionTest-mock-out-HelpA.patch (3.4 KB) - added by ris 15 months ago.
v1-0001-ReacquiringConsoleHandler-synchronize-reacquireOu.patch (1.0 KB) - added by ris 15 months ago.
v1-0001-TileSourceRule-add-getSourcesList-accessor-method.patch (1.3 KB) - added by ris 15 months ago.
v1-0002-DownloadWmsAlongTrackActionTest-use-TileSourceRul.patch (2.2 KB) - added by ris 15 months ago.
v1-0001-DownloadWmsAlongTrackActionTest-use-Awaitility-to.patch (2.2 KB) - added by ris 15 months ago.

Change History (106)

comment:1 Changed 20 months ago by Don-vip

Milestone: 18.03

Great! :) Did you try Mockito? It's used a lot and actively maintained, I never heared about JMockit before.

comment:2 Changed 20 months ago by ris

I used to use Mockito a while back but it isn't really designed to do in-place, global mocking. It's fine if you are able to instantiate all the objects you're using yourself for each test, but I'm trying to solve the problem of mocking classes whose instances are constructed in deeply embedded places or are globals. I switched to jmockit as it is far more capable and also, in my opinion at least, feels a lot less weird to work with.

comment:3 Changed 20 months ago by ris

Also note I'm experimenting with a patch (on top of this patch queue) in the PR to get travis running tests in non-headless mode too (using xvfb) so you may see travis going a bit haywire on there now and then.

comment:4 Changed 20 months ago by ris

That's interesting. The call System.setProperty("java.awt.headless", "true"); at the beginning of JOSMTestRules#before() turns out to be very unpredictable in whether it has any effect or not. It seems to depend on whether any of the static initializers in the classes loaded before its execution have caused the toolkit to already have been created.

However, if we remove it, every invocation of JOptionPanel will block execution and make the test run hang in non-headless mode. Which is fine - it's possible to mock out JOptionPanel in a way similar to ExtendedDialog, but it'll just need a bit more work before we had a non-hanging-under-non-headless-mode suite again. Either way, it wouldn't seem like a good thing to just continue using this odd semi-headless mode...

comment:5 Changed 19 months ago by Don-vip

Milestone: 18.0318.04

comment:6 Changed 19 months ago by ris

There's lots of progress on this - I just haven't pushed/posted my branch for a while as it's in quite a messy state.

comment:7 Changed 19 months ago by Don-vip

OK :) I didn't have enough time to review changes yet, that's why I pushed the milestone. Do you want me to wait for further progress on your side?

comment:8 Changed 19 months ago by ris

Yeah I've got a whole bunch of tidying going on - and then I'd want to have a go-through of the tests checking for ones that are now failing due to my changes.

Among the plus sides, I should have travis building the linux version with 3 different JREs, each in headless and headed (through xvfb) modes, and 1 macos build (headless). One day I'll look at appveyor and if needs be, simply find the offending crashy test and "skip" it somehow.

And then there's a bit of discussion about future directions I'd recommend (e.g. attempt to purge ourselves from use of isHeadless() switches in production code).

comment:9 in reply to:  8 Changed 19 months ago by Don-vip

Replying to ris:

purge ourselves from use of isHeadless() switches in production code.

This would be great, indeed :)

comment:10 Changed 19 months ago by ris

I think I've got the current PR https://github.com/openstreetmap/josm/pull/24 to a good state where the same number of tests pass/fail pre and post-patch and the patch series is now quite clean and logical, so now it's probably time to explain some of it. Note that github lists the changesets in a weird order based on their original commit dates which don't really make any sense after I've done reordering and rebasing of them, but whatever...

To get this to a point where the tests didn't "hang" (waiting for input) in either headed or headless modes I had to add three dialog "mocker" classes that allow dialog-driven flows to be tested. ExtendedDialogMocker, HelpAwareOptionPaneMocker and JOptionPaneSimpleMocker. The latter is named a "Simple" mocker as it doesn't attempt to handle the more complex showOptionDialog calls, but can also cope with ConditionalOptionPaneUtil-based dialogs. I've tried to follow similar API conventions for each mocker as much as possible though in reality the subtle differences between the semantics of the different "show dialog" calls limited the amount that made sense. Each one also keeps an invocationLog which can be used to verify the expected dialogs were "shown" after the code under test has been called. The implementations split the functionality into many protected methods, hopefully allowing for small details of their behaviour to be customized easily in subclasses without having to completely hard-code responses or re-implement complicated checking code.

There are also changes/additions to JOSMTestRules. After a test, it now tries to close (and dispose of) any windows that were left open. Otherwise these stay present in the event loop making debugging more unclear, using memory and possibly interfering with subsequent tests. Added a new .assertionsInEDT() method which uses EDTAssertionMocker to patch GuiHelper.runInEDTAndWait(...) to propagate AssertionErrors instead of the usual behaviour of simply printing the exception and continuing. This is needed if any of the above mentioned dialog mockers are called in the EDT and we want any of their assertions to actually take force. Arguably this should be the default in test mode and there should instead be a .noAssertionsInEDT() method, but that's a topic for another day.

Probably the biggest change is the simple removal of the System.setProperty("java.awt.headless", "true"); line at the top of begin() which will (sometimes, sort of) convince a non-headless test run that it is indeed headless and thus take isHeadless() branches. This is the change that required the addition of the dialog-mocking classes.

Note tests are all "passing" on travis (not that that actually means they're all passing, but we'll get there one day)

comment:11 Changed 19 months ago by ris

(I've also separated/re-ordered the changesets so that there shouldn't be any intermediate points which don't work - i.e. it should be fully possible to bisect into this series and pin down a change that caused a breakage)

comment:12 Changed 19 months ago by ris

I've also just pushed a commit on top that fixes test log output - previously only the first test of a run would get working log output in its report.

comment:13 Changed 18 months ago by Don-vip

Milestone: 18.0418.05

Thanks for the heads up :)

comment:14 Changed 17 months ago by ris

Starting to get a bit nervous about the size of the delta between my branch & master...

comment:15 Changed 17 months ago by Don-vip

Milestone: 18.0518.06

comment:16 Changed 16 months ago by Don-vip

Milestone: 18.0618.07

comment:17 Changed 16 months ago by ris

(sigh)

comment:18 Changed 16 months ago by Don-vip

Just to be sure, you're not waiting for me?

comment:19 Changed 16 months ago by ris

I'm waiting for anyone, because I'm feeling less and less confident developing on top of this branch as the bulk of it probably isn't applicable without the core mocking improvements.

comment:20 Changed 15 months ago by ris

Just did a rebase against master and pushed all the work I currently have on this branch up to the PR. It's in a (mostly) coherent state for review right now.

comment:21 Changed 15 months ago by Don-vip

Done :) Thanks!

comment:22 Changed 15 months ago by ris

Thanks - addressed. The final appveyor-related commit should probably be disregarded - reliable appveyor is still WIP

comment:23 Changed 15 months ago by Don-vip

OK. You can attach the patches when you judge it's ready and I'll apply them.

comment:24 Changed 15 months ago by ris

I'm going to attach these in git patch format (only way to include binary changes from what I can see), but it should be possible to apply them to any source tree using the git apply command. i.e. even in a svn repository.

comment:25 Changed 15 months ago by Don-vip

In PluginPreferenceHighLevelTest.testInstallWithoutRestartRequired(), pluginHandlerMocker is not used. Bug?

comment:26 Changed 15 months ago by Don-vip

In 14052/josm:

see #16010 - use JMockit to enable more extensive test coverage (patch by ris, modified)

see https://github.com/openstreetmap/josm/pull/24/commits for details

comment:27 Changed 15 months ago by Don-vip

In 14053/josm:

see #16010 - fix eclipse classpath

comment:28 Changed 15 months ago by ris

pluginHandlerMocker is not used

Not quite, but in this case I probably didn't need to bother assigning it to a variable, because I verified it being called through the captured loadPluginsCalled instead.

comment:29 Changed 15 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 14054/josm:

fix #16010 - fix java warning

comment:30 Changed 15 months ago by Don-vip

It seems one test is failing because of this. Can you please take a look?

comment:31 Changed 15 months ago by Don-vip

There is also a Findbugs warning:

IS: Inconsistent synchronization of org.openstreetmap.josm.tools.Logging$ReacquiringConsoleHandler.outputStreamMemo; locked 50% of time

comment:32 Changed 15 months ago by ris

Looking....

(also would be nice to reduce the amount of log spam for each test about keystroke assignments etc...)

comment:33 Changed 15 months ago by ris

Added a pair of patches which should fix this.

I find it tricky to catch these single regressions as I've almost *never* had a 100% passing test run on my local machine - not even in headless mode. There must be some magic going on in the jenkins machine. Not even the travis environment typically passes at 100%. Curiously, this particular test was still passing on travis.

comment:34 Changed 15 months ago by ris

...and there's a patch for fixing the findbugs issue.

comment:35 Changed 15 months ago by Don-vip

In 14062/josm:

see #16010 - bugfixes (patch by ris)

comment:36 Changed 15 months ago by Don-vip

The test is still failing.

comment:37 Changed 15 months ago by ris

Hmm thats its sibling test that was failing reliably for me - this one only fails sporadically for me & I'm still looking at it.

comment:38 Changed 15 months ago by ris

Hold on - this is a test that *calls out to the wild internet* - I'm not sure how it's supposed to be reproducible or reliable at all.

As it appears to be fetching standard tiles from osm.org I'm tempted to just port it to use TileSourceRule.

Version 0, edited 15 months ago by ris (next)

comment:39 Changed 15 months ago by ris

Done so in the attached patches. Of course I can't guarantee that this is going to fix jenkins - I'm unable to reproduce the failure on my local machine at the moment.

This is of course why people use pre-merge CI. Which is incidentally why I'm also working on some further tweaks to the travis & appveyor configurations.

comment:40 Changed 15 months ago by Don-vip

I know we're not really state of the art... Thanks for your work to help us improve the situation :)

comment:41 Changed 15 months ago by Don-vip

In 14066/josm:

see #16010 - more fixes (patch by ris)

comment:42 Changed 15 months ago by Don-vip

There is another problem with JMockit: all tests fail with Java 11 and 12 (they work with Java 10):

java.lang.UnsupportedOperationException: class redefinition failed: attempted to change the class NestHost or NestMembers attribute
	at java.instrument/sun.instrument.InstrumentationImpl.redefineClasses0(Native Method)
	at org.openstreetmap.josm.testutils.mockers.WindowMocker.<init>(WindowMocker.java:19)
	at org.openstreetmap.josm.testutils.mockers.ExtendedDialogMocker.<init>(ExtendedDialogMocker.java:72)
	at org.openstreetmap.josm.plugins.PluginHandlerJOSMTooOldTest.testUpdatePluginsDownloadBoth(PluginHandlerJOSMTooOldTest.java:108)
	at com.github.tomakehurst.wiremock.junit.WireMockRule$1.evaluate(WireMockRule.java:73)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:689)

Do you know if we can find a workaround? We should also report this issue upstream.

comment:43 Changed 15 months ago by ris

(Working on getting a sane java 11 dev environment up & running...)

comment:45 Changed 15 months ago by ris

If the absolute worst came to the worst we could at least prevent the lack of a working jmockit ruining *all* the tests by dropping in a ...mock... version of jmockit that simply does a junit "skip test" when instantiated.

comment:46 Changed 15 months ago by Don-vip

Right now I'm more concerned about JMockit itself. It seems maintained by a single guy modifying a very old ASM code base (5.x). Having this situation in 2018 (ASM 6 has been released like two years ago) make me very uncomfortable with this library. Would it be really difficult to switch to Mockito? It has already preliminary support of Java 12, I don't want to wait for JMockit updates every 6 months to be able to test latest version of Java.

comment:47 Changed 15 months ago by ris

Mockito, from what I can tell, doesn't really do global mocking, which is what I'm largely using jmockit for here. And global mocking is required because:

A) As much as we'd like otherwise, josm source is not all dependency-injected & easily mockable (yet?). There are many parts that depend on singletons. And many of these parts look as though they would significantly increase in complexity if they weren't.

B) Even if we did have a fully DI architecture (though it would still be extremely complex to mock deeply embedded references to classes), making these changes through instantiating newly mocked objects for the application state for every test would be extremely' expensive and you'd be back to the days of each test taking a minute or two.

Faced with these issues, jmockit appears to offer a miraculously superior solution.

On a slightly tangential note, doesn't java 10+ offer the possibility of native builds and thus give us a bit of slack from the treadmill of java upgrades? Surely the principal factor driving us to be staying on top of the bleeding edge versions is the need for us to be able to run on the version of java that the user has been told to go and download, which will always inevitably be Java <latest>...

...but if josm were self-contained...?

comment:48 Changed 15 months ago by Don-vip

OK. Well let's see if the author is responsive. I tried to upgrade ASM in his library but he has customized ASM a lot... This makes the update very difficult so I gave up.

comment:49 Changed 15 months ago by ris

Good news - I can reproduce the current jenkins failure ~1 in 8 runs, so that gives me a means to investigate it.

comment:50 Changed 15 months ago by ris

It's a timing issue - the fix will almost certainly involve inserting an awaitility call.

comment:51 Changed 15 months ago by ris

Ok - that reliably fixes it for me.

comment:52 Changed 15 months ago by Don-vip

In 14069/josm:

see #16010 - fix failing unit test (patch by ris)

comment:53 Changed 15 months ago by Don-vip

comment:54 Changed 15 months ago by ris

Right.

Problem is that was the last way I could get it to fail locally in headless mode.

I can get it to fail on travis in headless mode, but debugging any further will require me to figure out how to get the report off travis. Though I do wonder how far I might be able to get doing a bisection of my patch stack using travis as the bellwether.

comment:55 Changed 15 months ago by Don-vip

In 14076/josm:

see #16010 - JMockit must be included before Junit in the classpath

comment:56 Changed 15 months ago by ris

I'm not sure this is actually necessary if using the -javaagent:... argument but can't hurt.

comment:57 Changed 15 months ago by Don-vip

It's needed to run tests from Eclipse.

comment:58 Changed 15 months ago by Don-vip

In 14081/josm:

see #16010 - Ignore tests using JMockit on Java 11+, workaround to https://github.com/jmockit/jmockit1/issues/534

comment:59 Changed 15 months ago by Don-vip

In 14084/josm:

see #16010 - fix JMockit ignore rule

comment:60 Changed 15 months ago by ris

Having run a lot more tests it seems the failures of DownloadWmsAlongTrackActionTest are less to do with which system they're running on, far more to do with the set of other tests it's being run with. This probably means it's to do with another test not tidying up after itself or a test that now tidies up after itself, but didn't used to. A lot of the tests are quite sensitive to this and I've tried to make it better but we're clearly not there yet - find a way to run the tests in a different order than normal and I'm certain you'd get more failures. I'll keep investigating.

comment:61 Changed 14 months ago by Don-vip

Follow-up: #16590

comment:62 Changed 14 months ago by Don-vip

Wow, the JMockit developer is such a douche! I never faced this childish behavior on GitHub. I hope in the future we can get a more sane situation for JOSM source code and can use Mockito instead (the authors are a lot more friendly!)

Last edited 14 months ago by Don-vip (previous) (diff)

comment:63 Changed 14 months ago by ris

I don't disagree on the douchiness... but I do think using non-in-place mocking, however we fixed up JOSM, would still be very hard work and lead to back to the land of extremely slow tests. To me, an ideal solution would be someone sensible simply forking it. Which he may drive someone to do sooner or later.

comment:64 Changed 14 months ago by Don-vip

In 14187/josm:

see #16010 - fix JMockit failure on Java 11+

comment:65 in reply to:  63 ; Changed 13 months ago by Don-vip

Replying to ris:

I don't disagree on the douchiness...

We can speak again on the ticket I have posted a comment. Let's see how long it survives.

comment:66 in reply to:  56 Changed 13 months ago by Don-vip

Replying to ris:

I'm not sure this is actually necessary if using the -javaagent:... argument but can't hurt.

This is now mandatory with version 1.42.

comment:67 in reply to:  65 Changed 13 months ago by Don-vip

Replying to Don-vip:

Replying to ris:

I don't disagree on the douchiness...

We can speak again on the ticket

But I didn't see he closed the ticket. Pff... I guess we will have to patch the library ourselves. Seriously I can't stand this guy.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.