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

Ensure decaton works with older jdk #225

Merged
merged 10 commits into from
Mar 7, 2024
Merged

Conversation

ocadaruma
Copy link
Member

@ocadaruma ocadaruma commented Mar 6, 2024

  • Now Decaton compiles only on jdk >= 21 (by Add virtual threads support #224) but since there are many users who still use jdk 8,11,17, we still should run tests on these versions

@@ -39,8 +39,7 @@

@ExtendWith(MockitoExtension.class)
public class DecatonClientTest {
@Spy
private final DecatonClient<HelloTask> decaton = new DecatonClient<HelloTask>() {
private static class NoopClient implements DecatonClient<HelloTask> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Spying anonymous class fails on JDK8 with reflection error
https://github.com/line/decaton/actions/runs/8174863099/job/22350646261
image

I didn't want to dig into the detail as we likely drop JDK8-support in not-so-far-future so I just rewrote it with static class for now.

@ocadaruma ocadaruma marked this pull request as ready for review March 7, 2024 00:09
@ocadaruma ocadaruma requested a review from kawamuray March 7, 2024 01:32
Copy link
Member

@kawamuray kawamuray left a comment

Choose a reason for hiding this comment

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

just one minor question

distribution: temurin
java-version: |
${{ matrix.java }}
21
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentionally left or just forgot to remove line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally. We need to install 21 always to build the project, regardless test-jdk version.
Last one will be used as the global java version (refs: https://github.com/actions/setup-java?tab=readme-ov-file#install-multiple-jdks)

Copy link
Member

Choose a reason for hiding this comment

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

ah-ok. now understand how it works

@@ -33,6 +35,7 @@
import com.linecorp.decaton.testing.RandomExtension;
import com.linecorp.decaton.testing.processor.ProcessorTestSuite;

@EnabledForJreRange(min = JRE.JAVA_21)
Copy link
Member

Choose a reason for hiding this comment

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

Oh... there's such a convenient annotation existed... !

@ocadaruma ocadaruma requested a review from kawamuray March 7, 2024 07:52
Copy link
Member

@kawamuray kawamuray left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick follow-up!

@kawamuray kawamuray merged commit 17f06c6 into line:master Mar 7, 2024
5 checks passed
@ocadaruma ocadaruma deleted the jdk-tests branch March 7, 2024 08:42
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.

2 participants