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

Disable two tests in BookKeeperTest on JDK 17+ due to UnsupportedOperationException #4347

Merged
merged 1 commit into from
May 8, 2024

Conversation

shoothzj
Copy link
Member

@shoothzj shoothzj commented May 7, 2024

Motivation

JDK 17 and later throw UnsupportedOperationException for the suspend and resume methods, causing the testConstructionZkDelay and testConstructionNotConnectedExplicitZk tests to fail.

See jdk21 daily build error: https://github.com/apache/bookkeeper/actions/runs/8962118072

Changes

  • Disabled the two affected test methods for JDK 17 and above using the @EnabledForJreRange annotation.
  • Transitioned to JUnit 5 annotations and assertions.

Q&A

Q: why there are so many line changes? like format change
A: In junit5, the msg should be put in the latest param, related to some changes.

@shoothzj shoothzj force-pushed the bkkeeper-test-method-jdk17 branch 2 times, most recently from bcc5004 to 991d64a Compare May 7, 2024 01:21
@shoothzj
Copy link
Member Author

shoothzj commented May 7, 2024

@shoothzj shoothzj requested a review from dlg99 May 7, 2024 04:48
Copy link
Contributor

@dlg99 dlg99 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 for picking this up!

Overall this LGTM but the change calls for addition of the "Compatibility Check" CI job with JDK 21 (LTS), otherwise I can't really look at the PR and tell that the fix works. Also this way we won't break this in the future.
See

matrix:
include:
- step_name: Compatibility Check Java8
jdk_version: 8
- step_name: Compatibility Check Java11
jdk_version: 11
- step_name: Compatibility Check Java17
jdk_version: 17

and

bookkeeper/.asf.yaml

Lines 54 to 56 in 1ae4be0

- Compatibility Check Java11
- Compatibility Check Java17
- Compatibility Check Java8

@shoothzj
Copy link
Member Author

shoothzj commented May 7, 2024

@dlg99 IIUC, if you mean add jdk21 CI, I think we can open a new PR. And shall we postpone it after jdk21 daily build sucesses?
We have added jdk 21 daily build in #4292

…ationException

Signed-off-by: ZhangJian He <shoothzj@gmail.com>
@shoothzj shoothzj force-pushed the bkkeeper-test-method-jdk17 branch from 991d64a to 1b1e530 Compare May 8, 2024 05:26
@shoothzj shoothzj merged commit f66e962 into apache:master May 8, 2024
21 checks passed
@shoothzj shoothzj deleted the bkkeeper-test-method-jdk17 branch May 8, 2024 06:28
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 25, 2024
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…ationException (apache#4347)

### Motivation

JDK 17 and later throw `UnsupportedOperationException` for the `suspend` and `resume` methods, causing the `testConstructionZkDelay` and `testConstructionNotConnectedExplicitZk` tests to fail.

See jdk21 daily build error: https://github.com/apache/bookkeeper/actions/runs/8962118072

### Changes

- Disabled the two affected test methods for JDK 17 and above using the `@EnabledForJreRange` annotation.
- Transitioned to JUnit 5 annotations and assertions.

### Q&A

Q: why there are so many line changes? like format change
A: In junit5, the msg should be put in the latest param, related to some changes.

Signed-off-by: ZhangJian He <shoothzj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants