rusackas commented on PR #45251: URL: https://github.com/apache/superset/pull/45251#issuecomment-6278876969
Heya @mikebridge, this broke `master`'s unit tests once it landed. `implementation` went from `@cached_property` to a plain `@property` on both `SemanticLayer` and `SemanticView` here, which several existing tests elsewhere in the suite depended on for mocking (`view.implementation = mock`, or the `__dict__["implementation"] = mock` cached-property trick) -- a plain property has no setter and ignores `__dict__`, so those broke. Looks like your own CI run here predates the change (log timestamps show 2026-10-15, four days before merge), so it never saw this against current `master` -- in particular commit `21fad62645` from #44454 added one of the now-broken patterns three days after that run. Given `master` moved a fair bit in between, might be worth asking whether `apache/superset`'s branch protection should require "up to date with base branch" before merge, since this is exactly the kind of gap that catches. Opening a fix PR now for the breakage (updates the ~6 affected test files to the registry-patch/`PropertyMock` pattern you already used for the `SemanticLayer` tests here, plus the `StubSemanticLayer` e2e extension's `from_configuration` signature, which was still missing `cache_token`/`context`/`credentials`). Flagging here in case you'd rather take it yourself since you've got the full context, or want to sanity-check the approach. -- 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]
