Weird results from JoinNodeWayActionTest unit test

[GerdP]

Follow up of ticket:18189#comment:17
A unit test that tests a method which uses MainApplication.getMap().mapView may run into trouble when it doesn't use something like

        // setup a reasonable screen size
        MainApplication.getMap().mapView.setBounds(new Rectangle(1920, 1080));

before executing code that uses e.g. NavigatableComponent.getNearestWaySegments()
Without the above statement the height and width of the mapView are 0 and class NavigatableComponent doesn't seem to care about this.

I've written a patch to log an error when NavigatableComponent.getNearestWaySegments() or
NavigatableComponent.getNearestNodes() is used with a very small mapView but other methods may also suffer.
I'd prefer to have a reasonable value for width and height when unit tests are executed but I have no idea where this has to be implemented.

[GerdP]

experimental patch

[ris]

I'm slightly surprised you didn't also find yourself needing to also call

mapView.addNotify();
mapView.doLayout();

to get things to start behaving themselves, but I guess your tests must skirt around needing that.

If you wanted to add something like this, the place to look would be somewhere around https://josm.openstreetmap.de/browser/josm/trunk/test/unit/org/openstreetmap/josm/testutils/JOSMTestRules.java#L577 . It might seem like it might best fit in one of the methods like JOSMFixture.initContentPane() or JOSMFixture.initMainPanel(), but note people keep saying they want to get rid of JOSMFixture entirely.

One thing people probably don't want to do is add any setup steps that will slow down the 99% of tests that don't need this, but I expect you'd be safe simply adding a setBounds call...?

[GerdP]

@ris
getMap() returns null unless a layer is added.
I found that MapFrame is initialised with a hard coded 400x400 size:
https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/MapFrame.java#L200

A simple solution for the problem would be to do the same in the MapView constructor.

I see no performance issues as the setBounds() call simply sets the two int fields. Also, the MapView constructor isn't called frequently.

[ris]

You mean the real MapView constructor?

[GerdP]

Yes. I'm struggling with my Eclipse installation, the debugging ignores some breakpoints, so I still try to understand the effect of this change, but it did not seem to change the normal JOSM execution, as this overwrites the values again with real screen values.

[ris]

Yeah, my position on this is the same as it was in comment 2. As for "random failures", as much as I tried to remove inter-test dependencies and side effects, I wasn't totally successful and I'm certain a few of these remain. I mentioned a way of trying to narrow this down in https://josm.openstreetmap.de/ticket/16796#comment:48

[GerdP]

In 15561/josm:

see #18204: add code to initialize mapview to prevent random unit test results

[GerdP]

In 15563/josm:

see #18204: revert r15561 which didn't help, add .projection() instead.
If this also doesn't help I'll disable the tests as I really don't know how to find out what's wrong here.

[ris]

Yes, I wouldn't be surprised if this test needed .projection().

Incidentally this is why I put a lot of work into getting pre-merge tests working in github with travis and appveyor (​https://travis-ci.org/openstreetmap/josm/ ​https://ci.appveyor.com/project/don-vip/josm), so we had ways of fixing the CI that didn't involve experimental permanent commits. In 2019, the way of getting code merged to a project is to open a "pull request"/"merge request"/etc, which will get multi-platform CI run on it from the moment the ticket is opened. And importantly, merge will not be allowed until the CI passes. I can't even begin to describe what this workflow has done for my productivity in recent years.

I can't say what the current status of the travis/appveyor support is - I stopped because I just didn't get buy-in from anyone else and it doesn't work as a one-man-battle.

[GerdP]

In 15575/josm:

see #18204: - ignore tests until a fix is provided