Skip to content
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

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

zhaorui2022
Copy link
Contributor

@zhaorui2022 zhaorui2022 commented Oct 6, 2022

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

screenshot_before_1

After

screenshot_after_1

Case 2

Before

screenshot_before_2

After

screenshot_after_2

TESTING INSTRUCTIONS

Tested locally with some broken dashboards. Screenshots attached.

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #21724 (d41cb66) into master (d41cb66) will not change coverage.
The diff coverage is n/a.

❗ 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.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bkyryliuk bkyryliuk self-requested a review October 7, 2022 00:09
@mayurnewase
Copy link
Contributor

mayurnewase commented Oct 8, 2022

good stuff, have 2 questions.

  1. in case of error modal, should we send the alert to owner and not to the receivers?
  2. I think we should add 1 test case when there is error modal.

@zhaorui2022
Copy link
Contributor Author

zhaorui2022 commented Oct 11, 2022

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.

@dpgaspar dpgaspar self-requested a review October 13, 2022 19:01
Copy link
Member

@dpgaspar dpgaspar left a 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
Copy link
Member

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
Copy link
Member

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

@rusackas
Copy link
Member

rusackas commented Dec 2, 2022

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!

@zhaorui2022
Copy link
Contributor Author

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

Copy link
Member

@rusackas rusackas left a 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!

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants