Tests: use JMockit to enable more extensive test coverage

[ris]

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.

[Don-vip]

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

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

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

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

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

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

[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).

[Don-vip]

Replying to ris:

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

This would be great, indeed :)

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

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

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

[Don-vip]

Thanks for the heads up :)

[ris]

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

[ris]

(sigh)

[Don-vip]

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

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

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

[Don-vip]

​Done :) Thanks!

[ris]

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

[Don-vip]

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

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

[Don-vip]

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

[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

[Don-vip]

In 14053/josm:

see #16010 - fix eclipse classpath

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

[Don-vip]

In 14054/josm:

fix #16010 - fix java warning

[Don-vip]

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

[Don-vip]

There is also a Findbugs warning:

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

[ris]

Looking....

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

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

[ris]

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

[Don-vip]

In 14062/josm:

see #16010 - bugfixes (patch by ris)

[Don-vip]

The test is still failing.

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

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

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

[Don-vip]

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

[Don-vip]

In 14066/josm:

see #16010 - more fixes (patch by ris)

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

[ris]

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

[ris]

Have created ​https://github.com/jmockit/jmockit1/issues/534

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

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

[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...?

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

[ris]

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

[ris]

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

[ris]

Ok - that reliably fixes it for me.

[Don-vip]

In 14069/josm:

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

[Don-vip]

Not for us :(

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

[Don-vip]

In 14076/josm:

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

[ris]

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

[Don-vip]

It's needed to run tests from Eclipse.

[Don-vip]

In 14081/josm:

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

[Don-vip]

In 14084/josm:

see #16010 - fix JMockit ignore rule

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

[Don-vip]

Follow-up: #16590

[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!)

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

[Don-vip]

In 14187/josm:

see #16010 - fix JMockit failure on Java 11+

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

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

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

[taylor.smock]

Follow up: #23729: "Investigate moving from jmockit to something actively maintained"

Replying to 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?

The upstream jmockit project is dead (no commit since 2020). I don't know if the original developer lost interest, retired, or what.

[Don-vip]

My fears came true :) The guy was a complete moron, no surprise to see his project dying, as he was completely un-collaborative with his user base. Mockito has improved a lot in the past years, maybe it has now everything we need to perform global mocking without losing (too much) performance?

[taylor.smock]

I'd have to make a test branch with mockito changes to know if it will work. Right now (in #23729) stoecker is more inclined to keep jmockit (using the fork I mentioned in #23729), since it would be less work right now (and I do see his point on that).

Anyway, I put #23729 in the milestone:Longterm because it isn't an issue right now, but I can see it becoming an issue in the future.

P.S.
I recall you used Eclipse for JOSM development; I recently made a change which removed Eclipse project files in favor of maven pom files (partly due to the fact that the ivy plugin for Eclipse is no longer supported). If you start working on JOSM again and have issues moving from the Eclipse project files to maven, feel free to ping me. I think wiki:DevelopersGuide/Compiling#CompilingusingEclipseMaven is "good enough", but my daily driver isn't Eclipse.