Abdulrehman-PIAIC80387 commented on PR #43711: URL: https://github.com/apache/superset/pull/43711#issuecomment-5489731929
Thanks for the deep review @aminghadersohi — all resolved in `e06be4f`. - **#1 (ordering):** `side_effect` list now asserts `call_order == ["seed", "run"]`; fails if the seed drifts after `run()`. - **#2 (inert patch):** Dropped the `has_request_context` patch (compile.py never imports it). New test runs the real `set_query_context_form_data` and asserts `g.form_data` carries the datasource + serialized query — proves the payload is usable, not just that a mock fired. - **#5 (nit):** Dropped `current_user_email` from both docstrings. - **#3:** Agreed — coverage floor already held by `test_query_context_form_data_supports_request_dependent_jinja_macros`. Compile-path e2e can be a follow-up. ### Follow-ups / clarifications - **#4 (`g.form_data` teardown):** Pre-existing across all four peers; a shared teardown feels like its own PR. Happy to file a follow-up issue if you'd like. - **#6 (conflict with #43572):** Checked #43572 — its files are `chart_utils.py`, `plugins/*`, `schemas.py`, `get_chart_data.py`, `get_chart_type_schema.py`, `test_bubble_chart.py`. Doesn't touch `compile.py` or `preview_utils.py`, so no conflict. Did you mean a different PR? The identity-resolution note is really useful on the record. Thanks again. -- 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]
