You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/08/04 19:24:04 UTC

[GitHub] [incubator-superset] JasonD28 opened a new pull request #10517: feat: make screenshot timeout configurable

JasonD28 opened a new pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Selenium used to timeout after 10 seconds of trying to locate an element in DOM and after 60 seconds of waiting for the element to load after located. This PR makes those two values configurable in the config.py
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#issuecomment-668825308


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=h1) Report
   > Merging [#10517](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9c5b0e1c8608f4339474051241aa361050878339&el=desc) will **increase** coverage by `0.05%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10517/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10517      +/-   ##
   ==========================================
   + Coverage   70.87%   70.93%   +0.05%     
   ==========================================
     Files         605      605              
     Lines       32433    32431       -2     
     Branches     3423     3429       +6     
   ==========================================
   + Hits        22988    23006      +18     
   + Misses       9333     9312      -21     
   - Partials      112      113       +1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `54.82% <ø> (+0.02%)` | :arrow_up: |
   | #javascript | `59.93% <ø> (-0.02%)` | :arrow_down: |
   | #python | `70.42% <50.00%> (+0.08%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/screenshots.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvc2NyZWVuc2hvdHMucHk=) | `29.41% <0.00%> (ø)` | |
   | [superset/config.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.23% <100.00%> (+0.07%)` | :arrow_up: |
   | [superset/tasks/schedules.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdGFza3Mvc2NoZWR1bGVzLnB5) | `74.00% <0.00%> (-0.77%)` | :arrow_down: |
   | [superset/errors.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXJyb3JzLnB5) | `93.54% <0.00%> (-0.21%)` | :arrow_down: |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `99.30% <0.00%> (-0.01%)` | :arrow_down: |
   | [superset/viz\_sip38.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6X3NpcDM4LnB5) | `0.00% <0.00%> (ø)` | |
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `62.60% <0.00%> (ø)` | |
   | [...erset-frontend/src/datasource/DatasourceEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFzb3VyY2UvRGF0YXNvdXJjZUVkaXRvci5qc3g=) | `71.00% <0.00%> (ø)` | |
   | [superset-frontend/src/components/EditableTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRWRpdGFibGVUaXRsZS5qc3g=) | | |
   | [superset-frontend/src/CRUD/CollectionTable.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL0NSVUQvQ29sbGVjdGlvblRhYmxlLmpzeA==) | | |
   | ... and [9 more](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=footer). Last update [9c5b0e1...e521ea8](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk merged pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#discussion_r465281721



##########
File path: superset/utils/screenshots.py
##########
@@ -159,11 +161,11 @@ def get_screenshot(
         time.sleep(SELENIUM_HEADSTART)
         try:
             logger.debug("Wait for the presence of %s", element_name)
-            element = WebDriverWait(driver, 10).until(
+            element = WebDriverWait(driver, self._element_locate_wait).until(

Review comment:
       since it is an only place where value is accessed - you can use it directly here




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#discussion_r465307980



##########
File path: superset/config.py
##########
@@ -799,6 +799,12 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # and render for the email report.
 EMAIL_PAGE_RENDER_WAIT = 30
 
+# Time in seconds before selenium times out after trying to
+# locate an element on the page and wait for that element to
+# load for an alert screenshot.

Review comment:
       I find it useful to keep those separate while iterating on the solution. E.g. we have email reports deployed in production but alerts is still work in progress. 
   +1 to add the comment that those are the nobs for the thumbnail endpoint / alerting [ not 100 % sure which one is used here] 




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#discussion_r465308501



##########
File path: superset/config.py
##########
@@ -799,6 +799,12 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # and render for the email report.
 EMAIL_PAGE_RENDER_WAIT = 30
 
+# Time in seconds before selenium times out after trying to
+# locate an element on the page and wait for that element to
+# load for an alert screenshot.

Review comment:
       That's fine. Let's bring them together at some point though!




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#issuecomment-668825308


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=h1) Report
   > Merging [#10517](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9c5b0e1c8608f4339474051241aa361050878339&el=desc) will **decrease** coverage by `0.05%`.
   > The diff coverage is `50.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10517/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10517      +/-   ##
   ==========================================
   - Coverage   70.87%   70.82%   -0.06%     
   ==========================================
     Files         605      605              
     Lines       32433    32431       -2     
     Branches     3423     3429       +6     
   ==========================================
   - Hits        22988    22968      -20     
   - Misses       9333     9347      +14     
   - Partials      112      116       +4     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `54.24% <ø> (-0.55%)` | :arrow_down: |
   | #javascript | `59.93% <ø> (-0.02%)` | :arrow_down: |
   | #python | `70.42% <50.00%> (+0.08%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/screenshots.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvc2NyZWVuc2hvdHMucHk=) | `29.41% <0.00%> (ø)` | |
   | [superset/config.py](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.23% <100.00%> (+0.07%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <0.00%> (-16.67%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `77.27% <0.00%> (-5.20%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `44.00% <0.00%> (-4.00%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `88.57% <0.00%> (-2.86%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `60.25% <0.00%> (-2.36%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `37.75% <0.00%> (-1.66%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `52.12% <0.00%> (-1.22%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | `55.91% <0.00%> (-1.08%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-superset/pull/10517/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=footer). Last update [9c5b0e1...e521ea8](https://codecov.io/gh/apache/incubator-superset/pull/10517?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] willbarrett commented on a change in pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#discussion_r465305547



##########
File path: superset/config.py
##########
@@ -799,6 +799,12 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # and render for the email report.
 EMAIL_PAGE_RENDER_WAIT = 30
 
+# Time in seconds before selenium times out after trying to
+# locate an element on the page and wait for that element to
+# load for an alert screenshot.

Review comment:
       Let's add a note in this comment block that this is used for alerting functionality - we may also want to apply the same configuration to report generation if it is not already so.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] bkyryliuk commented on a change in pull request #10517: feat: make screenshot timeout configurable

Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10517:
URL: https://github.com/apache/incubator-superset/pull/10517#discussion_r465281211



##########
File path: superset/utils/screenshots.py
##########
@@ -102,6 +102,8 @@ def __init__(
         self._window: WindowSize = window or (800, 600)
         config_auth_func = current_app.config.get("WEBDRIVER_AUTH_FUNC", auth_driver)
         self._auth_func = auth_func or config_auth_func
+        self._element_locate_wait = current_app.config.get("SCREENSHOT_LOCATE_WAIT", 10)

Review comment:
       add SCREENSHOT_LOCATE_WAIT to the config and set default there rather than in code.
   here use:
   ```
    current_app.config["SCREENSHOT_LOCATE_WAIT"]
   ```




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org