Opened 4 years ago

Last modified 3 years ago

#18204 new defect

Weird results from JoinNodeWayActionTest unit test

Reported by: GerdP Owned by: team
Priority: normal Milestone:
Component: Unit tests Version:
Keywords: Cc: ris


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.

Attachments (1)

18204.patch (2.6 KB) - added by GerdP 4 years ago.
experimental patch

Download all attachments as: .zip

Change History (12)

Changed 4 years ago by GerdP

Attachment: 18204.patch added

experimental patch

comment:1 Changed 4 years ago by Don-vip

Cc: ris added

comment:2 Changed 4 years ago by ris

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


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

Last edited 4 years ago by Don-vip (previous) (diff)

comment:3 Changed 4 years ago by GerdP

getMap() returns null unless a layer is added.
I found that MapFrame is initialised with a hard coded 400x400 size:

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.

comment:4 Changed 4 years ago by ris

You mean the real MapView constructor?

comment:5 Changed 4 years ago by 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.

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

Last edited 4 years ago by ris (previous) (diff)

comment:7 Changed 4 years ago by GerdP

In 15561/josm:

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

comment:8 Changed 4 years ago by 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.

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

comment:10 Changed 4 years ago by GerdP

In 15575/josm:

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

comment:11 Changed 3 years ago by simon04

Summary: Weird results from unit testWeird results from JoinNodeWayActionTest unit test

Modify Ticket

Change Properties
Set your email in Preferences
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 GerdP
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.