[PATCH] Improvements for testing painting

[ris]

Again, this is a patch set that is centered around improving the tests for MinimapDialog, but actually I'm largely trying to improve the facilities for writing reliable tests.

Couple of potentially controversial things in here.

First three patches are really just tidying of MinimapDialogTest - nothing particularly interesting there. Then we have

Controversial patch number one: NavigatableComponent

Some changes to NavigatableComponent and MapViewState to make it able to successfully perform an updateLocationState in unit tests. If we detect we're in headless mode, these patches rather *assume* that we're running unit tests and generate appropriate "mock" behaviour, e.g. our top level window is (10, 10) from the screen top left. To me, this is slightly preferable to requiring tests to do complicated mocking to get this to work which may or may not require large parts of JOSMFixture to be re-evaluated etcetera...

There is an ongoing danger I can see though that we conflate "headless mode" with "test mode" which could end up biting us when cli/batch mode is taken into consideration. Don't know.

Controversial patch number two: ImagePatternMatching

Have I gone insane? Possibly. Testing images is difficult presuming you don't want to create fragile, possibly platform-specific "exact result" tests. When asserting properties about generated images (testing pixels etcetera) I found myself writing an awful lot of repetitive code until I got to the point I realized I was just doing manual pattern-matching. But we've got a well-known tool for concise pattern-matching, it's just that most implementations of it are designed to operate on strings of characters rather than pixel values - regexes. ImagePatternMatching is a set of utility methods that will convert a row or column of an image into characters using a supplied mapping function, then attempt to match it against the given regex, returning the Matcher. See in the example use - MinimapDialogTest$testViewportAspectRatio - how straightforward it makes asserting that the viewport marker rectangle is in the center of the painted image, without being affected with any absolute sizes. It would require very few changes now to allow these tests to be parametrized across different MinimapDialog dimensions.

The example use cases just show matching against a simple block color palette, but a large amount of flexibility is revealed by using a user-specified palette mapping function. A range of pixel shades could be mapped to a single character, or the mapping could be based on just a single channel - it's up to the caller.

Isn't this going to be slow and memory inefficient? Hopefully not for just single rows or columns. I mentally toyed with the idea that an entire image could be checked with a single (albeit complex) regular expression if the palettization converted it into a multiline string, but let's just leave that in the "ideas" column for now (part of the problem with that would be that when failing, it would generally be unclear where in the image was causing the match to fail).

I realize I haven't added docstrings for these methods - wanted to show the work before going to that bother.

Patches based on r13169, also visible on github:

​https://github.com/risicle/josm/compare/da7bc804f846699e72be4b56f851e39f5a5e068f...ris-minimapdialogtest-pixels

[Don-vip]

Replying to ris:

First three patches are really just tidying of MinimapDialogTest - nothing particularly interesting there.

Actually only the first two are not controversial. The third one makes an unnecessary call to commands() which will currently trigger a call to JOSMFixture (because the switch from JOSMFixture to TestRules is far from being finished).

Controversial patch number one: NavigatableComponent

There is an ongoing danger I can see though that we conflate "headless mode" with "test mode" which could end up biting us when cli/batch mode is taken into consideration.

I appreciate your concern. I'd say this is OK here, because we're in GUI classes and the changes are very limited (only three lines of code). So I'm ok with it. @bastiK is it for you too?

Controversial patch number two: ImagePatternMatching

Have I gone insane? Possibly.

Great achievements are made by insane people :)

See in the example use - MinimapDialogTest$testViewportAspectRatio - how straightforward it makes asserting that the viewport marker rectangle is in the center of the painted image, without being affected with any absolute sizes. It would require very few changes now to allow these tests to be parametrized across different MinimapDialog dimensions.

It looks really great, I like it :) @bastiK what do you think?

I realize I haven't added docstrings for these methods - wanted to show the work before going to that bother.

Yes, this new class clearly deserves proper documentation.

[Don-vip]

In 13172/josm:

see #15599 - apply first two patches of ris to improve MinimapDialogTest

[bastiK]

No objections! :)

[Don-vip]

Great :) I'll commit last two patches once we get full javadoc then :)

[ris]

New patches rebased against r13174.

[ris]

Hooold on I get an NPE with these, didn't notice. Hold off.

[ris]

So yes it seems these tests won't work without .commands() or at least what is going to replace it because without it you don't end up with .mapView on your MapFrame (or could this just be because the JOSMFixture replacements don't add a TestLayer or anything of the like? something for investigation tomorrow night methinks). Anyway if the worst comes to the worst this work can be sidelined until the fixture work is ready for it.

[ris]

Ok - it was indeed the missing layer causing the problem. v3 should work.

[ris]

(might it be nice to add this as a .testLayer() to the JOSMTestRule to save people some boilerplate?)

[Don-vip]

Replying to ris:

(might it be nice to add this as a .testLayer() to the JOSMTestRule to save people some boilerplate?)

Good idea!

[Don-vip]

I've got a test failure:

java.lang.AssertionError: Viewport marker not 2:1 aspect ratio
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.openstreetmap.josm.gui.dialogs.MinimapDialogTest.testViewportAspectRatio(MinimapDialogTest.java:379)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.openstreetmap.josm.testutils.JOSMTestRules$TimeoutThread.run(JOSMTestRules.java:558)

[ris]

Ah - interesting - could you try pasting the following in just before the failing assertion?

try {
    javax.imageio.ImageIO.write(probeScratchImage, "png", new java.io.File("failed.png"));
} catch (java.io.IOException ioe) {
    System.err.println("Failed writing image");
}

(at some point it would be nice to add a fancy assertion wrapper which would do this automagically on failure but there are a few unknowns there (where to put the image??))

[Don-vip]

probeScratchImage?

[Don-vip]

        try {
            javax.imageio.ImageIO.write(paintedSlippyMap, "png", new java.io.File("failed.png"));
        } catch (java.io.IOException ioe) {
            System.err.println("Failed writing image");
        }

[ris]

God damn it event clearly hasn't propagated. Will look at it in the morning.

What platform is this on btw?

[Don-vip]

Identification: JOSM/1.5 (13174 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 1703 (15063)
Java version: 1.8.0_152-b16, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080, \Display1 1920x1080, \Display2 1280x1024
Maximum Screen Size: 1920x1080

[ris]

Ah I have an idea why it might not be working - could you try changing line 358 from

mapView.zoomTo(mapView.getCenter());

to

mapView.zoomTo(mapView.getCenter().add(1., 0.));

NavigatableComponent attempts to ignore "moves" to the same position, but it might have just been "working" on my machine due to float numerical error or something...

(probeScratchImage yeah sorry copy-pasta'd it from another context...)

[Don-vip]

Yes, it's working, thanks :)

[Don-vip]

In 13181/josm:

fix #15599 - Improvements for testing painting (last patches by ris)

[ris]

fist pump