drivaspreset opened a new pull request, #45361: URL: https://github.com/apache/superset/pull/45361
### SUMMARY Closes test gaps in `TrinoEngineSpec` (`superset/db_engine_specs/trino.py`). **Tests only: no production code is changed.** This replaces #44661, which had fallen far behind `master`; it is re-implemented against current `master` and addresses all four review comments from that PR (see below). | | before | after | |---|---|---| | tests in `tests/unit_tests/db_engine_specs/test_trino.py` | 117 | 192 | | `trino.py` line coverage | 90% | 98% | (Measured with `coverage run --branch -m pytest ...` on the same commit before and after.) `TrinoEngineSpec` extends `PrestoBaseEngineSpec`, not `PrestoEngineSpec`, so several methods it uses are only reached through the generic base classes and were never pinned at the Trino layer. New coverage: - **Discovery:** `get_table_names` / `get_view_names` / `get_schema_names` / `get_catalog_names` (plain, empty, catalog-qualified, mixed case), and schema names containing `/`, spaces, `%`, `?`, non-ASCII round-tripping through `adjust_engine_params` / `get_schema_from_engine_params`. - **Types:** TIME / INTERVAL mapping, unmapped IPADDRESS / UUID / HyperLogLog, ROW nested in MAP / ARRAY, deeply nested ROW expansion, `convert_dttm` for TIME / INTERVAL. - **Exception mapping:** subclass and unmapped paths, message preservation. - **Metadata:** `get_extra_table_metadata` for non-partitioned tables, views, partitioned-but-empty tables and tables dropped mid-inspection (through a real `Database`, so the production `NoSuchTableError` handler runs); `get_function_names`; `Database.function_names` swallowing connection errors. - **Execution:** `cancel_query` rejecting malformed / injection-style IDs; worker-thread exception propagation in `execute_with_cursor` (before and after a query ID exists); Trino's 401 detection through `execute_with_oauth2_retry` (refresh and replay before progress, no replay after progress); the polling time limit; tracking URL; remaining small branches. **Characterization tests.** A few tests pin what the code does today even where that behavior looks questionable, so that a future fix shows up as a deliberate test change. Each says "Characterizes current behavior" and "Invert when fixed" in its docstring. They record: 1. Driver errors that arrive wrapped in a `sqlalchemy.exc` error (as they do through a real inspector) are not mapped to Superset DBAPI exceptions, while bare driver errors are. 2. Trino defines no `custom_errors`, so every error surfaces as `GENERIC_DB_ENGINE_ERROR`. 3. `execute_with_cursor` copies Flask's `g` into the worker thread shallowly, so mutable values are shared. 4. `_expand_columns` has no recursion-depth guard. 5. A `BaseException` (e.g. `KeyboardInterrupt`) in the worker thread is not recorded, so `execute_with_cursor` returns normally. Happy to drop any of these if reviewers would rather not pin current behavior. **Review feedback from #44661, addressed** - A missing-table test mocked `get_indexes` and so bypassed the production handler: it now runs through a real `Database.get_indexes`. - Permission-denied tests asserted a bare driver exception SQLAlchemy would have wrapped: they now use `sqlalchemy.exc.ProgrammingError` with `.orig`. - Worker-exception tests inspected `cursor.query_id` after the call: `handle_cursor` now records it at call time. - An OAuth test's docstring described a mid-flight case it did not exercise: replaced by tests that go through `execute_with_oauth2_retry`. **Unrelated flakiness fix in the same file.** `test_auth_custom_auth_denied` rebinds `superset.config.ALLOWED_EXTRA_AUTHENTICATIONS` and never restores it, which made `test_auth_custom_auth` fail under random test ordering (5 of 8 runs on an untouched copy of `master`'s file). It now uses `patch.dict`. ### TESTING INSTRUCTIONS ```bash pytest tests/unit_tests/db_engine_specs/test_trino.py ``` Expected: 192 passed. The file was also run 100 times in random order, 10 times with `-n 4`, and 15 times under CPU load, with no failures. The whole `tests/unit_tests/db_engine_specs` directory was run 20 times in a row with no failures. Coverage for `trino.py`: ```bash coverage run --branch --include='superset/db_engine_specs/trino.py' -m pytest tests/unit_tests/db_engine_specs/test_trino.py coverage report -m ``` ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
