test(csharp): coverage push: 82.5% → 85.9% (TLS cert + OpenSession fallback + Statement errors)#59
Merged
Conversation
…ack + errors) Phase 6 of the mock-fixture coverage push. Targets the four categories identified after PR adbc-drivers#55: TLS cert validation, OpenSession fallback, Statement error-wrappers, and the Standard transport's TLS-on branch. Two small fixture extensions enable the new tests: - HiveServer2TestServer.StatusCodeOverride — optional per-request hook that lets a test force any HTTP status (e.g. 401 or 500) for the Nth request. The driver-side THttpTransport surfaces non-2xx responses as TTransportException wrapping HttpRequestException, which is exactly what TryOpenSessionWithFallbackAsync's catch arms look for. - MockResultBuilder.WithPositionBase(int) — defaults to 1 (matches Hive/Spark's ColumnMapIndexOffset=1) and can be set to 0 for Impala fixtures (ColumnMapIndexOffset=0). The previous PR deferred an Impala GetObjects=All test for exactly this off-by-one reason; it's now enabled. New tests: 1. **TestCertificates** (Common/) — in-memory self-signed cert generation via CertificateRequest, with valid / expired variants and a temp-PFX writer for the trusted-cert-path branch. Conditional on NET5+ (net472 has a different X509 API). 2. **HiveServer2TlsImplCertChainTests** (Common/, NET5+) — drives every branch of ValidateCertificate (None / DisableValidation / NullCert / NameMismatch with and without AllowHostnameMismatch / AllowSelfSigned with and without chain errors / TrustedCertificatePath with and without AllowSelfSigned, expired-cert chain-invalid). Covers the UntrustedRoot arm of ThrowDetailedCertificateError naturally (it's the flag a self-signed cert always carries). The remaining ChainStatus arms (Revoked, PartialChain, NotTimeValid, etc.) need a trusted-chain construction that isn't portable from a unit test — they stay uncovered. 3. **MockServerOpenSessionFallbackTests** (Hive2/MockServer/) — drives TryOpenSessionWithFallbackAsync's three arms: 401 → immediate Unauthorized throw; 500 on every call → exhausts FallbackProtocolVersions and rethrows lastException; 500 on first call then 200 → fallback recovers and the connection opens. 4. **MockServerStatementErrorTests** (Hive2/MockServer/) — each metadata RPC (and ExecuteStatement) made to throw an InvalidOperationException; asserts the driver wraps it as HiveServer2Exception via the `catch (Exception) when (ex is not HiveServer2Exception)` arms in Statement.ExecuteQueryAsync / ExecuteUpdateAsync / Connection.GetTableSchema. 5. **MockServerStandardTlsTests** (Hive2/MockServer/) — connects with IsTlsEnabled=true to the plain-TCP mock; the handshake fails but the test only needs the TTlsSocketTransport(hostname,...) and TTlsSocketTransport(ipAddress,...) constructors plus the ValidateCertificate callback wiring to fire. Covers Hive/Spark/Impala StandardConnection in parallel. Coverage on `AdbcDrivers.HiveServer2` (excluding generated Thrift): **82.9% → 85.9% lines** (+3.0 points; 4292 → 4446 covered out of 5177). Per-file highlights: - HiveServer2TlsImpl: 38% → 63% (cert tests, +110 covered lines) - Hive2/HiveServer2StandardConnection: 63% → 75% (TLS-on) - Spark/SparkStandardConnection: 65% → 75% - Impala/ImpalaStandardConnection: 66% → 72% - HiveServer2Connection: 85% → 86% (fallback path) The remaining 4.1 points to 90% live in scattered 3-5 line catch-blocks in Connection/Statement metadata RPCs, plus the ChainStatus arms that aren't reachable without trusted-chain construction infrastructure. Each remaining test would land < 5 covered lines — diminishing returns. 1632/1641/1641 tests pass on net472 / net8.0 / net10.0 respectively (net472 skips the 9 cert-chain tests that require CertificateRequest). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 6 of a mock-fixture coverage push for the C# AdbcDrivers.HiveServer2 driver. Adds two small testing-fixture hooks (HTTP status-code override and configurable column position base) and five new test files that drive previously uncovered TLS validation, OpenSession HTTP fallback, Statement/Connection error wrappers, and Standard transport TLS setup branches across Hive, Spark, and Impala flavors. Reported line coverage moves from 82.9% to 85.9%.
Changes:
- Extend
HiveServer2TestServerwith aStatusCodeOverridehook andMockResultBuilderwithWithPositionBaseto enable Impala-offset and HTTP-failure scenarios. - Add
TestCertificateshelper (net5.0+) andHiveServer2TlsImplCertChainTestsexercising everyValidateCertificatearm plus theUntrustedRootpath ofThrowDetailedCertificateError. - Add mock-server tests for OpenSession fallback (401/500/recovery), Statement metadata error wrapping, Standard-transport TLS setup (hostname & IP overloads, all three flavors), and an Impala
GetObjectsall-depth test.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
csharp/test/AdbcDrivers.HiveServer2.TestServer/HiveServer2TestServer.cs |
Adds StatusCodeOverride to force non-OK HTTP responses for fallback tests. |
csharp/test/AdbcDrivers.HiveServer2.TestServer/MockResultBuilder.cs |
Adds WithPositionBase so Impala (offset 0) fixtures emit 0-indexed positions. |
csharp/test/AdbcDrivers.Tests.HiveServer2/Common/TestCertificates.cs |
New helper to build in-memory self-signed certs (valid/expired) and a temp cert file. |
csharp/test/AdbcDrivers.Tests.HiveServer2/Common/HiveServer2TlsImplCertChainTests.cs |
Covers ValidateCertificate branches and the UntrustedRoot arm. |
csharp/test/AdbcDrivers.Tests.HiveServer2/Hive2/MockServer/MockServerOpenSessionFallbackTests.cs |
Exercises 401-unauthorized, exhausted-fallback, and recovery paths. |
csharp/test/AdbcDrivers.Tests.HiveServer2/Hive2/MockServer/MockServerStatementErrorTests.cs |
Forces each metadata/execute RPC to throw to drive Statement wrapper arms. |
csharp/test/AdbcDrivers.Tests.HiveServer2/Hive2/MockServer/MockServerStandardTlsTests.cs |
Drives Standard TLS transport construction for Hive/Spark/Impala (hostname & IP). |
csharp/test/AdbcDrivers.Tests.HiveServer2/Impala/MockServer/ImpalaMockServerConnectionApiTests.cs |
Adds an Impala GetObjects=All test using WithPositionBase(0). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three of the four legit Copilot comments addressed:
1. MockServerStatementErrorTests: every test asserted
`Assert.ThrowsAnyAsync<Exception>` despite the names and class
docstring claiming each test verifies the
`catch (Exception) when (ex is not HiveServer2Exception)` wrapping
branch in Statement / Connection. ThrowsAny would have passed even
if the driver had stopped wrapping. Tightened all 9 tests to
`Assert.ThrowsAsync<HiveServer2Exception>` (or `Throws<...>` for the
sync `Connection.GetTableSchema` test). Confirmed all 9 still pass
under the stricter assertion.
Also dropped the dead `await Task.Yield()` in the GetTableSchema
test and renamed it to `_WrapsAsHiveServer2Exception` to match the
rest. The test is sync since `GetTableSchema` is sync.
2. MockServerStandardTlsTests: `TryOpenAndConnect` was swallowing
every Connect exception via `try { ... } catch { }`, so a future
refactor that broke the TLS-on path before the TTlsSocketTransport
ctor ran would have left the test silently passing. Renamed to
`ConnectAndAssertTlsFailure` and now asserts the specific
`HiveServer2Exception` the driver's OpenAsync catch wraps the
handshake failure with — that's what proves the transport-setup
path did run.
3. TestCertificates.SelfSignedAsTempFile: doc comment said "temp
.pfx file" but the export uses `X509ContentType.Cert` (DER public
cert, no private key). Fixed the comment to say ".cer file".
(The fourth Copilot comment, about the license-header style being
different from "modified from original" files like
HiveServer2TlsImplTest.cs, is the one already dismissed — these are
fresh files, not derived from upstream, so they get the shorter
ADBC-only header. Same pattern as PR adbc-drivers#55's new files.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
davidhcoe
approved these changes
May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 6 of the mock-fixture coverage push. Targets the four categories identified after PR #55: TLS cert validation, OpenSession fallback, Statement error-wrappers, and Standard transport TLS-on branches.
Coverage on
AdbcDrivers.HiveServer2(excluding generated Thrift bindings):Fixture extensions enabling the new tests
HiveServer2TestServer.StatusCodeOverrideFunc<int, HttpStatusCode>lets a test force any HTTP status (401, 500, etc.) for the Nth request. The driver-sideTHttpTransportsurfaces non-2xx asTTransportExceptionwrappingHttpRequestException— exactly whatTryOpenSessionWithFallbackAsync's catch arms look for.MockResultBuilder.WithPositionBase(int)ColumnMapIndexOffset=1). Set to 0 for Impala (ColumnMapIndexOffset=0). Enables an Impala GetObjects=All test that PR #55 had to defer.New tests
TestCertificates#if NET5_0_OR_GREATER— net472 has a different X509 API.HiveServer2TlsImplCertChainTestsValidateCertificate+ theUntrustedRootarm ofThrowDetailedCertificateError. The remaining ChainStatus arms (Revoked, PartialChain, NotTimeValid, etc.) need a trusted-chain construction that isn't portable from a unit test — they stay uncovered.MockServerOpenSessionFallbackTestsTryOpenSessionWithFallbackAsync's three arms: 401 → Unauthorized throw; 500 on every call → exhaustsFallbackProtocolVersions; 500 then 200 → fallback recovers.MockServerStatementErrorTestsHiveServer2Exceptionvia thecatch when (ex is not HiveServer2Exception)arms.MockServerStandardTlsTestsTTlsSocketTransportconstructors (hostname and IP variants) plusValidateCertificatecallback wiring fire before the handshake fails. Covers Hive/Spark/Impala StandardConnection.Per-file highlights
HiveServer2TlsImplHiveServer2StandardConnectionSpark/SparkStandardConnectionImpala/ImpalaStandardConnectionHiveServer2ConnectionWhat's left
The remaining 4.1 points to 90% live in scattered 3-5 line catch-blocks in
HiveServer2Connection/HiveServer2Statementmetadata RPCs, plus the ChainStatus arms inThrowDetailedCertificateErrorthat aren't reachable without trusted-chain construction infrastructure. Each remaining test would land < 5 covered lines — point of diminishing returns.Test plan
🤖 Generated with Claude Code