sha174n commented on PR #39997: URL: https://github.com/apache/superset/pull/39997#issuecomment-4525880365
@rusackas Thanks for the review — addressing each point: **1. @patch decorator order:** The current state (mock_sm first, mock_find_by_id second) is correct: `unittest.mock.patch` applies decorators bottom-up, so the bottom-most `@patch(security_manager)` injects the first argument. I've added `mock_find_by_id.assert_called_once_with(1)` to all three tests in a8ad911 — these assertions would fail if the binding were ever swapped, since the security_manager mock isn't called as a function. **2. PR title / scope:** The title was already updated to `test(chart): document UpdateChartCommand ownership behavior with unit tests`. This is now a tests-only PR documenting the existing behavior — the original ownership-everywhere change was reverted in 47506ba to preserve the report-worker path. The query-context skip is intentional and part of the locked-in behavior these tests guard. **3. Stale Copilot import comment:** Resolved — imports were moved to module-level in e02c3f9. -- 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]
