Modify

Opened 6 years ago

Closed 6 years ago

#15630 closed enhancement (duplicate)

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

Reported by: ris Owned by: team
Priority: minor Milestone:
Component: Unit tests Version:
Keywords: minimapdialog tests Cc:

Description

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

Change History (57)

comment:1 by Don-vip, 6 years ago

Milestone: 17.12

comment:2 by Don-vip, 6 years ago

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)

comment:3 by ris, 6 years ago

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?

comment:4 by Don-vip, 6 years ago

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

comment:5 by ris, 6 years ago

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?

comment:6 by ris, 6 years ago

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

comment:7 by ris, 6 years ago

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.

comment:8 by Don-vip, 6 years ago

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

comment:9 by ris, 6 years ago

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.

in reply to:  9 comment:10 by Don-vip, 6 years ago

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

comment:11 by Don-vip, 6 years ago

In 13234/josm:

see #15630 - add AppVeyor configuration file

comment:12 by ris, 6 years ago

That was fast.

/me crosses that off his todo list...

comment:13 by Don-vip, 6 years ago

In 13236/josm:

see #15630 - AppVeyor: fix build

comment:14 by Don-vip, 6 years ago

In 13235/josm:

see #15630 - AppVeyor: specify image & refresh environment

comment:15 by ris, 6 years ago

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

comment:16 by Don-vip, 6 years ago

In 13237/josm:

see #15630 - AppVeyor: fix build

comment:17 by Don-vip, 6 years ago

In 13238/josm:

see #15630 - add Travis configuration file

comment:18 by Don-vip, 6 years ago

In 13239/josm:

see #15630 - CI tweaks

comment:19 by Don-vip, 6 years ago

In 13240/josm:

see #15630 - CI tweaks

comment:20 by ris, 6 years ago

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

comment:21 by Don-vip, 6 years ago

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

comment:22 by ris, 6 years ago

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

comment:23 by ris, 6 years ago

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.

comment:24 by Don-vip, 6 years ago

Milestone: 17.1218.01

comment:25 by ris, 6 years ago

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

comment:26 by Don-vip, 6 years ago

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

comment:27 by ris, 6 years ago

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.

comment:28 by ris, 6 years ago

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

comment:29 by Don-vip, 6 years ago

Both French.

comment:30 by ris, 6 years ago

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)

comment:31 by Don-vip, 6 years ago

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)

comment:32 by ris, 6 years ago

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)

in reply to:  32 comment:33 by Don-vip, 6 years ago

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

comment:34 by ris, 6 years ago

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

comment:35 by Don-vip, 6 years ago

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

comment:36 by ris, 6 years ago

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)

comment:37 by Don-vip, 6 years ago

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

comment:38 by ris, 6 years ago

viewWidth=0 viewHeight=0

aha!

comment:39 by ris, 6 years ago

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

comment:40 by Don-vip, 6 years ago

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

comment:41 by Don-vip, 6 years ago

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)

comment:42 by ris, 6 years ago

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

comment:43 by Don-vip, 6 years ago

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

comment:44 by ris, 6 years ago

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

comment:45 by ris, 6 years ago

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

comment:46 by ris, 6 years ago

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.

comment:47 by Don-vip, 6 years ago

Milestone: 18.0118.02

comment:48 by ris, 6 years ago

Milestone: 18.0218.01

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.

comment:49 by ris, 6 years ago

Milestone: 18.0118.02

(oops)

comment:50 by ris, 6 years ago

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.

comment:51 by Don-vip, 6 years ago

Milestone: 18.02

ok thanks :)

comment:52 by ris, 6 years ago

Resolution: duplicate
Status: newclosed

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.