drivaspreset opened a new pull request, #44661: URL: https://github.com/apache/superset/pull/44661
### SUMMARY Closes test gaps for `TrinoEngineSpec` (Shortcut epic sc-105818), identified by a QA gap analysis that re-verified all 13 stories against current `master` rather than trusting the tickets as written (roughly half needed a corrected verdict — see per-story notes below). **Tests only, zero production code changes.** Test count: **116 → 152** (+36, incl. parametrize expansion) across **27 new test functions** plus 3 new parametrize cases on 2 existing tests. Line coverage of `superset/db_engine_specs/trino.py`: **92% → 94%** (24 → 18 missed statements). ### PER-STORY BREAKDOWN | Story | Status | Notes | |---|---|---| | 105828 (mocked-engine integration) | No new dedicated test | `test_trino.py` already *is* the mocked-engine service layer requested — no real Trino, no CI changes. Query lifecycle (`execute_with_cursor`/`handle_cursor`), auth-per-method, and schema/table browsing are collectively covered by existing tests plus 105829/105831/105876 below. Adding a separate "integration" test would just duplicate them. | | 105829 (table/view/schema names) | 8 new tests | `TrinoEngineSpec(PrestoBaseEngineSpec)` does **not** inherit `PrestoEngineSpec`, so `get_table_names`/`get_view_names` resolve to `base.py`'s generic inspector-based implementation, not Presto's `information_schema` query — previously untested for Trino. Covers normal listing, empty result, and catalog-qualified schema-prefix stripping. | | 105830 (metadata/information_schema) | No new tests | `get_columns` is already covered by existing `test_get_columns*`. `has_table` lives on the `Database` model (`models/core.py`), not the spec — out of scope. The spec never reads `system.runtime.queries` (only calls `kill_query`) — that bullet doesn't apply. Catalog/schema bullets overlap 105831. | | 105831 (schema/catalog browsing) | 4 new tests | `get_catalog_names` (from `PrestoBaseEngineSpec`) was previously only exercised through `PrestoEngineSpec`. Also characterizes that neither `get_schema_names` nor `get_catalog_names` has permission-denied handling — the raw driver exception propagates unmapped. | | 105832 (convert_dttm/types) | 5 new/extended cases | TIME → None (recognized type, unhandled by `convert_dttm`) and INTERVAL → None added to `test_convert_dttm`; INTERVAL added to `test_get_column_spec` (maps successfully, unlike convert_dttm); IPADDRESS/UUID/HyperLogLog fallback to `None`; ROW-nested-inside-MAP is not expanded (per `_expand_columns`'s own docstring). TIMESTAMP WITH TIME ZONE was already covered — not duplicated. | | 105833 (exception mapping) | 2 new tests | Existing `test_get_dbapi_exception_mapping` only exercised exact classes (`TrinoInternalError` etc., which happen to be Trino's own subclasses). New tests cover real polymorphism gaps: `TrinoConnectionError`/`TrinoAuthError` (subclass `OperationalError` but aren't Trino's bespoke wrappers) map correctly, while `Http502Error`/`IntegrityError` (outside the 3 handled `DatabaseError` categories) correctly fall through to the default. No regex tests added (none exist for Trino). | | 105834 (function names / extra metadata) | 4 new tests | `get_function_names` (from `PrestoBaseEngineSpec`) mirrors `test_presto.py`'s coverage, previously untested for Trino (generic pandas-shape edge cases aren't repeated — not Trino-specific). `get_extra_table_metadata` gets a genuinely-no-indexes case (vs. the existing Iceberg-filtered-to-empty cases) and a missing-table propagation case. | | 105872 (cancel_query injection) | 1 new parametrized test | `cancel_query` already calls `validate_cancel_query_id` with a safe regex on master, but no test asserted the negative path. New test confirms malformed/injection-style ids return `False` **and** `cursor.execute` is never called. | | 105873 (shared `g` across threads) | 1 characterization test | **sc-105873**. `execute_with_cursor`'s worker thread copies `g` via `setattr(g, key, value)` — a shallow copy. Pinned: mutating a dict attribute from the worker thread is visible in the original request's `g`, because both reference the same object. | | 105874/105878 (OAuth2) | 2 new tests | Malformed `encrypted_extra` JSON re-raises `JSONDecodeError` (real gap). One **characterization test (sc-105874)** for token-expiry mid-query: see "what surprised me" below — the "no refresh" framing needed correcting. `needs_oauth2`/OAuth2 detection was already well covered. | | 105875 (`_expand_columns` recursion) | 1 characterization test | **sc-105875**. No depth guard exists; a 50-level-deep nested ROW expands successfully, one column per level. Kept the depth well below Python's ~1000 default recursion limit, per the ticket's own note that the originally-proposed 20-level test would already pass today. | | 105876 (execute_with_cursor exceptions) | 2 new tests | A non-DB exception raised by the worker thread is re-raised unchanged in the calling thread, whether it happens before a query ID is ever assigned (loop exits via `execute_event`, not a hang) or after (clean stop, `handle_cursor` sees the real id). `time.sleep` patched for speed. | ### CHARACTERIZATION TESTS (pin current behavior, do not fix) - `test_execute_with_cursor_shares_mutable_g_state_across_threads` — sc-105873 - `test_execute_starts_oauth2_dance_without_refresh_attempt` — sc-105874 - `test_expand_columns_recursion_is_unbounded` — sc-105875 ### WHAT SURPRISED ME The original gap-analysis note for 105874 (based on reading `trino.py`/`base.py` alone) said OAuth2 "no refresh" was still present. Digging one layer deeper, that's **not quite right**: `superset/utils/oauth2.py` has a full token-refresh-and-retry mechanism (`execute_with_oauth2_retry`), wrapping `execute_with_cursor` from both `celery_task.py` and the sync executor, with its own dedicated tests. It only skips the retry once the query has made real progress (`can_retry=lambda: not query.progress`). So "no refresh happens" is true specifically for the **mid-query** case the ticket named (progress already made), not universally — I scoped the characterization test to the `TrinoEngineSpec.execute`-level behavior (no refresh attempted *at that layer*) and noted the fuller picture in the test's docstring rather than testing the generic retry wrapper itself, which isn't Trino-specific and already has its own coverage. ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/db_engine_specs/test_trino.py ``` All 152 tests pass; run 3x with no flakiness observed. `ruff check` and `ruff format --check` pass on the changed file. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API No production code was changed — `git diff --stat origin/master -- . ':!tests'` is empty. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
