Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15508 closed enhancement (fixed)

[PATCH] add TileSourceRule, use in MinimapDialogTest

Reported by: ris Owned by: team
Priority: normal Milestone: 17.11
Component: Unit tests Version: latest
Keywords: wiremock unit tests tile imagery minimap Cc: michael2402

Description (last modified by ris)

This isn't quite finished (missing some commenting and doc strings for some public methods etc) but I felt I should share a bit of what I'm up to.

The attached patches add a subclass of WireMockRule which when applied to a junit test will provide a (limited) mock tile server. It also has the ability to add these as tile sources to the ImageryLayerInfo list.

This rule can either be used on its own on a test if access from the ImageryLayerInfo list isn't needed or can be added to a JOSMTestRules using a new .fakeImagery() method. In the latter case, the rule will automagically replace the ImageryLayerInfo list with its list of sources at the appropriate time in the initialization routine (and also perform a small hack to remove SlippyMapBBoxChooser's helpful-but-also-untestable default hardcoded fallback osm tile source).

Or if .fakeImagery() call is added without any arguments it will simply add a default selection of useful tile sources.

A slight hack is required to mitigate a possible bug in WireMock (https://github.com/tomakehurst/wiremock/issues/97) which causes belated startup of a WireMock server, resulting in connection errors if this is not accounted for. I've tried to make this less severe by splitting TileSourceRule's apply() method in two, separately callable, apply() methods. This is taken advantage of in JOSMTestRule's apply() method, with the intention of beginning the server startup process early in the initialization routine leaving the test having to wait as long before it can reliably start its work. Even so, I've had to put a 3500ms wait in the test to get it to work reliably on my system, which I'm not that happy about.

As an example use of this rule, I've added a test of SlippyMapBBoxChooser's source-switching functionality (through the MinimapDialog as its host in this case).

I probably have more to say about this but it's 1am.

Will post the patches in a second, but the changes can also be seen through the prettier github interface:

https://github.com/risicle/josm/compare/82656d49c282fa8202af2164e1b0d730ce907f16...ris-tilesourcerule

(patches are against r13064)

Attachments (16)

v1-0001-TestUtils-add-getPrivateStaticField-in-the-same-vein.patch (1.5 KB ) - added by ris 6 years ago.
v1-0002-add-TileSourceRule-a-junit-rule-for-creating-mock-ti.patch (9.2 KB ) - added by ris 6 years ago.
v1-0003-JOSMTestRules-add-fakeImagery-method-s-for-setting-u.patch (4.1 KB ) - added by ris 6 years ago.
v1-0004-MinimapDialogTest-add-testSourceSwitching.patch (6.0 KB ) - added by ris 6 years ago.
v2-0001-TestUtils-add-getPrivateStaticField-in-the-same-vein.patch (1.5 KB ) - added by ris 6 years ago.
v2-0002-add-TileSourceRule-a-junit-rule-for-creating-mock-ti.patch (12.5 KB ) - added by ris 6 years ago.
v2-0003-JOSMTestRules-add-fakeImagery-method-s-for-setting-u.patch (4.2 KB ) - added by ris 6 years ago.
v2-0004-MinimapDialogTest-add-testSourceSwitching.patch (6.0 KB ) - added by ris 6 years ago.
v3-0001-TestUtils-add-getPrivateStaticField-in-the-same-vein.patch (1.5 KB ) - added by ris 6 years ago.
v3-0002-add-TileSourceRule-a-junit-rule-for-creating-mock-ti.patch (13.4 KB ) - added by ris 6 years ago.
v3-0003-JOSMTestRules-add-fakeImagery-method-s-for-setting-u.patch (4.2 KB ) - added by ris 6 years ago.
v3-0004-MinimapDialogTest-add-testSourceSwitching.patch (6.0 KB ) - added by ris 6 years ago.
v4-0001-TestUtils-add-getPrivateStaticField-in-the-same-vein.patch (1.5 KB ) - added by ris 6 years ago.
v4-0002-add-TileSourceRule-a-junit-rule-for-creating-mock-ti.patch (13.6 KB ) - added by ris 6 years ago.
v4-0003-JOSMTestRules-add-fakeImagery-method-s-for-setting-u.patch (4.2 KB ) - added by ris 6 years ago.
v4-0004-MinimapDialogTest-add-testSourceSwitching.patch (5.9 KB ) - added by ris 6 years ago.

Download all attachments as: .zip

Change History (42)

comment:1 by ris, 6 years ago

Description: modified (diff)
Type: defectenhancement

comment:2 by ris, 6 years ago

Description: modified (diff)

comment:3 by ris, 6 years ago

Description: modified (diff)

comment:4 by Don-vip, 6 years ago

Milestone: 17.11
Priority: minornormal

waw, this looks nice :) Thank you for spending time on improving JOSM unit tests! Looking forward to see if you find how to resolve the 3500ms delay.

comment:5 by ris, 6 years ago

Tests make me able to sleep at night.

Yeah 3500ms sucks, it seems WireMock are passing the buck to it being a Jetty bug...

comment:6 by ris, 6 years ago

Posting v2 of patch set with improved teardown routine, comments and doc strings.

Also visible at https://github.com/risicle/josm/compare/82656d49c282fa8202af2164e1b0d730ce907f16...ris-tilesourcerule-v2

comment:7 by ris, 6 years ago

Good news. I've managed to abolish the 3500ms delay. The trick is to send an early dummy request to the WireMock server to prompt it to start. Posting v3, which is also visible here:

https://github.com/risicle/josm/compare/82656d49c282fa8202af2164e1b0d730ce907f16...ris-tilesourcerule-v3

It would be good to know if this works for others.

in reply to:  7 comment:8 by Don-vip, 6 years ago

Replying to ris:

Good news. I've managed to abolish the 3500ms delay.

Great! :) Do you feel the patch is ready to be merged?

comment:9 by ris, 6 years ago

If others find it's reliably passing for them.

However I haven't fixed the checkstyle issues yet, of which there are a few.

comment:11 by ris, 6 years ago

Oh and v4 has been rebased against r13075

comment:12 by Don-vip, 6 years ago

Thanks, this looks very good! :)
Could you just please check javadoc warnings for public fields + public methods parameters and return types? (especially for TileSourceRule)

comment:13 by ris, 6 years ago

For me the only javadoc warnings relevant to me I get are from my *last* set of commits (PopupMenuButton) :D

Should really sort that out some time, think the only way to silence them would be to put in a bunch of repetitive notes on the constructor args (which are just pass-throughs to JButton)

comment:14 by Don-vip, 6 years ago

Resolution: fixed
Status: newclosed

In 13078/josm:

fix #15508 - add TileSourceRule, use in MinimapDialogTest (patch by ris, modified) + update WireMock to 2.10.1

comment:15 by ris, 6 years ago

Cool - further progress on testing of MinimapDialog/SlippyMapBBoxChooser looks like it will need a "working" MapView. "Working" in the sense that isVisibleOnScreen() and e.g. MapViewState.findTopLeftInWindow() will operate properly so that the "current view" loop will work.

In plugin development I've got around this by using jmockit to hard-wire these (in a way originally suggested by your own NavigatableComponentTest.NavigatableComponentMock):

https://github.com/risicle/josm-markseen/blob/v6/test/unit/org/openstreetmap/josm/plugins/markseen/MarkSeenRootTest.java#L68

but using heavy handed mocking tools feels it shouldn't really be necessary given that it's probably quite a common desire and josm/josm's test utils could be designed to accommodate a mock-"working" viewport.

So, looking into JOSMTestRules and JOSMFixture to see how this might be put into action, it feels a bit like a construction site - with old ways and not-yet-complete new ways of doing things. Is there something like this already in the works?

comment:16 by Don-vip, 6 years ago

Cc: michael2402 added

Michael, this is for you:)

comment:17 by michael2402, 6 years ago

I'm the one working on the JOSMTestRules and on most of the MapView coordinate /... stuff.

Well, not at the moment, since I have to finish my master thesis this week ;-). But after that I'll start working on my backlog...

The problem you are facing is a different one. I already started to extract the MapViewState to a new class so that those can be used in the tests more easily without the need of a global MapView instance. But many components of JOSM still reference the main map view. This is what the JOSMFixture is there for - it sets up a full running fake JOSM, with map view and everything. We had problems with this in the past because starting JOSM takes a lot of time and to keep the unit tests under and hour we can only run this fixture once. State changes in one test may then influence the results of other tests.

I migrated many tests to use the JOSMTestRules. I'm not really happy with the design myself but it seemed to be the easiest way to allow tests to require a initialized global data structure (Preferences, ...). The test rules reset the global state after the test has run. It's a bit hacky, especially since they need to be able to clean up after JOSMFixture and not influence tests that use the JOSMFixture.

If you feel the need for changing the current dialogs so that they e.g. receive a map view in their constructor, feel free to change this. I did it in several places (e.g. the layer list dialog which uses a local LayerManager instance). This makes testing much easier and more reliable.

comment:18 by ris, 6 years ago

(looking deeper into it, there are a few ways MapView is mocked to be semi-functional in different tests, but it would be nice to have a standard & easy way to do this. I'll continue thinking about it...)

(also, I realize that the sort of tests I'm looking at here would often be classed by many people as "Integration tests" but for me the line is a blurry one)

comment:19 by ris, 6 years ago

Thanks, that makes things a bit clearer.

comment:20 by michael2402, 6 years ago

And just a historical note: MapView was once the central instance holding all layers, most of the global state and a lot of stuff that did not belong there. This was cleaned up a lot the last years, but there are still things that should not be there.

In theory it should become a real view component one day, that only displays data and does not contain any global state. That way, we would not have that problem. But we are far away from that...

Some more notes on your tests:

  • Don't be afraid of extending / modifying SlippyMapBBoxChooser. Better than having a reflective call in the tests. You could e.g. make providers an instance field that is initialized with the global providers list. But then use a setProviders()-Method to overwrite that list in your test. That way you can be sure that your test leaves behind the intended SlippyMapBBoxChooser.
  • ByteArrayWrapper: At least Java has a library function for everything. ByteBuffer#wrap() and ByteBuffer#array() :D
  • ImageIO#write: Please do not ignore this error. There might still be some problem when converting that graphics to a PNG image. I had to help tracking down a problem where Java 9 throw a class not found exception when the image was in a specific byte ordering and Java 8 worked fine. It's really hard to find the problem with the test if that exception is silently ignored. Throw an AssertionError or do whatever, just pass it on somehow.

comment:21 by Don-vip, 6 years ago

If the test is purely autonomous and rely only on JOSM code & data, it can be seen as unit test. Integration tests for JOSM are those that rely on OSM ecosystem, see the few tests run in Jenkins which rely on:

  • JOSM wiki
  • JOSM plugins
  • Taginfo database
  • IANA database

comment:22 by ris, 6 years ago

Don't be afraid of extending / modifying SlippyMapBBoxChooser...

Ok, that sounds like a neat way of doing it - I try to restrict the amount I change things because... public interfaces and plugins and all that.

When it comes to passing MapView instances to constructors, is this the general route that things are going down? i.e. turning globals into dependency injections? My only concern I guess is that there may end up being ... a lot of them.

ByteBuffer#wrap()

Neat!

ImageIO#write

Guess so, think I was wanting to not bother consumers with something I saw as impossible, I'm kinda new to this java thing...

  • JOSM wiki
  • JOSM plugins
  • Taginfo database
  • IANA database

All look like things that could be WireMocked somewhere down the line :)

in reply to:  22 comment:23 by bastiK, 6 years ago

Replying to ris:

Don't be afraid of extending / modifying SlippyMapBBoxChooser...

Ok, that sounds like a neat way of doing it - I try to restrict the amount I change things because... public interfaces and plugins and all that.

Please do not hold back with reasonable changes! It requires a bit more care, but you can do most reworks without breaking binary compatibility (deprecating methods and classes instead of removing them directly).

When it comes to passing MapView instances to constructors, is this the general route that things are going down? i.e. turning globals into dependency injections?

Yes, there are quite a few changes in this direction in #15229. I found it useful to capture a functional subset by defining an interface and inject an instance of the interface and not the implementing class of the global.

My only concern I guess is that there may end up being ... a lot of them.

It isn't excessively used so far... As far as testing is concerned, an alternative to dependency injection is, to add a setter for the global object, so it can be replaced by a mocking instance.

  • JOSM wiki
  • JOSM plugins
  • Taginfo database
  • IANA database

All look like things that could be WireMocked somewhere down the line :)

Except that we want to check the current content of those URLs, so mocking defeats the purpose. ;)

comment:24 by ris, 6 years ago

Aaah I see the purpose of them now.

It isn't excessively used so far...

I was going to say: if I were to avoid using all global objects that I might need to mock, just to test SlippyMapBBoxChooser, it'll require passing in a layer manager, a prefs object...

But I realize that's going off the deep end a bit - MapView is particularly a difficult case that might need method overwriting because it might be needed to do something especially tricky - pretend it's drawing even though we're in headless mode.

It seemed to me the way forward would be for JOSMFixture, seeing as it's in charge of the setup of the environment to simply set things up with a pluggable overridden implementation of MapView as asked, but if we're trying to perform the setup once per session that would probably not do.

I'll keep reading code & thinking.

comment:25 by Don-vip, 6 years ago

Sonar is not happy with the use of Thread.sleep() in MinimapDialogTest. Do you think we could avoid it?

comment:26 by ris, 6 years ago

Just removing them wouldn't work - the tests are performing http requests, which is fundamentally an asynchronous task (well it certainly is with the threadpool-based way JMapViewer performs its requests).

Mocking the http client out is something I've explored but turned out to be extremely tricky, what's more it wouldn't avoid the asynchronicity of the operation as the tasks are handed out into threads long before they get near the http client.

Its' suggestion of Awaitility looks really interesting though, and I do agree that there will always be an element of fragility around relying on sleep times. I'll investigate.

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. Next status will be 'reopened'.

Add Comment


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