#20621 closed defect (fixed)
GitHub pull requests should trigger CI
Reported by: | simon04 | Owned by: | simon04 |
---|---|---|---|
Priority: | normal | Milestone: | 21.03 |
Component: | Git mirror | Version: | |
Keywords: | template_report regression ci | Cc: | Stereo, Don-vip |
Description
What steps will reproduce the problem?
- Work on a large feature/patch
- Open a GitHub pull request
What is the expected result?
CI via GitHub Actions runs
What happens instead?
CI via GitHub Actions does not run
Please provide any additional information below. Attach a screenshot if possible.
Attachments (0)
Change History (51)
comment:1 by , 4 years ago
comment:2 by , 4 years ago
Why is the release necessary for running unit tests? I'd be happy to have all tests run on Java 8 / Linux / headless.
I'm always very stressed out when a few unit tests fail after doing an SVN commit. I'd like to know this in advance, instead.
The same hods true when reviewing patches: you never know which tests fail (unless you're spending 30min to have the tests run locally, being blocked for everything else).
comment:3 by , 4 years ago
The two are indeed separate things, but I thought it'd be nice to have a jar to try out your nice new feature?
comment:4 by , 4 years ago
I don't care about the generated JAR. Btw, do we need to create a new release for every CI run? (Having 231 temporary releases on https://github.com/openstreetmap/josm/releases does not seem to be overly helpful to me in the long run.)
comment:5 by , 4 years ago
It's certainly useful to have builds to ask someone to try, and more convenient to have a full .app (or exe/deb/rpm) than just a nightly .jar you have to wait 24 hours for.
We could clean up old pre-releases, if you wanted to. Not many benefits to us.
follow-up: 7 comment:6 by , 4 years ago
Cc: | added |
---|---|
Keywords: | regression ci added |
Type: | enhancement → defect |
Actually, GitHub Actions start to run on pull requests, but fail rather quickly since createrelease fails, see https://github.com/openstreetmap/josm/pull/70/checks?check_run_id=2144212270
This is a regression of one of the patches applied via #20146, likely r17373.
I'd vote to split .github/workflows/ant.yml into:
- an action that runs
ant test
(using the java/os/headless matrix), and - an action that creates the GitHub releases, and builds various jar/app files (only picking one java version, only headless).
comment:7 by , 4 years ago
Replying to simon04:
I'd vote to split .github/workflows/ant.yml into:
- an action that runs
ant test
(using the java/os/headless matrix), and- an action that creates the GitHub releases, and builds various jar/app files (only picking one java version, only headless).
Agreed, those are two very different things. Go ahead :)
comment:8 by , 4 years ago
Milestone: | → 21.03 |
---|
comment:9 by , 4 years ago
@Stereo, would you be interested in unscrambling tests and builds/releases via GitHub Actions.
Btw: GitHub Actions supports storing workflow data as artifacts (this might be a nice alternative to the temporary releases)
comment:10 by , 4 years ago
Milestone: | 21.03 → 21.04 |
---|
comment:11 by , 4 years ago
Before we extend CI on Github could it be fixed first? The multi-daily failure mails are really disturbing.
comment:12 by , 4 years ago
Yeah, sigh, ... Debugging the CI is supper annoying due to #19889.
I don't want to add more CI actions, but to separate the exiting actions into test and build/deploy.
Maybe we should disable every non-working environment (Java/OS/headless combination) for now, and re-enable them step by step?
Btw: Appveyor is testing every commit twice due to the GitHub pre-release being created for each commit – https://ci.appveyor.com/project/don-vip/josm/history
comment:13 by , 4 years ago
We actually already use artifacts to upload ant test reports. See the bottom of https://github.com/openstreetmap/josm/actions/runs/697729205 for example.
I wouldn't mind working on it, but I would like to please get svn access. It's just really tedious otherwise.
comment:16 by , 4 years ago
Any idea why DownloadWmsAlongTrackActionTest.testTMSLayer
fails on GitHub Actions? -> #20695
follow-up: 18 comment:17 by , 4 years ago
Any idea what crashes the JVM in "Java 8 on windows-latest"?
[junitlauncher] # A fatal error has been detected by the Java Runtime Environment: [junitlauncher] # [junitlauncher] # EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x0000000067c29b1d, pid=6172, tid=0x0000000000000ef4 [junitlauncher] # [junitlauncher] # JRE version: OpenJDK Runtime Environment (Zulu 8.52.0.23-CA-win64) (8.0_282-b08) (build 1.8.0_282-b08) [junitlauncher] # Java VM: OpenJDK 64-Bit Server VM (25.282-b08 mixed mode windows-amd64 compressed oops) [junitlauncher] # Problematic frame: [junitlauncher] # V [jvm.dll+0x129b1d] [junitlauncher] # [junitlauncher] # Core dump written. Default location: D:\a\josm\josm\hs_err_pid6172.mdmp [junitlauncher] # [junitlauncher] # An error report file with more information is saved as: [junitlauncher] # D:\a\josm\josm\hs_err_pid6172.log [junitlauncher] # [junitlauncher] # If you would like to submit a bug report, please visit: [junitlauncher] # http://www.azulsystems.com/support/ https://github.com/simon04/josm/runs/2225967553#step:7:5214
comment:18 by , 4 years ago
Replying to simon04:
Any idea what crashes the JVM in "Java 8 on windows-latest"?
Probably this one:
It seems they never backported the fix to Java 8...
comment:19 by , 4 years ago
Workaround would be to call Toolkit.initIDs()
in test code (using reflection), when tests are run on Windows, with Java 8 and headless mode.
comment:20 by , 4 years ago
Thank you, let's see: https://github.com/simon04/josm/commit/7184b2cb83b08309650277b9ecd41e2d97966683
comment:21 by , 4 years ago
Milestone: | 21.04 → 21.03 |
---|
comment:24 by , 4 years ago
@stoecker 17693/josm means that your famous build script must be updated to look for the github job with the name "Java CI Build".
follow-up: 26 comment:25 by , 4 years ago
@stoecker, @Don-vip, are we ready for the milestone:21.03 release? I've just performed a (final?) i18n update, r17699.
@stoecker, have you updated the macOS linking script? Shall I revert the GitHub Actions name change from r17694?
follow-up: 27 comment:26 by , 4 years ago
Replying to simon04:
@stoecker, @Don-vip, are we ready for the milestone:21.03 release? I've just performed a (final?) i18n update, r17699.
Did you fix today's i18n issues first?
comment:27 by , 4 years ago
comment:29 by , 4 years ago
I fixed the Dutch issues (typical missing quotes) but can't fix the Arabic ones. I asked for help in #18856. All i18n issues must be solved before we do a release.
comment:30 by , 4 years ago
Priority: | normal → blocker |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Just marking this as blocker to make sure the build script on the josm.openstreetmap.de re-runs the right macOS CI job by name.
follow-up: 32 comment:31 by , 4 years ago
I've fixed a bunch of '
errors in Italian:
[exec] JAVA translation issue for language it: Mismatching single quotes: [exec] Translated text: Causato dall'attività umana [exec] Original text: Caused by human activity [exec] JAVA translation issue for language it: Mismatching single quotes: [exec] Translated text: L'incrocio non ha un nome [exec] Original text: Junction has no name [exec] JAVA translation issue for language it: Mismatching single quotes: [exec] Translated text: L'incrocio non ha un riferimento [exec] Original text: Junction has no reference [exec] JAVA translation issue for language it: Mismatching single quotes: [exec] Translated text: Modificato dall'uomo [exec] Original text: Modified by human hand [exec] JAVA translation issue for language it: Mismatching single quotes: [exec] Translated text: torre dell'orologio
The Arabic translations seem to rewrite the {x}
with a corresponding spelled-out word:
[exec] JAVA translation issue for language ar (3): Mismatching format entries: [exec] Translated text: تحديد {0}={1} لعنصرين [exec] Original text: Set {0}={1} for {2} object
"تحديد {0}={1} لعنصرين" = "Setting {0} = {1} of two objects"
I'd love to do a release without waiting for approx. 10 strings out of 9702 out of 41 languages to be fixed. Should I do a final i18n update now?
follow-up: 33 comment:32 by , 4 years ago
Replying to simon04:
The Arabic translations seem to rewrite the
{x}
with a corresponding spelled-out word:
I don't know what we should do about that. @Dirk, any idea?
comment:33 by , 4 years ago
follow-up: 38 comment:34 by , 4 years ago
P.S. I wouldn't delay a release because of these warnings ;-)
follow-up: 36 comment:35 by , 4 years ago
Very good. I'll do a final i18n update and then we're ready for the milestone:21.04 release...
@Dirk, what about comment:24? Will you update the script? Shall I revert the name change?
comment:36 by , 4 years ago
@Dirk, what about comment:24? Will you update the script? Shall I revert the name change?
Already done. We'll see if it works.
comment:37 by , 4 years ago
Priority: | blocker → normal |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
Perfect, thank you :)
follow-up: 41 comment:38 by , 4 years ago
Replying to stoecker:
P.S. I wouldn't delay a release because of these warnings ;-)
I thought it might provoke a crash when using Arabic if we include these erroneous strings? What's the behaviour then, the string is just not translated?
comment:39 by , 4 years ago
No crash :-)
-
test/unit/org/openstreetmap/josm/tools/I18nTest.java
diff --git a/test/unit/org/openstreetmap/josm/tools/I18nTest.java b/test/unit/org/openstreetmap/josm/tools/I18nTest.java index 4ac87849e..9e5e28f4d 100644
a b class I18nTest { 19 19 assertEquals("'{'foo''bar'}'", I18n.escape(foobar)); 20 20 assertEquals(foobar, I18n.tr(I18n.escape(foobar))); 21 21 } 22 23 @Test 24 void testAr() { 25 I18n.set("ar"); 26 for (int i = 0; i < 20; i++) { 27 String trn = I18n.trn("Set {0}={1} for {2} object", "Set {0}={1} for {2} objects", i, "foo", "bar", i); 28 System.out.println(trn); 29 } 30 } 22 31 }
تحديد foo=bar للا عنصر تحديد foo=bar لعنصر واحد تحديد foo=bar لعنصرين تحديد foo=bar لـ ٣ عناصر تحديد foo=bar لـ ٤ عناصر تحديد foo=bar لـ ٥ عناصر تحديد foo=bar لـ ٦ عناصر تحديد foo=bar لـ ٧ عناصر تحديد foo=bar لـ ٨ عناصر تحديد foo=bar لـ ٩ عناصر تحديد foo=bar لـ ١٠ عناصر تحديد foo=bar لـ ١١ عنصرًا تحديد foo=bar لـ ١٢ عنصرًا تحديد foo=bar لـ ١٣ عنصرًا تحديد foo=bar لـ ١٤ عنصرًا تحديد foo=bar لـ ١٥ عنصرًا تحديد foo=bar لـ ١٦ عنصرًا تحديد foo=bar لـ ١٧ عنصرًا تحديد foo=bar لـ ١٨ عنصرًا تحديد foo=bar لـ ١٩ عنصرًا Process finished with exit code 0
comment:40 by , 4 years ago
OK perfect! Sorry, I really thought it was a release-blocker problem, glad to see I was wrong.
follow-up: 42 comment:41 by , 4 years ago
Replying to Don-vip:
Replying to stoecker:
P.S. I wouldn't delay a release because of these warnings ;-)
I thought it might provoke a crash when using Arabic if we include these erroneous strings? What's the behaviour then, the string is just not translated?
As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.
follow-up: 43 comment:42 by , 4 years ago
Replying to stoecker:
As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.
But we can't let things as it is now: it becomes impossible to spot the other problems, and I would like to get Jenkins job to go back to stable: https://josm.openstreetmap.de/jenkins/job/JOSM-i18n/2071/parsed_console/
follow-up: 44 comment:43 by , 4 years ago
Replying to Don-vip:
Replying to stoecker:
As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.
But we can't let things as it is now: it becomes impossible to spot the other problems, and I would like to get Jenkins job to go back to stable: https://josm.openstreetmap.de/jenkins/job/JOSM-i18n/2071/parsed_console/
Correct. The entries must be fixed accordingly, but it's not critical.
comment:44 by , 4 years ago
Replying to stoecker:
Correct. The entries must be fixed accordingly, but it's not critical.
OK, but how? :D I'm not sure to understand the problem fully, but Arabic has a special plural rule which means "two"? How can we add the {}
parameter for this plural form?
comment:45 by , 4 years ago
r17702 is now the new tested but it doesn't seem to tag latest GitHub release.
comment:46 by , 4 years ago
https://www.githubstatus.com/
- Update - GitHub Actions is experiencing degraded performance. We are still investigating and will provide an update when we have one. Apr 1, 21:36 UTC
- Investigating - We are investigating reports of degraded performance for GitHub Packages. Apr 1, 21:30 UTC
follow-up: 49 comment:47 by , 4 years ago
I think it just runs in nightly cron at 3am. Will check tomorrow morning, good night!
comment:48 by , 4 years ago
Good Morning, yay, it's released on GitHub: https://github.com/openstreetmap/josm/releases/tag/17702-tested
comment:49 by , 4 years ago
Replying to Don-vip:
I think it just runs in nightly cron at 3am. Will check tomorrow morning, good night!
No. It simply needs extremely long :-)
comment:51 by , 4 years ago
It seems GitHub CI for Pull Requests is used for Crypto Mining malware.
https://www.heise.de/news/Cyberkriminelle-nutzen-GitHub-Actions-zum-Schuerfen-von-Kryptogeld-6005322.html
So you'd like all the unit tests to run on a github PR? I like that idea.
It currently fails because there will always already have a release based on the svn commit number.
We could have synthetic release numbers, like 12345-simon04:LinkEnvironment but I have no idea how the various bits would react to the build id not being an int.