eschutho commented on code in PR #42816:
URL: https://github.com/apache/superset/pull/42816#discussion_r3731794315
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
Review Comment:
Confirmed and fixed in `52f16c59bd` — this was the best catch of the review.
The per-tile evaluate now carries the same guard as the final block, and
`test_per_tile_diagnostics_failure_does_not_discard_capture` pins the
regression: the diagnostics evaluate raising a non-timeout error ("Execution
context was destroyed") on all three tiles still yields a successful combined
capture with a warning per tile.
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ holder_states
+ )
+ _record_visible_holder_states(
+ holder_states,
+ tile_number=i + 1,
+ )
+ elapsed, remaining = _deadline_values()
+ logger.info(
+ "report_readiness_tile url=%s expected_holders=%s "
Review Comment:
Fair — added in `52f16c59bd`: `test_tile_line_reports_mixed_holder_states`
drives 2 rendered + 1 empty + 1 error + 1 unready through the per-tile emitter
and asserts every field substring in the rendered message (a swap between any
two counters now fails), plus
`test_final_semantic_status_fires_exactly_once_for_error_holders` covers the
semantic-status branch. I stopped short of one-per-emitter across all ~9 call
sites — the counters all come from the same
`ChartHolderDiagnostics.from_holder_states`, which has its own mixed-state unit
tests, so the residual risk is per-emitter arg-ordering; the two
highest-traffic emitters (tile + final) are now pinned and the rest share the
pattern. Happy to extend to the Selenium pair if you want it.
##########
superset/utils/screenshot_utils.py:
##########
@@ -610,6 +661,60 @@ def _raise_if_budget_exhausted() -> None:
raise
else:
tile_elapsed = time.monotonic() - tile_wait_start
+ if report_execution_context:
+ holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ holder_states
+ )
+ _record_visible_holder_states(
+ holder_states,
+ tile_number=i + 1,
+ )
+ elapsed, remaining = _deadline_values()
+ logger.info(
+ "report_readiness_tile url=%s expected_holders=%s "
+ "mounted_holders=%s ready_holders=%s
rendered_holders=%s "
+ "empty_holders=%s error_holders=%s "
+ "virtualized_holders=%s unready_holders=%s "
+ "semantic_success=%s semantic_policy=%s tile=%s/%s "
+ "tile_elapsed_seconds=%.2f elapsed_seconds=%.2f "
+ "remaining_seconds=%s%s",
+ url,
+ report_execution_context.expected_chart_count,
+ diagnostics.mounted_holders,
+ diagnostics.ready_holders,
+ diagnostics.rendered_holders,
+ diagnostics.empty_holders,
+ diagnostics.error_holders,
+ diagnostics.virtualized_holders,
+ diagnostics.unready_holders,
+ diagnostics.semantic_success,
+ CHART_HOLDER_SEMANTIC_POLICY,
+ i + 1,
+ num_tiles,
+ tile_elapsed,
+ elapsed,
+ f"{remaining:.2f}" if remaining is not None else None,
+ context_suffix,
+ )
+ if diagnostics.error_holders:
Review Comment:
Agreed and changed in `52f16c59bd`, with one nuance: rather than demoting to
INFO, the per-tile WARNING is removed entirely (the `report_readiness_tile`
INFO line already carries `error_holders` per tile, so it was pure duplication)
and the final-block WARNING stays a WARNING. Rationale for keeping the final
one warm: `deliver_terminal_errors_with_warning` is the policy — the
deliverable went out with broken content, and the single WARNING per capture is
the operator-visible half of that contract. One error chart now produces
exactly one WARNING per run instead of N+1;
`test_final_semantic_status_fires_exactly_once_for_error_holders` pins it.
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -918,9 +918,9 @@ def test_spinner_timeout_logs_warning_and_raises(
assert "terminal_reason=readiness_timeout" in warning_call.args[0]
assert warning_call.args[1] == "http://example.com";
assert warning_call.args[3] == 1 # mounted holders
- assert warning_call.args[4] == 0 # ready holders
- assert warning_call.args[7] == 60
- assert warning_call.args[9] == [{"chartId": "42", "state":
"nothing_mounted"}]
+ assert warning_call.args[9] == 1 # unready holders
+ assert warning_call.args[16] == [{"chartId": "42", "state":
"nothing_mounted"}]
+ assert warning_call.args[17] == [{"chartId": "42", "state":
"nothing_mounted"}]
Review Comment:
Not a bug: `call.args` includes the format string at index 0, so a
`logger.warning` with 17 format arguments has indices 1–17 and `args[17]` is
the final `states` payload. These exact assertions run green in this PR's suite
(and have on two other branches carrying the same test).
##########
tests/unit_tests/utils/webdriver_test.py:
##########
@@ -1856,8 +1856,8 @@ def test_unready_diagnostics_logged_early_and_at_failure(
diagnostics,
)
failure_args = mock_logger.warning.call_args.args
- assert failure_args[9] == diagnostics
- assert failure_args[10] == diagnostics
+ assert failure_args[16] == diagnostics
+ assert failure_args[17] == diagnostics
Review Comment:
Same as the sibling comment: `call.args[0]` is the format string, so 17
format args occupy indices 1–17 and `failure_args[17]` is in bounds. The test
passes in this PR's CI.
--
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]
