test(csharp): stop hardcoding protocol in CloudFetch/CloseOperation E2E tests#508
Open
eric-wang-1990 wants to merge 4 commits into
Open
test(csharp): stop hardcoding protocol in CloudFetch/CloseOperation E2E tests#508eric-wang-1990 wants to merge 4 commits into
eric-wang-1990 wants to merge 4 commits into
Conversation
…2E tests
These tests pinned the connection protocol in the test body, overriding the
protocol selected by the connection config / CI matrix. Because Connect-level
options take precedence over the Open-level config protocol
(DatabricksDatabase.cs), a "rest" matrix job still opened Thrift sessions —
which fail with ENDPOINT_NOT_FOUND against a REST-only warehouse (e.g. Reyden).
- CloseOperationE2ETest: drop the hardcoded [Protocol] = "thrift" in both test
bodies. The CloseOperation code path is protocol-agnostic (per the class
docstring), so it now runs under whatever protocol the matrix selects.
- CloudFetchE2ETest: remove the in-test protocols = { "thrift", "rest" }
dimension (the CI matrix already runs rest and thrift as separate jobs, so it
was duplicating coverage and forcing Thrift even in the rest job). Each case
now runs once under the configured protocol; the REST-only result-disposition
setup keys off TestConfiguration.Protocol. 20 cases -> 10.
Genuine cross-protocol parity tests (SeaMetadataE2ETests, IntervalValueTests)
and REST/SEA-specific tests are intentionally left unchanged.
Co-authored-by: Isaac
Reverts the CloseOperationE2ETest change from the previous commit. The E2E run showed all 4 of these tests fail under REST: the assertion targets the composite_reader.close_operation trace event emitted by DatabricksCompositeReader.Dispose, which only exists on the Thrift path. The REST path (StatementExecutionConnection) has no composite reader, so the event is never emitted. This test is Thrift-specific by design, so the [Protocol] = "thrift" pin is correct; added a comment explaining why. Skipping it on REST-only warehouses is tracked separately. The CloudFetchE2ETest de-hardcoding is kept — validated passing under both the rest and thrift matrix jobs. Co-authored-by: Isaac
eric-wang-1990
added
e2e-test
eric-wang-1990
changed the title
…ment.dispose on SEA) DisposeEmitsCloseOperationEvent no longer hardcodes Thrift — it inherits the configured protocol and branches the assertion to match how each protocol releases the server-side operation: - Thrift: composite_reader.close_operation emitted by DatabricksCompositeReader.Dispose. - REST/SEA: statement.dispose emitted by StatementExecutionStatement.Dispose, which brackets the CloseStatementAsync (HTTP DELETE). To make the SEA event observable, StatementExecutionStatement.Dispose now starts its own span via this.TraceActivity (named StatementExecutionStatement.Dispose) instead of annotating Activity.Current. Dispose typically runs outside any ambient activity (e.g. connection pooling), so the prior Activity.Current?.AddEvent was usually dropped — in tests and in production. This mirrors DatabricksCompositeReader.Dispose on the Thrift path, which owns its own span. DisposeCloseOperation_..._Issue489 stays Thrift-pinned: it measures the Thrift TCloseOperationReq RPC latency via a started/completed event pair, which has no SEA equivalent (the REST close is a single DELETE). Co-authored-by: Isaac
eric-wang-1990
added
e2e-test
eric-wang-1990
changed the title
msrathore-db
approved these changes
Jun 4, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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
2 participants
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.
What & why
Several E2E tests hardcoded the connection protocol in the test body. Because Connect-level options take precedence over the Open-level config protocol (
DatabricksDatabase.cs:91-95), aprotocol: restmatrix job still opened Thrift sessions for those tests — which fail withENDPOINT_NOT_FOUND … (HTTP 404)against a REST-only warehouse (e.g. the Reyden nightly). This PR removes the incidental hardcoding so tests follow the matrix/config protocol, and makes the genuinely protocol-specific test branch on protocol instead of pinning.Changes
CloudFetchE2ETest— de-hardcodedRemoved the in-test
protocols = { "thrift", "rest" }dimension. The CI matrix already runsrestandthriftas separate jobs, so the dimension both duplicated coverage and forced Thrift inside the rest job. Each case now runs once under the configured protocol; the REST-only result-disposition setup keys offTestConfiguration.Protocol. 20 cases → 10.CloseOperationE2ETest— protocol-awareDisposeEmitsCloseOperationEventno longer pins Thrift. It inherits the configured protocol and branches the assertion to match how each protocol releases the server-side operation:composite_reader.close_operationemitted byDatabricksCompositeReader.Dispose.statement.disposeemitted byStatementExecutionStatement.Dispose, which brackets theCloseStatementAsync(HTTP DELETE) that releases the server statement. (There is no composite reader on the REST path.)DisposeCloseOperation_..._Issue489stays Thrift-pinned by design — it measures the ThriftTCloseOperationReqRPC latency via a started/completed event pair, which has no SEA equivalent (the REST close is a single DELETE).StatementExecutionStatement.Dispose()(driver) — own spanTo make the SEA branch's signal observable,
Dispose()now starts its own span viathis.TraceActivity(...)instead of annotatingActivity.Current.Disposetypically runs outside any ambient activity (e.g. connection pooling), so the priorActivity.Current?.AddEventwas usually dropped — in tests and in production. This mirrorsDatabricksCompositeReader.Disposeon the Thrift path and fixes a latent telemetry gap forstatement.dispose/statement.dispose.error.Validation
E2E run on this branch (commit
f62dd55) against the dual-protocol warehouse — both jobs green:CloseOperationE2ETest: failed under REST when Thrift-pinned in an earlier revision; now passes under REST via thestatement.disposebranch.CloudFetchE2ETest: all 10 cases pass under REST.Out of scope (intentionally unchanged)
SeaMetadataE2ETests,IntervalValueTests."rest"by design and pass today:ServerSidePropertyE2ETest,StatementExecutionDriverE2ETests.This pull request and its description were written by Isaac.