Modify

Opened 3 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#15599 closed enhancement (fixed)

[PATCH] Improvements for testing painting

Reported by: ris Owned by: team
Priority: normal Milestone: 17.12
Component: Unit tests Version: latest
Keywords: NavigatableComponent updateLocationState MapViewState ImagePatternMatching regex Cc: bastiK

Description

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

Change History (34)

comment:1 Changed 3 weeks ago by Don-vip

Milestone: 17.12

comment:2 in reply to:  description Changed 3 weeks ago by Don-vip

Cc: bastiK added
Priority: minornormal

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.

comment:3 Changed 3 weeks ago by Don-vip

In 13172/josm:

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

comment:4 Changed 3 weeks ago by bastiK

No objections! :)

comment:5 Changed 3 weeks ago by Don-vip

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

comment:6 Changed 3 weeks ago by ris

New patches rebased against r13174.

comment:7 Changed 3 weeks ago by ris

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

comment:8 Changed 3 weeks ago by 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.

comment:9 Changed 2 weeks ago by ris

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

comment:10 Changed 2 weeks ago by ris

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

Last edited 2 weeks ago by ris (previous) (diff)

comment:11 in reply to:  10 Changed 2 weeks ago by 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!

comment:12 Changed 2 weeks ago by 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)

comment:13 Changed 2 weeks ago by 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??))

Last edited 2 weeks ago by Don-vip (previous) (diff)

comment:14 Changed 2 weeks ago by Don-vip

probeScratchImage?

Changed 2 weeks ago by Don-vip

Attachment: failed.png added

comment:15 Changed 2 weeks ago by 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");
        }


comment:16 Changed 2 weeks ago by ris

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

What platform is this on btw?

comment:17 Changed 2 weeks ago by 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

comment:18 Changed 2 weeks ago by 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...)

Last edited 2 weeks ago by Don-vip (previous) (diff)

comment:19 Changed 2 weeks ago by Don-vip

Yes, it's working, thanks :)

comment:20 Changed 2 weeks ago by Don-vip

Resolution: fixed
Status: newclosed

In 13181/josm:

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

comment:21 Changed 2 weeks ago by ris

fist pump

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.