Test fixes for MST metadata expectations and User-Agent env leak#788
Open
vikrantpuppala wants to merge 2 commits into
Open
Test fixes for MST metadata expectations and User-Agent env leak#788vikrantpuppala wants to merge 2 commits into
vikrantpuppala wants to merge 2 commits into
Conversation
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
Clear all KNOWN_AGENTS env vars in test_useragent_header before connecting, mirroring the pattern in test_agent_detection.py. Without this, running the test inside an AI-agent shell (e.g. Claude Code, which sets CLAUDECODE=1) causes session.py to append " agent/<product>" to the User-Agent and the strict `in` assertion fails. CI runners don't set agent env vars so this passes there, but the test is environment- sensitive locally. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
vikrantpuppala
force-pushed
the
fix/useragent-test-env-leak
branch
from
b4ad201 to
4089f3f
Compare
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
… MST
The TestMstMetadata tests were written speculatively expecting the
Thrift Get{Catalogs,Schemas,Tables,Columns} RPCs to succeed inside an
active multi-statement transaction. The server's MST guard now rejects
these RPCs with TRANSACTION_NOT_SUPPORTED ("... is not supported within
a multi-statement transaction."), matching the intentional design where
MST exposes a small allowlist of SQL forms.
Flip the four `*_in_mst` tests into `*_blocked` tests asserting the
expected DatabaseError. The two `*_non_transactional_after_concurrent_*`
freshness tests are deleted outright — their entire premise ("Thrift
RPCs bypass MST and see concurrent DDL") is no longer valid, so there
is nothing meaningful left to assert.
Note: the rejection happens at the MST guard before reaching the txn,
so the active transaction remains usable after a blocked Get* (unlike
SQL forms in TestMstBlockedSql, where the failure aborts the txn).
The shared helper documents this distinction.
Verified against dogfood: 4 passed in 33.67s.
Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
vikrantpuppala
changed the title
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
1 participant
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
Two independent test-only fixes bundled together because both were uncovered while investigating CI red on
test-with-coverage.1. Flip
TestMstMetadatato assert blocked behaviorThe four
tests/e2e/test_transactions.py::TestMstMetadata::test_cursor_*_in_msttests, added in #775, were written speculatively expecting ThriftGet{Catalogs,Schemas,Tables,Columns}RPCs to succeed inside an active multi-statement transaction. The server's MST guard is intentionally designed to reject these ("... is not supported within a multi-statement transaction."), matching the small allowlist pattern documented inTestMstBlockedSql.*_in_msttests to*_blockedand rewrote them to assert the expectedDatabaseErrorvia a shared_assert_metadata_rpc_blockedhelper.*_non_transactional_after_concurrent_*freshness tests outright — their premise ("Thrift RPCs bypass MST and see concurrent DDL") is no longer valid, so there's nothing meaningful left to assert.TestMstMetadataandTestMstBlockedSqldocstrings to reflect actual server behavior.One subtle difference vs
TestMstBlockedSql's SQL-form blocks: the Get* rejection happens at the MST guard before reaching the transaction, so the active txn remains usable after a blocked Get* (whereas SQL-form blocks abort the txn). The shared helper documents this.2. Fix
test_useragent_headerenvironment-sensitivitytests/unit/test_session.py::test_useragent_headerstrictly asserts("User-Agent", "<name>/<version>") in http_headers. When run inside any AI-agent shell thatdatabricks.sql.common.agent.KNOWN_AGENTSknows about (e.g. Claude Code setsCLAUDECODE=1),session.py:68-70appendsagent/<product>to the header and theincheck fails. CI runners don't set agent env vars, so this passed there — it's an environment-sensitivity bug rather than a code regression.The fix mirrors the pattern in
tests/unit/test_agent_detection.py:42-49:monkeypatch.delenvevery known agent env var before the assertion. IteratingKNOWN_AGENTSkeeps the cleared list automatically in sync if new agents are added tosrc/databricks/sql/common/agent.py.Test plan
pytest tests/e2e/test_transactions.py::TestMstMetadataagainst dogfood: 4 passed in 33.67spytest tests/unit/test_session.py::TestSession::test_useragent_headerpasses inside a Claude Code shell (was failing before the fix)test-with-coverage(e2e + coverage) — verifying via this PRThis pull request and its description were written by Isaac.