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

KTOR-7812 Use default JDK for running tests on CI #4342

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

osipxd
Copy link
Member

@osipxd osipxd commented Sep 25, 2024

Subsystem
Tests infrastructure

Motivation
KTOR-7812 Use default JDK for running tests on CI

Solution
Use the JDK from JAVA_HOME to run tests on CI.

It is better to review commit by commit.

@osipxd osipxd self-assigned this Sep 25, 2024
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 689782f to 462fd68 Compare September 25, 2024 07:59
@bjhham
Copy link
Contributor

bjhham commented Sep 26, 2024

There seems to be some problems in the TLS package - we got an out-dated ECDH curve java.security.ProviderException: Curve not supported: secp128r1 (1.3.132.0.28) and some rogue reflection java.lang.reflect.InaccessibleObjectException: Unable to make private static java.time.Instant java.time.Instant.create(long,int) accessible: module java.base does not "opens java.time" to unnamed module @5b8bc155

You also seem to have unlocked a consistent failure in testHeaderIsTooLong(), so that's good 😄

@osipxd
Copy link
Member Author

osipxd commented Sep 26, 2024

Yes, finally these builds against different Java versions found something :)

@osipxd osipxd marked this pull request as draft September 26, 2024 15:23
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 462fd68 to 5749278 Compare September 30, 2024 10:34
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 5749278 to 8210bed Compare October 7, 2024 13:42
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch 6 times, most recently from 719000a to a623b53 Compare November 19, 2024 08:23
@@ -186,7 +186,7 @@ class CookiesIntegrationTests : ClientLoader() {
}

@Test
fun testCookiesWithWrongValue() = clientTests(listOf("js", "Darwin", "DarwinLegacy", "WinHttp")) {
fun testCookiesWithWrongValue() = clientTests(listOf("Js", "Darwin", "DarwinLegacy", "WinHttp", "Java")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

openjdk/jdk16@1c47244: Java 16 adds validation for header values, so this test doesn't pass on Java engine anymore

@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 433072c to 6f9571d Compare November 19, 2024 15:01
@osipxd osipxd marked this pull request as ready for review November 19, 2024 15:02
@osipxd osipxd requested a review from bjhham November 19, 2024 15:03
@osipxd
Copy link
Member Author

osipxd commented Nov 19, 2024

@bjhham, finally I've fixed this PR. After some tries I didn't manage to fix testHeaderIsTooLong, so I disabled it and created a task to fix it later:

  • KTOR-7811 Flaky test: SustainabilityTestSuite.testHeaderIsTooLong

@osipxd osipxd changed the title KTOR-7482 Use default JDK for running tests on CI KTOR-7812 Use default JDK for running tests on CI Nov 19, 2024
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 6f9571d to 2e4099b Compare November 21, 2024 16:37
@osipxd osipxd requested a review from e5l November 21, 2024 16:37
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