mikebridge opened a new pull request, #45499:
URL: https://github.com/apache/superset/pull/45499
### SUMMARY
Master's Python unit tests went red when several semantic-layer changes that
each passed CI alone were merged on the same day.
#45251 (the provider execution-context contract) changed two contracts, but
tests and a test provider merged earlier by other PRs still used the old ones:
1. **`SemanticView.implementation` is read-only.** It used to be a
`cached_property`, so a test could override the provider for one view with
`view.implementation = mock` or `view.__dict__["implementation"] = mock`. It is
now a plain property that binds the view through its layer's binding memo.
- Assigning to it raises `AttributeError: property 'implementation' of
'SemanticView' object has no setter`.
- Writing to `__dict__` is silently ignored, so the test talks to a
different provider than it set up.
2. **`SemanticLayer.from_configuration` takes the host's execution
identity.** Providers must now accept keyword-only `cache_token` and `context`,
plus an optional `credentials`. The E2E stub provider, which the Playwright
suite loads as a real extension, still had the old one-argument signature. The
host therefore failed to construct it: `from_configuration() got an unexpected
keyword argument 'cache_token'`.
**What this changes**
- **Tests:** each affected test now injects its provider the way #45251's
own tests do. It patches `SemanticView.implementation` with a `PropertyMock` or
a `property`. Where a test has two views with different providers, a
per-instance `property` keeps them apart.
- **No assertions are changed.**
- **The E2E stub provider** (`tests/e2e_extensions/semantic_stub`)
implements the new `from_configuration` signature and stores the supplied
`context` and `cache_token`, as the SDK contract asks. Its unit tests construct
it the way the host does.
- **No other provider in the repository uses the old signature, and no
production code assigns `implementation`.**
- A leftover `__dict__` write in `query_errors_test.py` is removed. That
test's fixture already patches the property, so the line had no effect.
**Failures fixed** (failing master run:
https://github.com/apache/superset/actions/runs/39316150460)
| Failing test | From | Old contract it relied on | Fix |
|---|---|---|---|
|
`semantic_layers/models_test.py::test_column_metadata_default_is_order_independent[*]`,
`::test_column_metadata_rejects_ambiguous_grain_ids`,
`::test_compatibility_resolves_grain_variants[*]` (8) | #44454 |
`view.implementation = mock` | Patch the property with `PropertyMock` |
|
`datasource/test_compatible_api.py::test_compatible_endpoint_resolves_semantic_variants[*]`
(2) | #44454 | `view.implementation = provider` | Patch the property with
`PropertyMock` |
|
`mcp_service/semantic_layer/tool/test_list_metrics.py::test_list_metrics_{default,embedded}_page_byte_bound`
(2 errors, in a fixture) | #44508 / #44454 | `semantic_view.implementation =
...` | Patch the property for the fixture's lifetime |
|
`dashboards/datasets_test.py::test_dashboard_datasets_isolate_ambiguous_view[True]`
| #44454 | `ambiguous.__dict__["implementation"] = ...`. Since the shared
fixture patches the property, this was ignored and the "ambiguous" view served
the healthy provider | Per-instance `property`: the ambiguous view gets its
provider and the healthy view keeps the fixture's |
|
`mcp_service/chart/tool/test_get_chart_data.py::test_{saved,unsaved}_chart_mapper_refusal_is_validation_error`
| #45360 | `view.__dict__["implementation"] = provider`, ignored, so the
mapper never saw the empty-filter provider | `monkeypatch` the property |
|
`semantic_layers/e2e_stub_extension_test.py::test_stub_runtime_schema_through_host_api`
| #45103 / #45401 | Stub provider's `from_configuration(cls, configuration)` |
Stub provider implements the new signature; the stub tests pass a test
execution context |
Every failure is already present at #45251's own merge commit. The two
semantic-layer merges after it added no new failures.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI change.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/semantic_layers tests/unit_tests/datasource \
tests/unit_tests/dashboards tests/unit_tests/mcp_service
```
- **Before:** on current `master`, the listed tests fail: 14 failed and 2
errors.
- **After:** with this change, they pass.
### 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]
