Skip to content

KTOR-9337 Skip CurlHttp2Test protocol version check on Windows#5465

Merged
e5l merged 1 commit intorelease/3.xfrom
claude/5458-curl-http2-mingw
Mar 19, 2026
Merged

KTOR-9337 Skip CurlHttp2Test protocol version check on Windows#5465
e5l merged 1 commit intorelease/3.xfrom
claude/5458-curl-http2-mingw

Conversation

@e5l
Copy link
Copy Markdown
Member

@e5l e5l commented Mar 19, 2026

Summary

  • Fixes Flaky: CurlHttp2Test.test protocol version is HTTP 2 on mingwX64 #5458
  • The Windows CI curl build does not negotiate HTTP/2 via ALPN, so CurlHttp2Test.test protocol version is HTTP 2 always fails on mingwX64 (returns HTTP/1.1 instead of HTTP/2.0)
  • Skips the test on Windows using Platform.osFamily == OsFamily.WINDOWS check, matching the existing pattern used in LibcurlTest

Closes #5458

Test plan

  • The test continues to run on Linux and macOS where HTTP/2 negotiation works
  • On Windows (mingwX64), the test is skipped instead of failing
  • All existing tests in the module continue to pass

🤖 Generated with Claude Code

The Windows CI curl build does not negotiate HTTP/2 via ALPN,
causing the protocol version assertion to always fail on mingwX64.
Skip the test on Windows using Platform.osFamily check, matching
the existing pattern used in LibcurlTest.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 19, 2026

📝 Walkthrough

Walkthrough

Added imports and an overridden test method to CurlHttp2Test that skips the HTTP 2 protocol version test on Windows platforms while delegating to the parent implementation on other operating systems.

Changes

Cohort / File(s) Summary
Test Override
ktor-client/ktor-client-curl/desktop/test/io/ktor/client/engine/curl/test/CurlHttp2Test.kt
Added ExperimentalNativeApi and Test imports. Overrode test protocol version is HTTP 2 test method with @Test and @OptIn(ExperimentalNativeApi::class) annotations to conditionally return early on Windows; otherwise delegates to superclass implementation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the required template sections (motivation via issue reference, solution description) and includes test plan details and implementation approach.
Linked Issues check ✅ Passed The code change directly addresses issue #5458 by implementing the short-term mitigation: skipping the test on Windows using platform-specific logic matching existing patterns.
Out of Scope Changes check ✅ Passed The change is narrowly scoped to the CurlHttp2Test override for Windows compatibility, with no unrelated modifications to other files or functionality.
Title check ✅ Passed The title accurately describes the main change: skipping a specific HTTP/2 protocol version test on Windows, which matches the core objective of the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/5458-curl-http2-mingw
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

sslVerify = false
}

// https://github.com/ktorio/ktor/issues/5458
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.

We have an YouTrack issue for this KTOR-9337

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — updated the PR title to reference KTOR-9337.

Copy link
Copy Markdown
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Please mention KTOR-9337 in the PR title

@e5l e5l enabled auto-merge (squash) March 19, 2026 14:57
@e5l e5l changed the title #5458 Skip CurlHttp2Test protocol version check on Windows KTOR-9337 Skip CurlHttp2Test protocol version check on Windows Mar 19, 2026
@e5l
Copy link
Copy Markdown
Member Author

e5l commented Mar 19, 2026

Addressed review feedback:

  • Updated PR title to reference KTOR-9337 as requested by @osipxd

CI failures investigation — all 3 failing checks contain pre-existing flaky tests unrelated to this PR's change (CurlHttp2Test.kt). The same tests also fail on the release/3.x base branch:

  1. DarwinLegacyEngineTest.testRequestInRunBlocking[macosArm64] and DarwinEngineTest.testRequestInRunBlockingDispatchersDefault[macosArm64]withTimeout(1000) too tight for CI agents under load. Pass locally. → Filed Flaky test: DarwinEngineTest.testRequestInRunBlockingDispatchersDefault times out on CI #5475
  2. TestApplicationTest.testStreamingResponse[mingwX64] — Race condition in channel-based interleaving assertion. Also fails on JVM on base branch. → Filed Flaky test: TestApplicationTest.testStreamingResponse race condition on mingwX64 #5476
  3. ContentTest.testEmptyContent[mingwX64] — 20s timeout on Windows native, no error message. Also seen on macosArm64 on base branch. → Filed Flaky test: ContentTest.testEmptyContent times out on mingwX64 #5477

@e5l e5l merged commit 91c4fc6 into release/3.x Mar 19, 2026
20 of 23 checks passed
@e5l e5l deleted the claude/5458-curl-http2-mingw branch March 19, 2026 16:24
@osipxd osipxd linked an issue May 6, 2026 that may be closed by this pull request
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.

Flaky: CurlHttp2Test.test protocol version is HTTP 2 on mingwX64

2 participants