fix(ingestion): bind the test-connection diagnoses to errors drivers really raise#30155
Open
IceS2 wants to merge 4 commits into
Open
fix(ingestion): bind the test-connection diagnoses to errors drivers really raise#30155IceS2 wants to merge 4 commits into
IceS2 wants to merge 4 commits into
Conversation
github-actions
Bot
added
Ingestion
safe to test
…ly raise (#30153) Rebased onto main after #30150 squash-merged; the dedup this branch was stacked on now lives in main, so this carries only the provenance fixes. Every matcher kept or added is traceable to a real driver observation - the exception class imported, or the driver source / official docs cited. Where provenance could not be established the matcher was deleted rather than guessed. - airflow: the gate passed against any web server (JSONDecodeError swallowed to {}) and, for MWAA, against a hardcoded stub; both now use a strict accessor that really fails. A raw get_raw error (a mistyped host) no longer strands the caller on a swallowed None, and a 429 surfaces the driver's real HTTPError instead of a blank LimitsException. - mssql: the six error numbers were dead under every supported driver (pytds keeps only the last message's number); dropped 4060/300/911/262, kept 18456/229/297, verified live against Azure SQL Edge. - powerbi: the status rules were dead on the real envelope (None -> TypeError); fixed, retry budget bounded, AADSTS causes split. - databricks: GetTables/GetViews no longer pass with nothing resolved; the dead, over-broad "forbidden" rule removed. - redshift/snowflake/looker/dbtcloud: rules that cannot fire deleted, with the fixtures that "proved" them. Fixtures build errors through the driver's own factory and drive TestConnectionRunner end to end, so they cannot drift from the driver.
IceS2
force-pushed
the
fix/testconn-p0-provenanced-tests
branch
from
July 17, 2026 14:32
2364af3 to
4bf5838
Compare
…g fact The provenance comments explained why each dead rule is absent - valuable, but written as multi-paragraph essays. Each is now the fact plus a source reference: the pytds message-folding quirk, why NETWORK_ERRORS is not folded (looker, dbtcloud), the snowflake login-403, the strict-accessor rationale. No rule, matcher or test changed.
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return cast( | ||
| "requests.Response", | ||
| self._request("GET", path, data, headers=headers, retry_wait=retry_wait, raw=True), | ||
| self._request("GET", path, data, headers=headers, retry_wait=retry_wait, retries=retries, raw=True), |
Contributor
There was a problem hiding this comment.
When all bounded attempts receive a retryable status, _request exits the retry loop with None, which this method casts to Response. Repeated Airflow 504s then fail at response.raise_for_status(), and repeated Power BI 429s fail at response.ok, producing an unclassified AttributeError instead of the intended HTTP diagnosis.
Context Used: CLAUDE.md (source)
…haviour The source-level tests were updated with the fixes; the parallel topology tests still asserted the old behaviour and failed CI. - databricks get_tables/get_views: no longer return [] on an unresolved schema (the false-pass a mandatory step used to hide) - now raise, as the source test already checks. - airflow: the CheckAccess gate reads via test_get_version, not the lenient get_version, so its decoration tests mock the strict accessor.
Code Review ✅ ApprovedRefactors test-connection diagnostic matchers to align with actual driver error behavior, improving reliability by replacing speculative rules with provenance-backed logic. Verified against real SQLAlchemy engine wrappers and driver-specific error patterns. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Contributor
🟡 Playwright Results — all passed (34 flaky)✅ 4537 passed · ❌ 0 failed · 🟡 34 flaky · ⏭️ 95 skipped
🟡 34 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
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.
Closes #30153
Stacked on #30150 — targets
connector-improvements-reviewso the diff is only this branch's work. GitHub will retarget it tomainwhen #30150 merges.Every matcher kept or added here is traceable to a real driver observation: the exception class imported, or the driver source / official documentation cited in a comment. Where provenance could not be established, the matcher was deleted rather than guessed at.
The connection reported the wrong outcome
ometa/client.pyreturns the rawResponseon aJSONDecodeError;_parse_responsethen catchesExceptionand returns{}, socheck_accessnever raised. Added a stricttest_get_version()(get_raw+raise_for_status, mirroring dbt Cloud's_test_get) and pointed the gate at it;get_versionkeeps its lenient behaviour for ingestion.Matchers.errnoneeds anintatargs[0]; pytds (the default) puts the message there, pymssql a(number, message)tuple, pyodbc a SQLSTATE string. Added_mssql_number, reading.number/.msg_noand the pymssql tuple across the chain and.orig. The driver comment named pymssql and pyodbc while the default is pytds — corrected.get_tables/get_viewsreturned[]when no catalog or schema resolved, so a mandatory step passed. They now raise, as Unity Catalog already did.codeundererror, so the shared client returnedNoneandDashboardsResponse(**None)raisedTypeError. AddedPowerBiApiError+_test_get, and split the vague auth diagnosis into the three AADSTS causes — gated onInvalidSourceException, so a status-coded REST error echoing a code still classifies by status.Rules deleted, with the evidence
28000/28P01— the module's own docstring already said connect-phase errors carry no SQLSTATE. Confirmed against PostgreSQL 15: wrong password →pgcode=None, missing database →pgcode=None, query-phase denial →pgcode='42501'. That validated the deletion and keeping 42501.contains("forbidden")—Error.__str__returns only the message; the status lives incontext["http-code"], and "forbidden" appears nowhere in databricks-sql. Replaced with a regression test pinning that a table namedforbidden_itemsis not misread as "Access denied".290404— a bad account returns 403, andForbiddenError.__init__addsER_HTTP_GENERAL_ERROR + 250001, so the code is 540001. Keyed on the message instead: 540001 is an artifact of that double-add, the message is the driver's deliberate signal.NETWORK_ERRORSfold — driving the real transport shows aConnectionRefusedErrorcomes back asResponse(ok=False, value=b'[Errno 61] Connection refused'). The type is destroyed, so the rules cannot fire; removal changes no diagnosis, because_transport_textalready yields the same titles.NETWORK_ERRORSfold — the rules were dead by precedence, not absence: a real DNS failure does carrygaierror, but the connector's own requests-typed rules always match first. Deleted rather than adding a TCP preflight: dbt Cloud is public SaaS,requestshonoursHTTPS_PROXYand a raw probe does not, so a preflight would fail a working proxied setup — the caseping's own docstring warns about.Judgement calls worth a look
GetQueriesprovokes; pytds keeps only the last message's number (297), and 297's own text lacks "permission was denied", so the number is the only signal that catches it.context["http-code"]) exists, but whether a real 403 populates it could not be established without a live workspace, so no matcher.Tests
The fixtures are the point. mssql builds its errors through pytds's own
_create_exception_by_messageand the realpyodbcclass, and proves the codes with German messages that no Englishcontainsrule can match. Several connectors now driveTestConnectionRunner.run()to aTestConnectionResultagainst a realmssql+pytdsengine, so SQLAlchemy does the.origwrapping itself.Mutation-checked: neutering
_mssql_numberfails 9 tests — against the previous state, where neuteringMatchers.errnoentirely left all 20 mssql tests green.Not validated live against real Airflow, Power BI, Databricks or Snowflake; provenance is driver source, official docs, or a local PostgreSQL container. Only the connectors named here were audited.
Greptile Summary
This PR aligns ingestion test-connection diagnoses with errors produced by the supported drivers and APIs. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Nonebefore Airflow or Power BI handles them.Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/testconn-p0..." | Re-trigger Greptile
Context used (3)