Modify

Opened 3 years ago

Closed 3 years ago

#21150 closed enhancement (fixed)

[PATCH] Add JUnit5 annotation for WireMockServer

Reported by: taylor.smock Owned by: Don-vip
Priority: normal Milestone: 21.07
Component: Unit tests Version:
Keywords: junit5 Cc:

Description (last modified by taylor.smock)

Add a @BasicWiremock annotation for use by tests

PR available at https://github.com/openstreetmap/josm/pull/73 or https://gitlab.com/smocktaylor/josm/-/merge_requests/9 .

Attachments (1)

21150.patch (127.0 KB ) - added by taylor.smock 3 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by taylor.smock, 3 years ago

Owner: changed from taylor.smcok to taylor.smock
Status: assignednew

comment:2 by taylor.smock, 3 years ago

Description: modified (diff)
Summary: [WIP PATCH] Add JUnit5 annotation for WireMockServer[PATCH] Add JUnit5 annotation for WireMockServer

comment:4 by taylor.smock, 3 years ago

OK. Everything is passing in the workflow for https://github.com/openstreetmap/josm/pull/73 except for the following actions:

  • PMD (failing due to "HttpError: Resource not accessible by integration")
  • Checkstyle (see above)
  • Java 8 on Windows test (failing on master, exact same test (org.openstreetmap.josm.data.projection.ProjectionRegressionTest), see https://github.com/openstreetmap/josm/runs/3127318161 -- I wonder if something changed in the Windows runners in the past two days?)

by taylor.smock, 3 years ago

Attachment: 21150.patch added

in reply to:  4 ; comment:5 by Don-vip, 3 years ago

Replying to taylor.smock:

  • PMD (failing due to "HttpError: Resource not accessible by integration")
  • Checkstyle (see above)

Seems to be caused by GitHub security that prevents people without write access to the repo do do many things. Not sure how to fix it, I will disable these tasks for PR until we move the repo to JOSM org and investigate further.

This test is very sensible to the Math functions implementation, especially Math.cos (which is a native lib in the JRE), so
yes, even a Windows update could have an impact. There was a huge change of Math implementation in Java 9 (switch of library) so that would explain why only Java 8 fails. I'll see how to disable this test in this particular environment, as it works fine with recent versions of Java.

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

in reply to:  5 ; comment:6 by taylor.smock, 3 years ago

Replying to Don-vip:

Seems to be caused by GitHub security that prevents people without write access to the repo do do many things. Not sure how to fix it, I will disable these tasks for PR until we move the repo to JOSM org and investigate further.

I kind of figured that the PMD/checkstyle tasks could be ignored, as long as they actually passed locally (AFAIK, there should be no differences from environment on those two, so if it passes locally, all is good). I've got a .gitlab-ci pipeline we could split apart just to be able to run PMD/checkstyle on forks, if needed (see https://gitlab.com/smocktaylor/josm/-/merge_requests/1, but ignore the test-java job -- its running over an hour, which causes GitLab to kill it).

This test is very sensible to the Math functions implementation, especially Math.cos (which is a native lib in the JRE), so
yes, even a Windows update could have an impact. There was a huge change of Math implementation in Java 9 (switch of library) so that would explain why only Java 8 fails. I'll see how to disable this test in this particular environment, as it works fine with recent versions of Java.

I kind of figured, once I went and did some investigation (it doesn't make sense for a very simple change (fd2d90316d45548b8695496a23c187e7cbf5ed22) would cause CI to fail, especially when it shouldn't affect any of the code for the failing test).

in reply to:  6 comment:7 by Don-vip, 3 years ago

Replying to taylor.smock:

I kind of figured, once I went and did some investigation (it doesn't make sense for a very simple change (fd2d90316d45548b8695496a23c187e7cbf5ed22) would cause CI to fail, especially when it shouldn't affect any of the code for the failing test).

I think the test environment changed around the same time of this commit, but the issue is likely unrelated.

comment:8 by Don-vip, 3 years ago

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

comment:9 by Don-vip, 3 years ago

Resolution: fixed
Status: assignedclosed

In 18106/josm:

fix #21150 - Add JUnit5 annotation for WireMockServer (patch by taylor.smock)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
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.