Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICU-22721 Have the testdata folder trigger CI actions for both icu4c & icu4j #3199

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

mihnita
Copy link
Contributor

@mihnita mihnita commented Sep 21, 2024

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22721
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

Copy link
Member

@cjchapman cjchapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look fine to me, but they don't fix the issue withmvn verify failing CoreTest locally on Rich's and my machines that I reported in the ICU-22906 Jira ticket.

@markusicu markusicu changed the title ICU-22906 The testdata folder to trigger CI actions for both icu4c & icu4j ICU-22906 Have the testdata folder trigger CI actions for both icu4c & icu4j Sep 23, 2024
@markusicu
Copy link
Member

  • I made the PR title a little less mystifying (more like the commit message)
  • who should be the assignee (=main reviewer)?
  • the ticket is for ICU 76, but we are past the code freeze; are you going to ask for an exception, or can this slip to 77?

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

who should be the assignee (=main reviewer)?

I propose Elango?
He added the initial filter for separating icu4c/icu4j, so he is familiar with the space.

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

the ticket is for ICU 76, but we are past the code freeze; are you going to ask for an exception, or can this slip to 77?

I would ask for an exception.
At any time some fixes something in MF2 that touches the shared tests
(lets say some emergency between RC and release)
there is a risk that it will break "the other side" (icu4c <=> icu4j)

Because the testdata is not triggering builds for both icu4c and icu4j, as it should.


Is this "code" though?
It is nothing we ship, and does not affects the binaries of anything we ship.

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

These changes look fine to me, but they don't fix the issue with mvn verify failing CoreTest locally on Rich's and my machines that I reported in the ICU-22906 Jira ticket.

I don't see how this can still fail.
I tried it and I can't reproduce it.

We can try to see what is happening in your / Rich cases.
But there is no change that I can imagine one can do in the code base that would fix that.

I think that the cause of those failure was fixed.

This change tries to prevent that from happening again.

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

Some thinks to try on machines that are still failing

My best guess is that the failure happens in a feature branch, and that bringing that up to date with main would solve the issues.

  1. if it is a branch in a personal (forked) repo, update that personal repo
    (GitHub https://github.com/<user>/icu, "Synk fork"
  2. Synk local clone
    Go to the repo folder and git checkout main
    in icu4j run mvn verify
  3. If step 2 works, bring the feature branch up to date main
    git checkout <my branch> then git merge master

  • if the
  • try (in the failing repo) to checkout the main branch, git pull and try mvn verify again
  • if it works on main but fails in a branch, try bringing the branch up to date with main
  • mvn clean in the icu4j folder
  • remove the ~/.m2/releases folder (the maven local repo), which works like a maven cache
  • git clone in a new folder and build there, making sure the current branch is main

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

I talked with Elango over breakfast, and assigned this to him.

@cjchapman
Copy link
Member

I tried this with a fresh clone of icu this morning, and I still get the error:

% cd ~/tmp
% git clone https://github.com/unicode-org/icu.git
...
% cd icu/icu4j 
% mvn verify
...
[INFO] Running com.ibm.icu.dev.test.message2.CoreTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.026 s <<< FAILURE! -- in com.ibm.icu.dev.test.message2.CoreTest
[ERROR] com.ibm.icu.dev.test.message2.CoreTest.test -- Time elapsed: 0.026 s <<< FAILURE!
org.junit.ComparisonFailure: UnitTest {src=[Default int64: {$val}!], params=[Lcom.ibm.icu.dev.test.message2.Param;@22cdc378, exp="Default int64: 1.234.567.890.123.456.770!"} expected:<...234.567.890.123.456.[77]0!> but was:<...234.567.890.123.456.[80]0!>
	at org.junit.Assert.assertEquals(Assert.java:117)
	at com.ibm.icu.dev.test.message2.TestUtils.runTestCase(TestUtils.java:177)
	at com.ibm.icu.dev.test.message2.TestUtils.runTestCase(TestUtils.java:136)
	at com.ibm.icu.dev.test.message2.CoreTest.test(CoreTest.java:55)
...

Here's the full log:
mvn_verify_fail_20240924.txt

@cjchapman
Copy link
Member

Oh, I missed the .m2 part, I'll try that now.

@cjchapman
Copy link
Member

I got the same thing when I renamed the .m2 folder:

% mv .m2 renamed_m2
% cd tmp
% rm -rf icu
% git clone https://github.com/unicode-org/icu.git
...
% cd icu/icu4j 
% mvn verify
...
[INFO] Running com.ibm.icu.dev.test.message2.CoreTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.027 s <<< FAILURE! -- in com.ibm.icu.dev.test.message2.CoreTest
[ERROR] com.ibm.icu.dev.test.message2.CoreTest.test -- Time elapsed: 0.027 s <<< FAILURE!
org.junit.ComparisonFailure: UnitTest {src=[Default int64: {$val}!], params=[Lcom.ibm.icu.dev.test.message2.Param;@22cdc378, exp="Default int64: 1.234.567.890.123.456.770!"} expected:<...234.567.890.123.456.[77]0!> but was:<...234.567.890.123.456.[80]0!>
...

Here's the full log:
mvn_verify_fail_20240924_renamed_m2.txt

@markusicu
Copy link
Member

the ticket is for ICU 76, but we are past the code freeze; are you going to ask for an exception, or can this slip to 77?

I would ask for an exception.

sgtm, noted in the agenda doc. now you just need to satisfy your reviewers :-P

@cjchapman
Copy link
Member

@markusicu and @mihnita Just to be clear: I'm fine with the changes in this PR, I just don't want the original JIRA issue to be closed until Rich's and my original problem is fixed.

Copy link
Contributor

@echeran echeran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the changes in this PR, I just don't want the original JIRA issue to be closed until Rich's and my original problem is fixed.

We are past code freeze, in an ask-for-permission phase.
If the problem described in the ticket is not resolved by this PR, then we should not merge this PR for that ticket.
Otherwise our process forces us to close the ticket for 76 and clone it for later -- but then we would also have to rewrite the closed ticket to reflect what was actually done here.

@mihnita mihnita changed the title ICU-22906 Have the testdata folder trigger CI actions for both icu4c & icu4j ICU-22721 Have the testdata folder trigger CI actions for both icu4c & icu4j Sep 24, 2024
@mihnita mihnita force-pushed the mihai_workflow_testdata branch from 4c35760 to 22f504f Compare September 24, 2024 18:40
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@mihnita mihnita merged commit 9fb6dca into unicode-org:main Sep 24, 2024
101 checks passed
@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

Thank you for the approval.
Mihai

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

Here's the full log: mvn_verify_fail_20240924_renamed_m2.txt

Thank you, that helped (I built on my side and compared).

And I was able to reproduce it.

It works with JDK 8, 11, 17, which is what I used and what we test in GitHub CI.

And it fails with JDK 21.

I am looking into the cause, and possible solution.

But in the meantime:

  • Checked with Markus, and he said this would not be an RC blocker
  • BUT we want a fix before the release
  • And also before release I will add JDK 21 to the test in the GitHub CI
    We already have an issue created for it (https://unicode-org.atlassian.net/browse/ICU-22336)

@cjchapman
Copy link
Member

@mihnita I'm glad you're able to reproduce it now. I'll be happy to test the fix when the time comes. Thanks for digging into this, and thanks again for the work-around you provided me so I could run the time zone tests locally!

@markusicu
Copy link
Member

We should test ICU 76 at least also with JDK 21.

That might want a separate ticket from this one which asked for “the latest JDK version”.

@mihnita
Copy link
Contributor Author

mihnita commented Sep 24, 2024

We should test ICU 76 at least also with JDK 21.
That might want a separate ticket from this one which asked for “the latest JDK version”.

I created https://unicode-org.atlassian.net/browse/ICU-22919
And I will claim it.

@mihnita mihnita deleted the mihai_workflow_testdata branch September 25, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants