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-22505 Ensure default TZ remains unchanged by each test #2670

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Oct 12, 2023

Summary:

  1. We extended the test framework base class so that we can automatically check that every test restores the default time zone to what it was when it began (both JDK and ICU TZ)
  2. @mihnita checked, and the behavior of JDK TimeZone.getDefault() always checks the latest value, but ICU TimeZone.getDefault() gets whatever is cached.
  3. All of the tests had already been properly restoring the default time zone. So what we think is that somewhere in the main (aka runtime) code is setting the ICU TimeZone, and it's throwing off our assumptions that the test code starts off with a proper and undisturbed state.
  4. To counteract the problem, we are setting the ICU default TimeZone before every test to be the same default JDK TimeZone that is used for our testing purposes. Even though it might mask the source of the underlying problem, perhaps in our main code, it gets our tests working and enabled again.
  5. This buys us time for ICU 75 to do a proper investigation and fix for whatever the source of the underlying problem is.
  6. While we were at it, because we could, we also added checks to ensure that tests aren't changing the default locale (both ICU and JDK). It turns out that a couple didn't restore the default locale, so we fixed that, even though it wasn't causing test failures yet.

As @mihnita mentioned in the ICU-TC meeting on Thursday, the key to reproducing the problem was when he realized that there are 2 different systems for setting the default time zone on Linux, and Debian and Ubuntu differ on which one they prefer in their desktop UIs. He wrote:

This is the "official" way if you search for changing time zone on Debian:
sudo timedatectl set-timezone "America/Los_Angeles"
Does nothing for Java. Seems to be in sync with the changes you do in UI.


The Ubuntu way (also works on Debian), and affects the Java behavior:
sudo dpkg-reconfigure tzdata

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22505
  • 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
Contributor

@mihnita mihnita left a comment

Choose a reason for hiding this comment

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

Reviewed. Thanks.

@mihnita
Copy link
Contributor

mihnita commented Oct 12, 2023

It looks like this PR only adds the detection, but does not fix anything
(I don't see test classes updated to restore a "damaged" default timezone).
Want to do it in 2 steps?

mihnita
mihnita previously approved these changes Oct 13, 2023
Copy link
Contributor

@mihnita mihnita left a comment

Choose a reason for hiding this comment

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

Thank you very much!
Sorry for the delay, it was a lot bigger than I thought :-)

@echeran echeran marked this pull request as ready for review October 13, 2023 03:26
@echeran
Copy link
Contributor Author

echeran commented Oct 13, 2023

@markusicu @yumaoka @roubert @hugovdm I'll wait a little bit before squashing so you can have a chance to review commit by commit, if you want. Despite the number of files touched in this PR, I don't think much of it is interesting beyond CoreTestFmwk.java, maybe TestFmwk.java, and this commit 7f8cf22 that re-enables tests that were previously failing due to the mismatch in the default JDK timezone.

Summary:

  1. We extended the test framework base class so that we can automatically check that every test restores the default time zone to what it was when it began (both JDK and ICU TZ)
  2. @mihnita checked, and the behavior of JDK TimeZone.getDefault() always checks the latest value, but ICU TimeZone.getDefault() gets whatever is cached.
  3. All of the tests had already been properly restoring the default time zone. So what we think is that somewhere in the main (aka runtime) code is setting the ICU TimeZone, and it's throwing off our assumptions that the test code starts off with a proper and undisturbed state.
  4. To counteract the problem, we are setting the ICU default TimeZone before every test to be the same default JDK TimeZone that is used for our testing purposes. Even though it might mask the source of the underlying problem, perhaps in our main code, it gets our tests working and enabled again.
  5. This buys us time for ICU 75 to do a proper investigation and fix for whatever the source of the underlying problem is.
  6. While we were at it, because we could, we also added checks to ensure that tests aren't changing the default locale (both ICU and JDK). It turns out that a couple didn't restore the default locale, so we fixed that, even though it wasn't causing test failures yet.

@markusicu
Copy link
Member

Summary:

Please hoist this summary up to the PR description for easier discovery later.

  1. This buys us time for ICU 75 to do a proper investigation and fix for whatever the source of the underlying problem is.

Ticket?

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.

@echeran @mihnita praise: very nice piece of work! thank you!

@roubert
Copy link
Member

roubert commented Oct 13, 2023

I'm sorry to say, but I just fetched the echeran/ICU-22505 branch into my own repository and ran mvn test and it still failed in common_tests, just like it does at HEAD. The default timezone on my machine is Europe/Zurich and it's running current Debian testing.

@markusicu
Copy link
Member

markusicu commented Oct 13, 2023

I'm sorry to say, but I just fetched the echeran/ICU-22505 branch into my own repository and ran mvn test and it still failed in common_tests, just like it does at HEAD. The default timezone on my machine is Europe/Zurich and it's running current Debian testing.

This is good... for locally reproducing! Can you load it into an IDE and debug and tell us which test fails why?

Copy link
Contributor

@hugovdm hugovdm left a comment

Choose a reason for hiding this comment

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

Tests are also passing on my machine on commit 0af450b. 👍
(I did need an "mvn clean", interestingly - due to my previous build from my own branch.)

@echeran
Copy link
Contributor Author

echeran commented Oct 15, 2023

Tests are also passing on my machine on commit 0af450b. 👍 (I did need an "mvn clean", interestingly - due to my previous build from my own branch.)

Great to hear, and thanks for the info on needing to use mvn clean. @mihnita and I were eventually able to reproduce the problem and then see that the PR fixes it locally, and since we are using the same setup as @roubert, your info might help us diagnose what the discrepancy is.

As @mihnita mentioned in the ICU-TC meeting on Thursday, the key to reproducing the problem was when he realized that there are 2 different systems for setting the default time zone on Linux, and Debian and Ubuntu differ on which one they prefer in their desktop UIs. He wrote:

This is the "official" way if you search for changing time zone on Debian:
sudo timedatectl set-timezone "America/Los_Angeles"
Does nothing for Java. Seems to be in sync with the changes you do in UI.


The Ubuntu way (also works on Debian), and affects the Java behavior:
sudo dpkg-reconfigure tzdata

@echeran
Copy link
Contributor Author

echeran commented Oct 15, 2023

Please hoist this summary up to the PR description for easier discovery later.

Done.

Ticket?

https://unicode-org.atlassian.net/browse/ICU-22550

@roubert
Copy link
Member

roubert commented Oct 16, 2023

This is good... for locally reproducing! Can you load it into an IDE and debug and tell us which test fails why?

I've now taken a closer look at what exactly fails in common_tests for me and it seems like it's unrelated to timezones and this PR, the failure is in TestDataDrivenJDK and I can reproduce it both at HEAD and at this PR like this:

$ mvn test -Dtest='com.ibm.icu.dev.test.format.NumberFormatDataDrivenTest#TestDataDrivenJDK' -Dsurefire.failIfNoSpecifiedTests=false

If I delete that test case then mvn test finishes successfully on my machine, both at HEAD and at this PR.

@markusicu
Copy link
Member

I've now taken a closer look at what exactly fails in common_tests for me and it seems like it's unrelated to timezones and this PR, the failure is in TestDataDrivenJDK and I can reproduce it both at HEAD and at this PR like this:
...
If I delete that test case then mvn test finishes successfully on my machine, both at HEAD and at this PR.

Good to know!
This test is known to be sensitive to changes in JDK behavior, and depends on using or avoiding certain kinds of JDKs.
It sounds like this PR is good to go then.
Thanks!

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.

lgtm pse squash

@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

@echeran echeran merged commit 3c44e03 into unicode-org:maint/maint-74 Oct 16, 2023
@echeran echeran deleted the ICU-22505 branch October 16, 2023 23:25
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.

5 participants