-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(report): Capture unexpected errors in report screenshots. Fixes #21653 #21724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21724 +/- ##
=======================================
Coverage 66.76% 66.76%
=======================================
Files 1847 1847
Lines 70562 70562
Branches 7742 7742
=======================================
Hits 47110 47110
Misses 21451 21451
Partials 2001 2001
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
good stuff, have 2 questions.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. This looks good, please address the comments and rebase
def test_not_call_find_unexpected_errors_if_feature_disabled( | ||
self, mock_find_unexpected_errors, mock_firefox, mock_webdriver_wait | ||
): | ||
app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the default no need to set it
url = get_url_path("Superset.dashboard", dashboard_id_or_slug=1) | ||
webdriver_proxy.get_screenshot(url, "grid-container", user=user) | ||
|
||
assert mock_find_unexpected_errors.called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please set the config back to it's expected state
app.config["SCREENSHOT_REPLACE_UNEXPECTED_ERRORS"] = False
Hi @zhaorui2022 - just following up to see if you can address comments and resolve the merge conflicts. Would love to get this across the finish line! Holler if we can help! Thanks for the contribution! |
…lace unexpected errors with real error messages in screenshots
5d3b957
to
2c1e87f
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me like comments have been addressed and we're all set here! I'll merge shortly if there's no further reason from @dpgaspar not to do so!
SUMMARY
As mentioned in #21653, there are cases that users receive a screenshot with "Unexpected errors" and "See More" links and there is no error on the server side. It might be caused by transient errors and the error is already gone after opening the dashboard again. In this case, there is no way to know what exactly happened and why we got the errors. Adding an option to log those errors to help debug issues and improve systems, and also reveal the errors to users to understand what happens.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Case 1
Before
After
Case 2
Before
After
TESTING INSTRUCTIONS
Tested locally with some broken dashboards. Screenshots attached.
ADDITIONAL INFORMATION