Skip to content

Exclude new time zones from TestTimeZoneUtils#20638

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
Akanksha-kedia:23aug
Aug 30, 2023
Merged

Exclude new time zones from TestTimeZoneUtils#20638
tdcmeehan merged 1 commit intoprestodb:masterfrom
Akanksha-kedia:23aug

Conversation

@Akanksha-kedia
Copy link
Copy Markdown
Contributor

Before fix:
Error : [ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.448 s <<< FAILURE! - in com.facebook.presto.util.TestTimeZoneUtils
[ERROR] com.facebook.presto.util.TestTimeZoneUtils.testNamedZones Time elapsed: 1.293 s <<< FAILURE!
java.time.zone.ZoneRulesException: Unknown time-zone ID: Europe/Kyiv
at java.time.zone.ZoneRulesProvider.getProvider(ZoneRulesProvider.java:272)
at java.time.zone.ZoneRulesProvider.getRules(ZoneRulesProvider.java:227)
at java.time.ZoneRegion.ofId(ZoneRegion.java:120)
at java.time.ZoneId.of(ZoneId.java:411)
at java.time.ZoneId.of(ZoneId.java:359)
at com.facebook.presto.util.TestTimeZoneUtils.assertTimeZone(TestTimeZoneUtils.java:98)
at com.facebook.presto.util.TestTimeZoneUtils.testNamedZones(TestTimeZoneUtils.java:81)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)

[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestTimeZoneUtils.testNamedZones:81->assertTimeZone:98 » ZoneRules Unknown time-zone ID: Europe/Kyiv
[INFO]
[ERROR] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0
[INFO]

After fix:
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.826 s - in com.facebook.presto.util.TestTimeZoneUtils
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:13 min
[INFO] Finished at: 2023-08-23T16:41:58+05:30
[INFO] ------------------------------------------------------------------------

@Akanksha-kedia Akanksha-kedia requested a review from a team as a code owner August 23, 2023 11:22
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When the Joda version was upgraded in this PR: #19758, the release notes mentioned that the runtime version needed to be upgraded to have the relevant timezone data.

If we have to exclude, we should not exclude all Europe timezones from tests but only the ones which were added as part of the upgrade.

Also, did you also try running the test using the mentioned Java runtime version (11.0.10)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

Sorry its 17 no error i have corrected accordingly, with
image
same issue is coming with java 11

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

openjdk version "17.0.7" 2023-04-18
OpenJDK Runtime Environment Homebrew (build 17.0.7+0)
OpenJDK 64-Bit Server VM Homebrew (build 17.0.7+0, mixed mode, sharing)
this version of java , no issue is coming

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@imjalpreet please help to review this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdcmeehan please help to review the PR.

Copy link
Copy Markdown
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Changes look good. Please fix commit message in accordance to our guidelines. In particular, describe the fix, use proper captilization, and use the imperative mood. https://cbea.ms/git-commit/

@Akanksha-kedia Akanksha-kedia changed the title mvn test failed about testing prestodb Exclude Europe/Zaporozhye, Europe/Uzhgorod, Europe/Kiev , Europe/Kyiv time zone from TestTimeZoneUtils Aug 29, 2023
@Akanksha-kedia Akanksha-kedia changed the title Exclude Europe/Zaporozhye, Europe/Uzhgorod, Europe/Kiev , Europe/Kyiv time zone from TestTimeZoneUtils Exclude Europe/Zaporozhye, Europe/Uzhgorod, Europe/Kiev, Europe/Kyiv time zone from TestTimeZoneUtils Aug 29, 2023
@Akanksha-kedia
Copy link
Copy Markdown
Contributor Author

Changes look good. Please fix commit message in accordance to our guidelines. In particular, describe the fix, use proper captilization, and use the imperative mood. https://cbea.ms/git-commit/

i have done required changes.

@tdcmeehan
Copy link
Copy Markdown
Contributor

This is better. Please try the following since the commit message title is over 50 characters

Exclude new time zones from TestTimeZoneUtils

Exclude Europe/Zaporozhye, Europe/Uzhgorod, Europe/Kiev, Europe/Kyiv time zone from TestTimeZoneUtils

@Akanksha-kedia Akanksha-kedia changed the title Exclude Europe/Zaporozhye, Europe/Uzhgorod, Europe/Kiev, Europe/Kyiv time zone from TestTimeZoneUtils Exclude new time zones from TestTimeZoneUtils Aug 30, 2023
@Akanksha-kedia
Copy link
Copy Markdown
Contributor Author

@tdcmeehan done please review and merge accordingly

@tdcmeehan tdcmeehan merged commit 10a4cb9 into prestodb:master Aug 30, 2023
@wanglinsong wanglinsong mentioned this pull request Oct 3, 2023
54 tasks
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.

3 participants