[PATCH] MinimapDialogTest: add tests covering "show downloaded area" functionality

[ris]

Quite a simple extension of MinimapDialogTest using largely the existing mechanisms to test the "show downloaded area" feature.

Patch against r13187, also visible here:

​https://github.com/risicle/josm/compare/79b06231b3e9342e2c95c1081ff4f82eb466b1d7...ris-minimapdialogtest-downloadedarea

And that mostly concludes the testing work I wanted to do to cover my previous feature addition... can probably get back to feature work now...

[Don-vip]

I have two failures:

java.lang.AssertionError: Row 88 failed to match pattern ^g+bv+bg+$
	at org.junit.Assert.fail(Assert.java:88)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.imageStripPatternMatchInner(ImagePatternMatching.java:51)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.rowMatch(ImagePatternMatching.java:211)
	at org.openstreetmap.josm.gui.dialogs.MinimapDialogTest.testShowDownloadedAreaLayerSwitching(MinimapDialogTest.java:634)

java.lang.AssertionError: Row 88 failed to match pattern ^d+bq+v+bg+d+$
	at org.junit.Assert.fail(Assert.java:88)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.imageStripPatternMatchInner(ImagePatternMatching.java:51)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.rowMatch(ImagePatternMatching.java:211)
	at org.openstreetmap.josm.gui.dialogs.MinimapDialogTest.testShowDownloadedArea(MinimapDialogTest.java:484)

[ris]

When those assertions fail they should also print to the stderr the failed row/col in character form - would you be able to paste that too please?

[Don-vip]

Sure:

testShowDownloadedAreaLayerSwitching:
Full strip failing to match pattern ^g+bv+bg+$: ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddbqqqqqqqqqqqqqqqqqqqqqqqqqqbdddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

testShowDownloadedArea:
Full strip failing to match pattern ^d+bq+v+bg+d+$: dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvvvvvvvvvvvvvvqqqqqqqqqqbddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

[ris]

Thinking about this, it's a very odd error to get, because there's very little special that the test's doing, and the downloaded area marks seem to be "out" a bit relative to the viewport marker... I've got to wonder if we've stumbled on a genuine bug. Could you check with this build whether, in real use, the "downloaded area" marks appear accurate?

[ris]

/me considers grabbing MS's gratis Win10 VM image to see if he can use it as a ghetto java development environment to debug this.

[ris]

Ok, that's really weird. I can't reproduce the failure on a Win10 vm running jdk 1.8.0_152-b16.

All 7 tests pass fine.

[Don-vip]

Still failing with me, what can I do to help?

[ris]

Yeah I'm probably going to have to try some "could you try this" at some point unless I stumble upon someone's machine that will reproduce the failure.

Incidentally, this got me thinking about cross-platform CI... it might be nice to add an ​Appveyor and/or Travis builder for the JOSM repo so that we get, respectively, continuous windows and macos testing in addition to Jenkins' linux coverage.

The free tier for Appveyor and Travis are both restricted to github repos from what I can see, but there's no reason they can't work against the ​github mirror we already have. Probably wouldn't be much more effort than setting up a couple of .yml files in the repo root.

[Don-vip]

Replying to ris:

Incidentally, this got me thinking about cross-platform CI... it might be nice to add an ​Appveyor and/or Travis builder for the JOSM repo so that we get, respectively, continuous windows and macos testing in addition to Jenkins' linux coverage.

The free tier for Appveyor and Travis are both restricted to github repos from what I can see, but there's no reason they can't work against the ​github mirror we already have. Probably wouldn't be much more effort than setting up a couple of .yml files in the repo root.

Good idea :)

[Don-vip]

In 13234/josm:

see #15630 - add AppVeyor configuration file

[ris]

That was fast.

/me crosses that off his todo list...

[Don-vip]

In 13236/josm:

see #15630 - AppVeyor: fix build

[Don-vip]

In 13235/josm:

see #15630 - AppVeyor: specify image & refresh environment

[ris]

(also, enabling it for pull requests will allow those of us without access to all these platforms to test against them before submitting patches here)

[Don-vip]

In 13237/josm:

see #15630 - AppVeyor: fix build

[Don-vip]

In 13238/josm:

see #15630 - add Travis configuration file

[Don-vip]

In 13239/josm:

see #15630 - CI tweaks

[Don-vip]

In 13240/josm:

see #15630 - CI tweaks

[ris]

Hmm also can't reproduce on a Win7 32bit machine.

[Don-vip]

Maybe linked to screen resolution? I can reproduce on my two Win10 machines (desktop & laptop). My main resolution is 1920x1080

[ris]

Ah that is an interesting idea - I was under the impression that headless mode prevented java from communicating with the host windowing system entirely but perhaps not...?

[ris]

No joy on either the Win7 or Win10 vm at 1920x1080. Got to wonder if it's just that your machines are faster than mine and hit a race condition that mine generally doesn't.

[ris]

My genius idea to debug this against appveyor through a PR was foiled: ​https://ci.appveyor.com/project/don-vip/josm/build/66

[Don-vip]

Yeah I don't find how to fix this error: Maximum allowed artifact storage size of 1000 Mb will be exceeded.
Nowhere I've found a way to configure how artifacts can be deleted, like on Jenkins. This is a basic CI feature...

[ris]

Yes, though I'm mainly concerned with the jvm crash halfway through, meaning half the tests don't get to run.

Surely we don't really need *all* of the test environments to build the javadoc? Test output html alone would be pretty useful.

[ris]

Ok got appveyor to run the test, though no joy - passes happily.

    [junit] Running org.openstreetmap.josm.gui.dialogs.MinimapDialogTest
    [junit] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 8.582 sec

are both of your failing machines german locale by any chance? it's hard to see how that could have anything to do with it though...

[Don-vip]

Both French.

[ris]

Ok could you try again please with the above patch on top and tell me the Full strip failing to match pattern string for testShowDownloadedArea? (failure is expected)

[Don-vip]

This is what I get:

2018-01-14 00:20:03.456 INFOS: GET https://josm.openstreetmap.de/wiki/Fr:StartupPage -> 200
2018-01-14 00:20:04.379 INFOS: Defaults for imagery.generic.default_autozoom differ:  != true
2018-01-14 00:20:04.379 INFOS: Defaults for imagery.generic.default_autoload differ:  != true
2018-01-14 00:20:04.379 INFOS: Defaults for imagery.generic.default_showerrors differ:  != true
2018-01-14 00:20:04.441 AVERTISSEMENT: No configuration settings found.  Using hardcoded default values for all pools.
2018-01-14 00:20:04.488 AVERTISSEMENT: Region [TMS_BLOCK_v2] Resetting cache
2018-01-14 00:20:04.597 INFOS: GET http://localhost:53795/b9d42546/10/511/339.png -> 200
2018-01-14 00:20:04.614 INFOS: GET http://localhost:53795/b9d42546/10/512/339.png -> 200
Full strip failing to match pattern ^d+bq+v+bg+d+$: ############dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvv######################b#########################################################################################################dddddddddddddddddddddddddddd

java.lang.AssertionError: Row 88 failed to match pattern ^d+bq+v+bg+d+$
	at org.junit.Assert.fail(Assert.java:88)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.imageStripPatternMatchInner(ImagePatternMatching.java:51)
	at org.openstreetmap.josm.testutils.ImagePatternMatching.rowMatch(ImagePatternMatching.java:211)
	at org.openstreetmap.josm.gui.dialogs.MinimapDialogTest.testShowDownloadedArea(MinimapDialogTest.java:484)

[ris]

Very interesting. This suggests that it's the viewport position itself that isn't being set properly. I take it this still happens with a clean git clone of the branch in question? (the reason I say that is that all of the test runs I've done have been from git clones - would be good to rule out that this isn't due to some screwup in the git -> .patch -> svn process)

[Don-vip]

Replying to ris:

I take it this still happens with a clean git clone of the branch in question?

Yes same error:

2018-01-15 00:18:01.482 INFOS: GET http://localhost:50094/e89be2ce/10/512/339.png -> 200
2018-01-15 00:18:01.494 INFOS: GET http://localhost:50094/e89be2ce/10/511/339.png -> 200
Full strip failing to match pattern ^d+bq+v+bg+d+$: dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvvvvvvvvvvvvvvqqqqqqqqqqbddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

[ris]

Ok, another debugging test patch to apply & test please. This should print what it *thinks* the current viewport bounds should be.

[Don-vip]

Here you go:

2018-01-18 01:31:11.464 INFOS: GET http://localhost:54903/dc8cea6/10/511/339.png -> 200
2018-01-18 01:31:11.469 INFOS: GET http://localhost:54903/dc8cea6/10/512/339.png -> 200
Reported bbox: Bounds[51.7425241,-0.0083476,51.7425241,-0.0083476]
Full strip failing to match pattern ^d+bq+v+bg+d+$: dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvvvvvvvvvvvvvvqqqqqqqqqqbddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

[ris]

That's a tellingly screwed up result - could you try the next patch please? (which incidentally implements toString for MapViewState, which is a generally useful method we might like to keep anyway)

[Don-vip]

Here's the new output:

Reported bbox: Bounds[51.7425241,-0.0083476,51.7425241,-0.0083476]
Reported MapViewState: org.openstreetmap.josm.gui.MapViewState [projecting=WGS 84 / Pseudo-Mercator viewWidth=0 viewHeight=0 scale=8.604068975896771 topLeft=EastNorth[e=-929.2480534658272, n=6753703.722751409]]
Reported slippyMap bounding box: Bounds[51.7185884,-0.0083476,51.7425241,0.0302983]
Full strip failing to match pattern ^d+bq+v+bg+d+$: dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvvvvvvvvvvvvvvqqqqqqqqqqbddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

[ris]

viewWidth=0 viewHeight=0

aha!

[ris]

Let's see what today's debugging patch brings forth...

[Don-vip]

MapView bounds 1: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 2: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 3: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 4: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 5: java.awt.Rectangle[x=0,y=0,width=500,height=500]
Reported bbox: Bounds[51.7425241,-0.0083476,51.7425241,-0.0083476]
Reported MapViewState: org.openstreetmap.josm.gui.MapViewState [projecting=WGS 84 / Pseudo-Mercator viewWidth=0 viewHeight=0 scale=8.604068975896771 topLeft=EastNorth[e=-929.2480534658272, n=6753703.722751409]]
Reported slippyMap bounding box: Bounds[51.7185884,-0.0083476,51.7425241,0.0302983]
Full strip failing to match pattern ^d+bq+v+bg+d+$: dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddgggggggggbvvvvvvvvvvvvvvvvvqqqqqqqqqqbddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd

I can debug to see where this viewWidth=0 comes from...

[Don-vip]

OK I get it! The difference comes from I was not running the test in headless mode. If I add -Djava.awt.headless=true I got:

2018-01-23 23:45:29.292 AVERTISSEMENT: Headless. Using fake clipboard.
MapView bounds 1: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 2: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 3: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 4: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 5: java.awt.Rectangle[x=0,y=0,width=500,height=500]
MapView bounds 6: java.awt.Rectangle[x=0,y=0,width=500,height=500]
Reported bbox: Bounds[51.7305578,-0.0276705,51.7544872,0.0109753]
Reported MapViewState: org.openstreetmap.josm.gui.MapViewState [projecting=WGS 84 / Pseudo-Mercator viewWidth=500 viewHeight=500 scale=8.604068975896771 topLeft=EastNorth[e=-3080.26529744002, n=6755854.739995383]]
Reported slippyMap bounding box: Bounds[51.7305578,-0.0276705,51.7544872,0.0109753]

and test success.

Can you please see if the test can succeed in both modes? (-Djava.awt.headless=true and -Djava.awt.headless=false)

[ris]

Oh! I thought we implicitly always ran the tests in headless mode. Assumed we weren't even geared up to do otherwise - never even checked. If I know that's the difference, I guess I'll be able to replicate it for myself. But that's tomorrow night's task...

[Don-vip]

Jenkins always runs them in headless mode because we have no choice, but this is not true by default from an IDE (right click => run as JUnit test).

[ris]

because we have no choice

Not quite true, you can always run Xvfb and set the DISPLAY env var appropriately. Quite straightforward.

In fact, I have a feeling travis may provide an Xvfb server by default...

[ris]

(and yes, I can reproduce this now if I run in non-headless mode - will figure it out when I get time)

[ris]

So really the issue here is going to be that back in #15599 I made the assumption that tests were always going to be made in headless mode and used isHeadless() as an indicator that we should pretend the window's showing even though it isn't with methods such as

private static Point findTopLeftOnScreen(JComponent position) {
    if (GraphicsEnvironment.isHeadless()) {
        // in our imaginary universe the window is always (10, 10) from the top left of the screen
        Point topLeftInWindow = findTopLeftInWindow(position);
        return new Point(topLeftInWindow.x + 10, topLeftInWindow.y + 10);
    } else {
        try {
            return position.getLocationOnScreen();
        } catch (JosmRuntimeException | IllegalArgumentException | IllegalStateException e) {
            throw BugReport.intercept(e).put("position", position).put("parent", position::getParent);
        }
    }
}

and

protected boolean isVisibleOnScreen() {
    return GraphicsEnvironment.isHeadless() || (
        SwingUtilities.getWindowAncestor(this) != null && isShowing()
    );
}

so... we're going to have to come up with better ways to flag "this is not on a visible window but please pretend that it is". It's a weird/lucky accident that my previous MinimapDialog tests worked on non-headless.

[ris]

Though I can't figure out how JOSMTestRules.java:347

    protected void before() throws InitializationError, ReflectiveOperationException {
        // Tests are running headless by default.
        System.setProperty("java.awt.headless", "true");
        ...

only sometimes appears to have an effect then. Surely this should force any test that uses JOSMTestRules to be in headless mode. Yet that doesn't seem to happen.

[ris]

(oops)

[ris]

Note - you may want to deprioritize this (rather than bumping the milestone every month) as what I'm doing at the moment is looking deeper into the requirements the project has for testing & mocking infrastructure and what else I would need for me to be able to write the tests I want to be able to write for the features I want to be able to work on.

[Don-vip]

ok thanks :)

[ris]

This is now included in https://josm.openstreetmap.de/ticket/16010