rusackas opened a new pull request, #45501:
URL: https://github.com/apache/superset/pull/45501
### SUMMARY
`master`'s unit tests are currently red. #45251 ("add the provider execution
context contract") changed
`SemanticLayer.implementation`/`SemanticView.implementation` from
`@cached_property` to a plain `@property`, which several existing tests
elsewhere in the suite depended on being settable -- either via direct
assignment (`view.implementation = mock`) or the `__dict__` cache-population
backdoor (`view.__dict__["implementation"] = mock`) that only works against a
`cached_property`. A plain property has no setter and ignores instance
`__dict__`, so those tests broke.
This wasn't caught before merge: #45251's own CI run predates the `master`
it eventually landed on by about four days, and one of the now-broken patterns
(`test_compatible_api.py`, from #44454) was added to `master` during that gap,
so the combination was never actually tested together. Flagged to @mikebridge
on #45251 and separately worth asking whether branch protection should require
being up to date with `master` before merge, to prevent this exact class of gap.
**Fix:** update each affected test to patch the actual resolution path
instead of the property itself, rather than reverting #45251's design:
- `models_test.py`: two `SemanticView` tests now use the `PropertyMock`
pattern #45251 already established for the equivalent `SemanticLayer` tests in
the same file.
- `test_compatible_api.py`: patches `SemanticBindingMemo.bind` directly,
since this test exercises the real model resolver rather than a pre-populated
cache.
- `test_list_metrics.py` / `test_get_chart_data.py`: the same
`PropertyMock`/`monkeypatch` substitution for their own mock-injection helpers.
- `datasets_test.py`: this one needs two different `SemanticView` instances
to resolve `.implementation` to two different values in the same test (one
healthy, one deliberately ambiguous) -- dispatches on instance identity,
falling back to whatever is already installed (the `semantic_view` fixture's
own mock) for every other instance.
- `e2e_stub_extension_test.py` / the e2e semantic-stub extension itself:
`StubSemanticLayer.from_configuration()` was still missing the
`cache_token`/`context`/`credentials` keyword arguments #45251 added to the
provider contract.
No production code changed -- this is entirely test-side compatibility with
#45251's already-merged design.
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/semantic_layers/
tests/unit_tests/dashboards/datasets_test.py
tests/unit_tests/datasource/test_compatible_api.py
tests/unit_tests/mcp_service/semantic_layer/
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
```
All 2256 tests across these affected areas pass, confirmed against current
`master` directly (not just this branch).
### 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
🤖 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]
