drivaspreset opened a new pull request, #43607:
URL: https://github.com/apache/superset/pull/43607
### SUMMARY
Closes the unit-test gaps in `PrestoEngineSpec`. **No production code
changes** — this is tests
only
Most of this file's behaviour was either proven only in the integration
suite (which needs an app
and a metadata DB) or not proven at all. Several methods were completely
uncovered: `handle_cursor`,
`get_tracking_url`, `get_function_names`, `get_allow_cost_estimate`,
`get_catalog_names`, and the
`PrestoBaseEngineSpec.convert_dttm` override. That matters because the logic
here is pure and
deterministic — reproducing its failures end-to-end needs a live Presto
cluster, but pinning it down
needs only mocks.
`superset/db_engine_specs/presto.py`: **306 → 161 missed statements (34% →
63%)**, and
**+86 test cases** (`tests/unit_tests/db_engine_specs/test_presto.py` 47 →
131,
`tests/unit_tests/models/core_test.py` 77 → 79).
The commits are grouped by behaviour area and are readable in order:
| Commit | Area | Cases |
|---|---|---:|
| 1 | error regexes and `_extract_error_message` | 19 |
| 2 | `convert_dttm` precision and `column_type_mappings` | 17 |
| 3 | `handle_cursor` and `get_tracking_url` | 9 |
| 4 | cost estimation | 13 |
| 5 | view, table and catalog discovery | 18 |
| 6 | `get_function_names` | 4 |
| 7 | `expand_data` | 4 |
| 8 | `Database.function_names`, null cost estimate | 3 |
#### Highlights
**All nine `custom_errors` regexes are now asserted.** Previously only the
HTTP 401 pattern had a
test, so a typo in any of the other eight shipped silently — invisible until
a user hit that exact
error. This is the most directly user-facing logic in the file: the
difference between
*"Either the username "bob" or the password is incorrect"* and a raw pyhive
string in a red toast.
**The Stop button.** `handle_cursor` is what notices a user clicked Stop —
the click only updates the
query's status in the metadata database, and the `cursor.cancel()` this loop
issues is what actually
releases the Presto cluster resources. That branch had no test; had it
regressed, Stop would appear
to work in the UI while the query kept running.
**The millisecond/microsecond override.** `PrestoEngineSpec.convert_dttm`
deliberately overrides
`PrestoBaseEngineSpec.convert_dttm` to render TIMESTAMP literals with
millisecond rather than
microsecond precision, and nothing pinned that. It is also this file's only
silent-failure mode: a
wrong-precision literal produces a *valid* query returning the wrong rows,
with no error. A refactor
collapsing the two methods now fails a test instead of quietly shifting
every Presto timestamp
filter.
**Schema round-trip.** `adjust_engine_params` percent-quotes the schema into
the URI path with
`safe=""` and `get_schema_from_engine_params` unquotes it. Any asymmetry
between the pair silently
points SQL Lab at the wrong schema, and a schema containing `/` would be
read back as a different
name entirely. Tested as a round-trip over `/`, spaces, `%` and non-ASCII,
since testing either half
alone cannot catch a mismatched pair.
#### Mirrored integration tests
Twelve cases mirror tests that already exist in
`tests/integration_tests/db_engine_specs/presto_tests.py`, reusing that
suite's known-good
expectations. **The integration originals are intentionally left in place —
that file is untouched by
this PR** (`git diff --stat` shows no changes to it). Each mirror's
docstring names its original by
file and line, so the relationship is greppable rather than accidental.
Removing the now-duplicated
integration tests, if ever wanted, is separate work.
#### Characterization tests — 8 tests that assert current, incorrect
behaviour
These pass by pinning bugs, not by endorsing them. Each is labelled
`CHARACTERIZATION test` in its
docstring and explains what a fix would change. No `xfail` anywhere — that
would silently mask the
gap. Happy to split any of these out or drop them if maintainers would
rather they land alongside
their fixes:
1. **`extract_errors` raises `KeyError` when `context` is omitted.** Four of
the nine patterns
declare message placeholders their own regex never captures (`username`,
`hostname`, `port`), so
the eager-`gettext` `str % dict` raises. Reachable from real call sites.
2. **`handle_cursor` raises `TypeError` on missing split counts.**
`completedSplits`/`totalSplits`
are read with `stats.get(...)` and passed straight to `float()`, so a
poll reporting a state
without split counts raises inside the SQL Lab worker rather than
degrading to "no progress
information".
3. **`get_allow_cost_estimate` raises `InvalidVersion` on a malformed
`version`.** An admin typo in
a database's Extra JSON raises rather than simply disabling cost
estimation.
4. **`query_cost_formatter` raises `TypeError` on a null estimate value.**
`humanize` guards
`int(value)` with `except ValueError` only, but a JSON `null` arrives as
`None`. One null field
breaks the whole cost panel — note that an *absent* key is handled
cleanly, so the asymmetry is
the bug.
5. **`expand_data` crashes on malformed JSON — two unguarded sites, one test
each.** `destringify`
is `json.loads` with no `try/except` (`superset/result_set.py`), reached
from both the ARRAY
branch and the ROW branch. Behind `PRESTO_EXPAND_DATA`, which defaults
off. Both sites are
pinned, since a fix guarding only one would leave the other crashing.
6. **`get_function_names` raises `KeyError` on a DataFrame without a
`Function` column.** Included
because "handles an empty function list" has two readings that behave
differently; both are now
asserted explicitly.
7. **The "table does not exist" message double-quotes the table name.**
`TABLE_DOES_NOT_EXIST_REGEX` captures the surrounding single quotes, so
users see
`The table "'default.foo'" does not exist.` Cosmetic, and changing
user-facing copy is a product
decision.
#### Two other findings, not addressed here
-
`tests/integration_tests/db_engine_specs/presto_tests.py::test_get_catalog_names`
returns early
unless the example database is Presto, and asserts against a **list**
while `get_catalog_names`
returns a **set** — it would fail if it ever ran. Left alone deliberately
(that file is untouched);
the unit test added here is the method's first real coverage.
- `get_create_view` interpolates `schema` and `table` into `SHOW CREATE
VIEW` with an f-string rather
than binding them. Those values come from the metadata database rather
than user input, so this is
not a security finding under `SECURITY.md`; one test case pins the
interpolation so the assumption
is visible if that ever changes.
Happy to open GitHub issues for any of the above that maintainers consider
real — I did not want to
file eight issues speculatively.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes. The meaningful before/after is coverage of
`superset/db_engine_specs/presto.py`:
```
BEFORE 508 statements 306 missed 180 branches 13 partial 34%
AFTER 508 statements 161 missed 180 branches 22 partial 63%
```
Methods that went from **entirely uncovered** to covered:
| Lines | Method |
|---|---|
| 308-315 | `PrestoBaseEngineSpec.convert_dttm` (microseconds) |
| 340-341 | `get_catalog_names` |
| 402-419 | `estimate_statement_cost` |
| 431-462 | `query_cost_formatter` |
| 1029-1030 | `get_allow_cost_estimate` |
| 1077 | `get_table_names` |
| 1105-1127 | `get_view_names` |
| 1235-1319 | `expand_data` |
| 1374-1385 | `get_create_view` |
| 1389-1393 | `get_tracking_url` |
| 1398-1442 | `handle_cursor` |
| 1451-1460 | `_extract_error_message` |
One caveat worth stating: **line coverage understates the change for the
error regexes.** The loop in
`BaseEngineSpec.extract_errors` already read as covered after a single
pattern matched, while 8 of 9
patterns had no assertion on their output at all. Read the case counts
alongside the percentages.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/db_engine_specs/test_presto.py
tests/unit_tests/models/core_test.py
```
210 tests, ~1.3s. No app, no database, no Presto cluster, no network — all
mocked.
Coverage, for the numbers above:
```bash
python -m coverage run -m pytest
tests/unit_tests/db_engine_specs/test_presto.py
python -m coverage report --include="*/db_engine_specs/presto.py"
--show-missing
```
To confirm the integration suite is untouched:
```bash
git diff --stat master..HEAD -- tests/integration_tests/ # expect no output
```
### 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
None of the above applies: tests only
--
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]
