EnxDev commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4060343285
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else
None
+ ),
)
if imge is None:
raise ReportScheduleScreenshotFailedError(
"Screenshot failed; aborting to avoid sending a
partial report"
)
+ if for_delivery:
+ self._validate_screenshot(imge)
Review Comment:
Could we carry the tiled capture's validation result through this gate too?
The latest fix records screenshot approval only in
`_get_validated_screenshot()`. `take_tiled_screenshot()` returns its combined
image without that approval, so an accepted empty/error-only dashboard still
fails here with `blank_final_image` when it uses tiling.
This also affects a dashboard that does contain validated chart content:
extending `test_sparse_contentful_tile_is_not_diluted_by_empty_tiles` with
`validate_report_screenshot(result, report_context)` reproduces the failure.
Its content-bearing tile and the corresponding combined region both pass, but
the surrounding empty tiles make the whole-image classifier reject it. Could we
extend both that test and
`test_blank_combined_image_is_allowed_for_terminal_empty_states` through final
validation, preserving the tiled result while still checking the final bytes
for corruption?
##########
superset/utils/webdriver.py:
##########
@@ -721,25 +755,50 @@ def _wait_for_charts_ready( # noqa: C901
)
return
chart_holder_states = page.evaluate(FIND_CHART_HOLDER_STATES_JS)
- mounted_holders = len(chart_holder_states)
- ready_holders = sum(
- holder.get("state") in ready_states for holder in
chart_holder_states
+ diagnostics =
ChartHolderDiagnostics.from_holder_states(chart_holder_states)
Review Comment:
Could the final diagnostics use a scan that includes the actual states of
off-screen holders for a standard report? `FIND_CHART_HOLDER_STATES_JS` returns
`virtualized` before checking for an error whenever a holder is below the
viewport. This path forces those holders to render and includes them in the
full-page screenshot, so their errors are part of the delivered report.
With one rendered chart in view and an error panel below it, the actual JS
scan produces `rendered` + `virtualized`; this logs `error_holders=0`,
`semantic_success=True`, and no `report_semantic_status` warning. A
full-dashboard diagnostic scan here would make the warning policy cover the
same content as the readiness check and screenshot.
##########
superset/commands/report/execute.py:
##########
@@ -808,12 +810,16 @@ def _get_screenshots(self) -> list[bytes]:
imge = screenshot.get_screenshot(
user=user,
log_context=self._log_context,
- report_execution_context=self._report_execution_context,
+ report_execution_context=(
+ self._report_execution_context if for_delivery else
None
Review Comment:
Could we preserve the execution deadline when opting bootstrap captures out
of delivery validation? Passing `None` also changes `_wait_for_charts_ready()`
from the report's remaining budget to `SCREENSHOT_LOAD_WAIT`, which defaults to
60 seconds. A scheduled CSV/XLSX/TEXT report with no saved query context can
therefore fail on a chart taking 90 seconds to render, even with plenty of
execution budget left.
I reproduced the timeout change with the actual readiness helper and a
mocked browser: a 900-second context with the usual reserves allows 690
seconds; removing that context caps the same wait at 60. Keeping deadline
propagation separate from the delivery-validation opt-out would avoid this
regression.
--
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]
