fix(reports): propagate PlaywrightTimeout so execution transitions to ERROR state

[eschutho]

SUMMARY

When Playwright times out waiting for a chart to load, the execution should transition to a clean ERROR state. The previous code swallowed the exception with pass in the outer except PlaywrightTimeout block, causing get_screenshot() to return None silently. This took a late code path (if not imges) that could be bypassed when partial screenshots existed, lost the original error message, and left the execution in WORKING state — blocking all future runs of the same report until a manual DB reset.

Root cause (superset/utils/webdriver.py):

# Before (buggy)
except PlaywrightTimeout:
    # raise again for the finally block, but handled above
    pass  # swallows the exception; get_screenshot() returns None

# After (fixed)
except PlaywrightTimeout:
    raise  # propagates to _get_screenshots(), converted to ReportScheduleScreenshotFailedError

The finally: browser.close() block runs regardless of whether an exception is raised, so the comment was also incorrect.

Propagation chain after fix:

1. PlaywrightTimeout raised in WebDriverPlaywright.get_screenshot()
2. Caught by except Exception as ex in _get_screenshots() → re-raised as ReportScheduleScreenshotFailedError(f"Failed taking a screenshot {str(ex)}")
3. Caught by state machine → update_report_schedule_and_log(ReportState.ERROR)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — state machine behavior change, no UI change.

TESTING INSTRUCTIONS

1. Run the updated unit test:

   pytest tests/unit_tests/utils/webdriver_test.py::TestWebDriverPlaywrightErrorHandling::test_get_screenshot_raises_on_element_wait_timeout
   

2. To verify end-to-end: configure a report against a chart that never finishes loading (e.g. broken datasource), trigger the report, and confirm the execution log shows Error state rather than staying Working.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

[bito-code-review]

Code Review Agent Run #31a07b

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 6f6dd0d..6f6dd0d

  • superset/utils/webdriver.py
  • tests/unit_tests/utils/webdriver_test.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codeant-ai-for-open-source]

Suggestion: This test can produce false positives when Playwright is not installed because PlaywrightTimeout is aliased to Exception, so pytest.raises(PlaywrightTimeout) will also accept unrelated exceptions. Capture the raised exception and verify it is the exact timeout object injected via mock_element.wait_for.side_effect, and assert the specific timeout log call to ensure the failure came from the element-wait timeout path. [logic error]

Severity Level: Major ⚠️

- ⚠️ Unit test may pass for wrong exception type.
- ⚠️ Timeout path regressions may go undetected without Playwright.
- ⚠️ Weaker coverage for Playwright-based report screenshots.

Suggested change

	with pytest.raises(PlaywrightTimeout):
	driver.get_screenshot(
	"http://example.com", "test-element", mock_user
	)
	assert result is None
	# Should log timeout for element wait
	# Should log the timeout before re-raising
	assert mock_logger.exception.call_count >= 1
	expected_timeout = mock_element.wait_for.side_effect
	with pytest.raises(PlaywrightTimeout) as exc_info:
	driver.get_screenshot(
	"http://example.com", "test-element", mock_user
	)
	assert exc_info.value is expected_timeout
	# Should log the timeout before re-raising
	mock_logger.exception.assert_any_call(
	"Timed out requesting url %s", "http://example.com"
	)

Steps of Reproduction ✅

1. Open `superset/utils/webdriver.py` and verify that when Playwright cannot be imported,
`PlaywrightTimeout` is aliased to `Exception` at lines 72–76 (`PlaywrightTimeout =
Exception`).

2. Open `tests/unit_tests/utils/webdriver_test.py` and locate
`test_get_screenshot_raises_on_element_wait_timeout` at line 643, which uses `with
pytest.raises(PlaywrightTimeout): driver.get_screenshot(...)` around the call to
`WebDriverPlaywright.get_screenshot`.

3. Note that in an environment without `playwright.sync_api` installed,
`PlaywrightTimeout` imported in this test (`from superset.utils.webdriver import
PlaywrightTimeout` at line 647) is the same alias to `Exception`, so
`pytest.raises(PlaywrightTimeout)` is effectively `pytest.raises(Exception)` and will
succeed for any exception subclass, not just the timeout coming from
`mock_element.wait_for`.

4. Also observe that the test only asserts `mock_logger.exception.call_count >= 1` (line
695), while `WebDriverPlaywright.get_screenshot` logs multiple different exception paths
in `superset/utils/webdriver.py` (e.g. timeout-specific log at line 283 and generic error
log at lines 132–135), so any logged exception plus any raised `Exception` will make the
test pass, allowing regressions in the specific element-wait timeout path to go undetected
when Playwright is not installed.

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** tests/unit_tests/utils/webdriver_test.py
**Line:** 689:695
**Comment:**
	*Logic Error: This test can produce false positives when Playwright is not installed because `PlaywrightTimeout` is aliased to `Exception`, so `pytest.raises(PlaywrightTimeout)` will also accept unrelated exceptions. Capture the raised exception and verify it is the exact timeout object injected via `mock_element.wait_for.side_effect`, and assert the specific timeout log call to ensure the failure came from the element-wait timeout path.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

👍 | 👎

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.45%. Comparing base (5106afb) to head (be9e07c).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/utils/webdriver.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #39176   +/-   ##
=======================================
  Coverage   64.45%   64.45%           
=======================================
  Files        2541     2541           
  Lines      131662   131662           
  Branches    30517    30517           
=======================================
  Hits        84863    84863           
  Misses      45333    45333           
  Partials     1466     1466           

Flag	Coverage Δ
hive	40.08% <0.00%> (ø)
mysql	60.75% <0.00%> (ø)
postgres	60.83% <0.00%> (ø)
presto	40.09% <0.00%> (ø)
python	62.41% <0.00%> (ø)
sqlite	60.46% <0.00%> (ø)
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #8d4c9a

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 6f6dd0d..be9e07c

  • tests/unit_tests/utils/webdriver_test.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[Vitor-Avila]

Do we need to also raise on this PlaywrightError block?

[eschutho]

I don't think it's required because _get_screenshots() will raise it.

[Vitor-Avila]

left a non-blocking comment checking if we need a similar change in the other except block