Modify

Opened 7 weeks ago

Last modified 96 minutes ago

#15630 new enhancement

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

Reported by: ris Owned by: team
Priority: minor Milestone: 18.01
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 (47)

comment:1 Changed 7 weeks ago by Don-vip

Milestone: 17.12

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

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

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

comment:5 Changed 7 weeks ago by 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?

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

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

comment:8 Changed 5 weeks ago by Don-vip

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

comment:9 Changed 4 weeks ago by 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.

comment:10 in reply to:  9 Changed 4 weeks ago by 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 :)

comment:11 Changed 4 weeks ago by Don-vip

In 13234/josm:

see #15630 - add AppVeyor configuration file

comment:12 Changed 4 weeks ago by ris

That was fast.

/me crosses that off his todo list...

comment:13 Changed 4 weeks ago by Don-vip

In 13236/josm:

see #15630 - AppVeyor: fix build

comment:14 Changed 4 weeks ago by Don-vip

In 13235/josm:

see #15630 - AppVeyor: specify image & refresh environment

comment:15 Changed 4 weeks ago by 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)

comment:16 Changed 4 weeks ago by Don-vip

In 13237/josm:

see #15630 - AppVeyor: fix build

comment:17 Changed 4 weeks ago by Don-vip

In 13238/josm:

see #15630 - add Travis configuration file

comment:18 Changed 4 weeks ago by Don-vip

In 13239/josm:

see #15630 - CI tweaks

comment:19 Changed 4 weeks ago by Don-vip

In 13240/josm:

see #15630 - CI tweaks

comment:20 Changed 4 weeks ago by ris

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

comment:21 Changed 4 weeks ago by Don-vip

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

comment:22 Changed 4 weeks ago by 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...?

comment:23 Changed 4 weeks ago by 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.

comment:24 Changed 4 weeks ago by Don-vip

Milestone: 17.1218.01

comment:25 Changed 2 weeks ago by ris

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 Changed 2 weeks ago by 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...

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

comment:28 Changed 13 days ago by 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...

comment:29 Changed 13 days ago by Don-vip

Both French.

Changed 12 days ago by ris

comment:30 Changed 12 days ago by 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)

comment:31 Changed 10 days ago by 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)

comment:32 Changed 10 days ago by 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)

comment:33 in reply to:  32 Changed 9 days ago by 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

comment:34 Changed 6 days ago by ris

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

comment:35 Changed 6 days ago by 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

comment:36 Changed 3 days ago by 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)

comment:37 Changed 3 days ago by 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

comment:38 Changed 3 days ago by ris

viewWidth=0 viewHeight=0

aha!

comment:39 Changed 3 hours ago by ris

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

comment:40 Changed 2 hours ago by 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...

comment:41 Changed 2 hours ago by 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)

comment:42 Changed 96 minutes ago by 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...

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set.
to The owner will be changed from team to the specified user.
The owner will change to ris
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.