bito-code-review[bot] commented on code in PR #44465:
URL: https://github.com/apache/superset/pull/44465#discussion_r4066846264
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working.
Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture
implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the
execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not
a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error
is
+not successful chart data. Logs distinguish `rendered_holders`,
`empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate. It correctly
identifies that the term 'terminal' in the documentation could be
misinterpreted as a complete capture failure, whereas the code indicates that
terminal chart errors are still considered capture-ready and are handled with a
warning. Updating the wording to clarify that the readiness wait stops without
retry, rather than blocking delivery, improves the accuracy of the
documentation.
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -478,6 +478,37 @@ There are many reasons that reports might not be working.
Try these steps to che
### Blank PDF or PNG captures
+Final screenshot bytes are validated independently of the capture
implementation.
+PDF delivery requires evidence that the exact PDF was assembled from validated
+images during this execution. Capture rejection remains sticky for the
execution;
+substituting another attachment cannot clear it. Detection is a heuristic, not
a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a blank
+classification. Extremely sparse legitimate output can therefore be rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
+
+Chart error and empty states are terminal for readiness, but a rendered error
is
+not successful chart data. Logs distinguish `rendered_holders`,
`empty_holders`,
+`error_holders` and `semantic_success`; `report_semantic_status` warns when a
+completed capture includes terminal chart errors.
+
+This is not a guarantee that every delivered image contains useful data. A
large
+enough title or logo can still exceed the detector's content thresholds while
the
+rest of the page is incomplete; removing the sparse-content override does not
+solve that general limitation. Recognized terminal chart errors are
capture-ready
+and may be delivered with a warning, provided the
+image passes visual validation. Operators requiring all charts to contain data
+must not treat this policy as that guarantee. The strict invariant is that an
Review Comment:
<!-- Bito Reply -->
The suggestion to consolidate the disclaimer text is appropriate. The
updated text in the provided diff successfully merges the three separate
caveats into a single, cohesive paragraph, which improves the readability and
scannability of the troubleshooting section.
**docs/admin_docs/configuration/alerts-reports.mdx**
```
+Final screenshot bytes are validated independently of the capture
implementation.
+PDF delivery requires evidence that the exact PDF was assembled from
validated
+images during this execution. Capture rejection remains sticky for the
execution;
+substituting another attachment cannot clear it. Detection is a heuristic,
not a
+guarantee that every chart contains correct data. Uniform fills are rejected
+regardless of colour or theme. A small label or logo does not override a
blank
+classification. Extremely sparse legitimate output can therefore be
rejected;
+a solid-fill KPI without visible structure has no pixel-only exemption.
```
##########
tests/unit_tests/utils/test_report_execution.py:
##########
@@ -44,6 +46,48 @@ def _report_config(**overrides: int) -> dict[str, int |
bool]:
return config
+def test_chart_holder_diagnostics_separate_terminal_errors_from_success() ->
None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "rendered"},
+ {"chartId": "3", "state": "empty"},
+ {"chartId": "4", "state": "error"},
+ {"chartId": "5", "state": "virtualized"},
+ ]
+ )
+
+ assert diagnostics.mounted_holders == 5
+ assert diagnostics.ready_holders == 5
+ assert diagnostics.rendered_holders == 2
+ assert diagnostics.empty_holders == 1
+ assert diagnostics.error_holders == 1
+ assert diagnostics.virtualized_holders == 1
+ assert diagnostics.unready_holders == 0
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_count_unready_holders() -> None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states(
+ [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "waiting_on_database"},
+ {"chartId": "3", "state": "nothing_mounted"},
+ ]
+ )
+
+ assert diagnostics.ready_holders == 1
+ assert diagnostics.unready_holders == 2
+ assert diagnostics.semantic_success is False
+
+
+def test_chart_holder_diagnostics_do_not_treat_zero_holders_as_success() ->
None:
+ diagnostics = ChartHolderDiagnostics.from_holder_states([])
+
+ assert diagnostics.mounted_holders == 0
+ assert diagnostics.semantic_success is False
Review Comment:
<!-- Bito Reply -->
The user has addressed the reviewer's suggestion by adding docstrings to the
three new test functions in `tests/unit_tests/utils/test_report_execution.py`.
This change aligns the code with the repository's documentation requirements.
--
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]
