Modify

Opened 6 weeks ago

Last modified 5 hours ago

#20433 assigned task

Imagery Integration tests

Reported by: GerdP Owned by: Don-vip
Priority: normal Milestone: Longterm
Component: Unit tests Version:
Keywords: imagery jenkins Cc: Don-vip, stoecker

Description

IIGTR the job is submitted every 6 hours but its runtime is > 7 hours.

Attachments (1)

20433.patch (1.9 KB) - added by taylor.smock 6 weeks ago.
Enable parallel test execution and annotate ImageryPreferenceTestIT#testImageryEntryValidity with @Execution(ExecutionMode.CONCURRENT)

Download all attachments as: .zip

Change History (37)

comment:1 Changed 6 weeks ago by stoecker

Probably should be reduced to once every 24h?

comment:2 Changed 6 weeks ago by Don-vip

Owner: changed from team to Don-vip
Status: newassigned

It's a combination of:

  • regression from JUnit 5 migration which disabled the parallel execution
  • the cumulative increasing number of timeout errors

I changed the cron to once per 24h, and still have the parallel execution on my radar.

Any help to fix imagery sources which timeout will help to reduce the duration of this test and provide working entries to our users.

@Gerd what does "IIGTR" mean?

Last edited 6 weeks ago by Don-vip (previous) (diff)

comment:3 Changed 6 weeks ago by Don-vip

Component: CoreUnit tests

comment:4 Changed 6 weeks ago by GerdP

IIGTR: If I got that right

comment:5 Changed 6 weeks ago by GerdP

reg. help: I have no clue about all the parameters used in the WMS/TMS definitions or how the results of the test help to find what has to be changed :(
Isn't that something that has to be done by those who created the wiki entries?

comment:6 in reply to:  5 Changed 6 weeks ago by Don-vip

Replying to GerdP:

Isn't that something that has to be done by those who created the wiki entries?

You can't expect people who submitted a source several years ago to monitor it daily for production issues, change of url, broken certificates and so on.

comment:7 Changed 6 weeks ago by Don-vip

As for clues... Well for each error you have to dig: has the layer name changed, has the server url changed, has the server been decommissioned, are they blocking German IP addresses, and so on.

comment:8 Changed 6 weeks ago by GerdP

You can't expect people who submitted a source several years ago to monitor it daily for production issues, change of url, broken certificates and so on.

My idea is something like an automatic filtering of the wrong entries so that JOSM can still show the entry but with a flag that it is probably not working because of errors in the wiki definition.

Well for each error you have to dig.

Does that mean you contact someone and ask or do you have tools to find that out?

comment:9 in reply to:  2 ; Changed 6 weeks ago by taylor.smock

Replying to Don-vip:

  • regression from JUnit 5 migration which disabled the parallel execution

Oops. I'll see if I can fix that.

comment:10 in reply to:  9 Changed 6 weeks ago by Don-vip

Replying to taylor.smock:

Oops. I'll see if I can fix that.

It's OK, I should just have to configure the job to add required properties. It's pure Jenkins configuration, I can do it without modifying anything in the source tree I think.

comment:11 Changed 6 weeks ago by taylor.smock

There is a JUnit annotation for running parallel tests.

Unfortunately, it is currently considered experimental.

Anyway, I'll post a patch for it (I finished up the work for the patch about the time you posted).

Its fairly tiny. The hard part (for me) is extracting it from the rest of the stuff I've been modifying for #16567, and then ensuring it applies cleanly (for you).

Changed 6 weeks ago by taylor.smock

Attachment: 20433.patch added

Enable parallel test execution and annotate ImageryPreferenceTestIT#testImageryEntryValidity with @Execution(ExecutionMode.CONCURRENT)

comment:12 Changed 6 weeks ago by Don-vip

Doesn't setting the property at this location enable parallel mode for ALL tests?

Last edited 6 weeks ago by Don-vip (previous) (diff)

comment:13 in reply to:  12 Changed 6 weeks ago by taylor.smock

Replying to Don-vip:

Doesn't setting the property at this location enable parallel mode for ALL tests?

-Djunit.jupiter.execution.parallel.enabled=true just allows the @Execution annotations to be used.

If I added -Djunit.jupiter.execution.parallel.mode.default=concurrent, then yes.

Link to docs: https://junit.org/junit5/docs/5.7.0/user-guide/#writing-tests-parallel-execution (if you want to look at all the various config options).

Last edited 6 weeks ago by taylor.smock (previous) (diff)

comment:14 Changed 6 weeks ago by Don-vip

And of course I totally forgot to mention that I committed r17474 which is the main reason for the recent duration increase.

comment:15 Changed 6 weeks ago by Don-vip

@Taylor ok thank you, I didn't know about the annotation and thought the properties were the only way to go.

comment:16 Changed 6 weeks ago by Don-vip

In 17478/josm:

see #16567 - see #20433 - restore parallel execution of imagery integration test

comment:17 in reply to:  15 Changed 6 weeks ago by taylor.smock

Replying to Don-vip:

@Taylor ok thank you, I didn't know about the annotation and thought the properties were the only way to go.

No problem. There is a lot of new things in JUnit 5, and I am pretty certain I know less than half of the new features. In this case, I assumed that JUnit wouldn't force tests to be all parallel or all sequential. And to be fair, a good portion is marked as experimental (specifically, in this case, the concurrent execution).

Let me know how it worked on the server -- it took ~260 minutes on my machine with and without the patch.

comment:18 Changed 2 weeks ago by Don-vip

Test still takes 7 hours as follows:

  • ImageryPreferenceTestIT.AR-ign-wms => 3h39
  • ImageryPreferenceTestIT.AR-Mapa-Educativo-wms => 1h00
  • 20 tests take more than 1 min
  • 20 tests take between 15s and 1 min

comment:19 Changed 2 weeks ago by Don-vip

In 17517/josm:

see #20433 - don't loop over all tile sources if we face maximum allowed server time

comment:20 Changed 12 days ago by Don-vip

Zalitoar made a great PR to fix Argentina sources: https://github.com/osmlab/editor-layer-index/pull/1058/files

comment:21 Changed 12 days ago by Don-vip

Milestone: Longterm

comment:22 Changed 8 days ago by Don-vip

Ticket #20546 has been marked as a duplicate of this ticket.

comment:23 Changed 8 days ago by mdk

Several tests are failing because of invalid bounding boxes (see #20354).
Are the BBOX values hard coded or calculated?

comment:24 Changed 43 hours ago by GerdP

The job seems to block anything else on Jenkins?

comment:25 Changed 38 hours ago by Don-vip

Yes the Argentina entries make everything hang. You can help me by fixing them. See the ELI PR above.

comment:26 Changed 38 hours ago by Don-vip

Keywords: imagery jenkins added
Summary: Is Jenkins job "JOSM-Imagery-Integration" OK?Imagery Integration tests

comment:27 Changed 34 hours ago by GerdP

How can I help with this PR?

comment:28 Changed 33 hours ago by Don-vip

Review if the changes are OK and update JOSM wiki accordingly.

comment:29 in reply to:  28 ; Changed 19 hours ago by GerdP

Replying to Don-vip:

Review if the changes are OK and update JOSM wiki accordingly.

I fear you are ten steps ahead. I have zil knowledge about this stuff, I just use background images in JOSM and I understand that this is about the entries that I see in the corresponding JOSM menu.
So far I've cloned ​https://github.com/osmlab/editor-layer-index.git and I found https://josm.openstreetmap.de/wiki/Maps#Documentation and started to read, but got lost in all the details.

I failed to download the patch in the "PR" as text, GitHub seems to hide that somehow? I don't know how to use the PR in my local clone cause I don't know how to use git / GitHub etc.
Do I 1st have to learn git to help with this or can I skip this somehow?

Last edited 15 hours ago by stoecker (previous) (diff)

comment:30 Changed 16 hours ago by stoecker

I failed to download the patch in the "PR" as text, GitHub seems to hide that somehow?

If you have a pull URL like ​https://github.com/osmlab/editor-layer-index/pull/1058/files add a .patch or .diff behind the number like ​https://github.com/osmlab/editor-layer-index/pull/1058.patch :-)

comment:31 in reply to:  30 ; Changed 15 hours ago by GerdP

Replying to stoecker:

I failed to download the patch in the "PR" as text, GitHub seems to hide that somehow?

If you have a pull URL like ​https://github.com/osmlab/editor-layer-index/pull/1058/files add a .patch or .diff behind the number like ​https://github.com/osmlab/editor-layer-index/pull/1058.patch :-)

Oh, so very obvious ;)

comment:32 in reply to:  31 Changed 15 hours ago by stoecker

Replying to GerdP:

Replying to stoecker:

I failed to download the patch in the "PR" as text, GitHub seems to hide that somehow?

If you have a pull URL like ​https://github.com/osmlab/editor-layer-index/pull/1058/files add a .patch or .diff behind the number like ​https://github.com/osmlab/editor-layer-index/pull/1058.patch :-)

Oh, so very obvious ;)

There probably is a hidden text or button in the UI somewhere, which links to that as well. I have no idea where...

comment:33 in reply to:  29 Changed 8 hours ago by Don-vip

Replying to GerdP:

Replying to Don-vip:

Review if the changes are OK and update JOSM wiki accordingly.

I fear you are ten steps ahead. I have zil knowledge about this stuff
Do I 1st have to learn git to help with this or can I skip this somehow?

This has nothing to do with Git or GitHub. You just have to look at the changes, review them, report them to JOSM wiki when they're OK. I've disabled the job until we fix at least Argentina sources as it makes Jenkins hang everyday.

comment:34 Changed 6 hours ago by GerdP

OK, I guess I am able to transform the changes in the PR to the syntax used in the JOSM wiki. I still have no clue what actions the review includes.

comment:35 in reply to:  34 ; Changed 5 hours ago by stoecker

Replying to GerdP:

OK, I guess I am able to transform the changes in the PR to the syntax used in the JOSM wiki.

Not necessary. Call "ant imageryindexdownload". You'll get the recent ELI file in our XML format.

I still have no clue what actions the review includes.

Essentially check if the changes improve the situation or are incorrect. Sadly we can't rely on any changes from ELI to be correct.

Any changes which improve the situation should be copied. Everything else added to ignore list.

comment:36 in reply to:  35 Changed 5 hours ago by GerdP

Replying to stoecker:

Replying to GerdP:

OK, I guess I am able to transform the changes in the PR to the syntax used in the JOSM wiki.

Not necessary. Call "ant imageryindexdownload". You'll get the recent ELI file in our XML format.

My understanding is that the PR was not yet applied. I may be wrong with that.

I still have no clue what actions the review includes.

Essentially check if the changes improve the situation or are incorrect. Sadly we can't rely on any changes from ELI to be correct.

Any changes which improve the situation should be copied. Everything else added to ignore list.

Well, that's my problem. I don't know what changes I should look for and how to decide if new is better than old. E.g. I see that the PR removes a line containing "EPSG:4326",
How do I know if this is a good idea or not? It also adds new entries. What has to be done to verify those?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain Don-vip.
as The resolution will be set.
to The owner will be changed from Don-vip 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

Add Comment


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

 
Note: See TracTickets for help on using tickets.