fix(report): Capture unexpected errors in report screenshots. Fixes #21653
SUMMARY
As mentioned in https://github.com/apache/superset/issues/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
- [x] Has associated issue: #21653
- [x] Required feature flags: SCREENSHOT_REPLACE_UNEXPECTED_ERRORS
Codecov Report
Merging #21724 (d41cb66) into master (d41cb66) will not change coverage. The diff coverage is
n/a.
:exclamation: Current head d41cb66 differs from pull request most recent head 2c1e87f. Consider uploading reports for the commit 2c1e87f to get more accurate results
@@ 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
| Flag | Coverage Δ | |
|---|---|---|
| mysql | 77.95% <0.00%> (ø) |
|
| postgres | 78.02% <0.00%> (ø) |
|
| presto | 52.42% <0.00%> (ø) |
|
| python | 81.02% <0.00%> (ø) |
|
| sqlite | 76.48% <0.00%> (ø) |
|
| unit | 50.91% <0.00%> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
good stuff, have 2 questions.
- in case of error modal, should we send the alert to owner and not to the receivers?
- I think we should add 1 test case when there is error modal.
good stuff, have 2 questions.
1. in case of error modal, should we send the alert to owner and not to the receivers? I think two approach could both work. The reason I prefer sending it to receivers now is that even if some of the charts are broken in the screenshot, there might be other charts in the same screenshot that are valuable to the receivers. 2. I think we should add 1 test case when there is error modal. I was thinking about that but didn't figure out a way to mock an error. It might due to my experience with any front end related code. Will try to investigate more and see if I can add a test case where it shows an error modal.
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!
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! Updated