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/09/16 16:20:55 UTC

[GitHub] [incubator-superset] eschutho opened a new pull request #10897: fix: front end CI tests and test runner

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


   ### SUMMARY
   Fixes a CI error 'every step must define a uses or run key' and the remaining jest tests that weren't running on CI.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   Manually check that tests are running: https://github.com/apache/incubator-superset/actions/runs/256530135
   
   ### 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] eschutho closed pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho closed pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897


   


----------------------------------------------------------------
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] nytai merged pull request #10897: fix: front end CI tests and test runner

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


   


----------------------------------------------------------------
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] eschutho closed pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho closed pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897


   


----------------------------------------------------------------
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] nytai edited a comment on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693149037


   @eschutho unfortunately cypress is a required check. It seems the sqllab tabs test is particularly flaky -- it fails quite often on master builds. You can close and reopen the PR to trigger a CI run. I've rerun the cypress check 3 times now... let's see if it passes. If it doesn't pass it may be worth [temporarily] disabling it. 


----------------------------------------------------------------
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 #10897: Fix front end CI tests and test runner

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=h1) Report
   > Merging [#10897](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4eeee4c2eb49a46d1c98d93f4c93bbffee5d8164?el=desc) will **increase** coverage by `6.16%`.
   > The diff coverage is `87.50%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10897/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10897      +/-   ##
   ==========================================
   + Coverage   59.59%   65.76%   +6.16%     
   ==========================================
     Files         780      813      +33     
     Lines       37203    38307    +1104     
     Branches     3339     3595     +256     
   ==========================================
   + Hits        22171    25191    +3020     
   + Misses      14845    13009    -1836     
   + Partials      187      107      -80     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `56.44% <75.00%> (+0.26%)` | :arrow_up: |
   | #javascript | `61.76% <87.50%> (?)` | |
   | #python | `61.44% <ø> (ø)` | |
   
   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/10897?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...plore/components/controls/FilterBoxItemControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJCb3hJdGVtQ29udHJvbC5qc3g=) | `74.07% <50.00%> (+61.11%)` | :arrow_up: |
   | [...nd/src/dashboard/util/getComponentWidthFromDrop.js](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldENvbXBvbmVudFdpZHRoRnJvbURyb3AuanM=) | `95.65% <100.00%> (+33.74%)` | :arrow_up: |
   | [.../src/explore/components/controls/BoundsControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Cb3VuZHNDb250cm9sLmpzeA==) | `93.10% <100.00%> (+13.79%)` | :arrow_up: |
   | [superset-frontend/src/setup/setupPlugins.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwUGx1Z2lucy50cw==) | `22.22% <0.00%> (-77.78%)` | :arrow_down: |
   | [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
   | [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `32.35% <0.00%> (-67.65%)` | :arrow_down: |
   | [...rset-frontend/src/setup/setupErrorMessagesExtra.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlc0V4dHJhLnRz) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupErrorMessages.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlcy50cw==) | `54.54% <0.00%> (-45.46%)` | :arrow_down: |
   | [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
   | ... and [248 more](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10897?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/10897?src=pr&el=footer). Last update [4eeee4c...2b1f551](https://codecov.io/gh/apache/incubator-superset/pull/10897?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] eschutho commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -37,39 +37,49 @@ export default function getComponentWidthFromDrop({
     return component.meta.width;
   }
 
-  const draggingWidth = getDetailedComponentWidth({
+  const {
+    width: draggingWidth,
+    minimumWidth: minDraggingWidth,
+  } = getDetailedComponentWidth({
     component,
     components,
   });
 
-  const destinationWidth = getDetailedComponentWidth({
+  const {
+    width: destinationWidth,
+    occupiedWidth: draggingOccupiedWidth,
+  } = getDetailedComponentWidth({
     id: destination.id,
     components,
   });
 
-  let destinationCapacity =
-    destinationWidth.width - destinationWidth.occupiedWidth;
+  let destinationCapacity = Number(destinationWidth - draggingOccupiedWidth);
 
   if (Number.isNaN(destinationCapacity)) {
-    const grandparentWidth = getDetailedComponentWidth({
+    const {
+      width: grandparentWidth,
+      occupiedWidth: grandparentOccupiedWidth,
+    } = getDetailedComponentWidth({
       id: findParentId({
         childId: destination.id,
         layout: components,
       }),
       components,
     });
 
-    destinationCapacity =
-      grandparentWidth.width - grandparentWidth.occupiedWidth;
+    destinationCapacity = Number(grandparentWidth - grandparentOccupiedWidth);
   }
 
-  if (Number.isNaN(destinationCapacity) || Number.isNaN(draggingWidth.width)) {
-    return draggingWidth.width;
+  if (
+    Number.isNaN(destinationCapacity) ||
+    Number.isNaN(Number(draggingWidth))
+  ) {
+    return draggingWidth;

Review comment:
       @ktmud does this look more readable to you? It keeps the original return value here, to play it safe.
   you do have to coerce the value twice this way b/c the value is defined in the code multiple times depending on the conditions.




----------------------------------------------------------------
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 #10897: Fix front end CI tests and test runner

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=h1) Report
   > Merging [#10897](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4eeee4c2eb49a46d1c98d93f4c93bbffee5d8164?el=desc) will **increase** coverage by `6.25%`.
   > The diff coverage is `87.50%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10897/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10897?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10897      +/-   ##
   ==========================================
   + Coverage   59.59%   65.85%   +6.25%     
   ==========================================
     Files         780      813      +33     
     Lines       37203    38307    +1104     
     Branches     3339     3595     +256     
   ==========================================
   + Hits        22171    25226    +3055     
   + Misses      14845    12977    -1868     
   + Partials      187      104      -83     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `56.89% <75.00%> (+0.71%)` | :arrow_up: |
   | #javascript | `61.76% <87.50%> (?)` | |
   | #python | `61.44% <ø> (ø)` | |
   
   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/10897?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...plore/components/controls/FilterBoxItemControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJCb3hJdGVtQ29udHJvbC5qc3g=) | `74.07% <50.00%> (+61.11%)` | :arrow_up: |
   | [...nd/src/dashboard/util/getComponentWidthFromDrop.js](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldENvbXBvbmVudFdpZHRoRnJvbURyb3AuanM=) | `95.65% <100.00%> (+33.74%)` | :arrow_up: |
   | [.../src/explore/components/controls/BoundsControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Cb3VuZHNDb250cm9sLmpzeA==) | `93.10% <100.00%> (+13.79%)` | :arrow_up: |
   | [superset-frontend/src/setup/setupPlugins.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwUGx1Z2lucy50cw==) | `22.22% <0.00%> (-77.78%)` | :arrow_down: |
   | [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
   | [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `32.35% <0.00%> (-67.65%)` | :arrow_down: |
   | [...rset-frontend/src/setup/setupErrorMessagesExtra.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlc0V4dHJhLnRz) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupErrorMessages.ts](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlcy50cw==) | `54.54% <0.00%> (-45.46%)` | :arrow_down: |
   | [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
   | ... and [250 more](https://codecov.io/gh/apache/incubator-superset/pull/10897/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10897?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/10897?src=pr&el=footer). Last update [4eeee4c...2b1f551](https://codecov.io/gh/apache/incubator-superset/pull/10897?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] ktmud commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -50,7 +50,7 @@ export default function getComponentWidthFromDrop({
   let destinationCapacity =
     destinationWidth.width - destinationWidth.occupiedWidth;
 
-  if (Number.isNaN(destinationCapacity)) {
+  if (Number.isNaN(Number(destinationCapacity))) {

Review comment:
       Can we move these `Number` coercing above to where the variables are defined?




----------------------------------------------------------------
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] eschutho commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693554309


   restarting checks.


----------------------------------------------------------------
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] eschutho commented on a change in pull request #10897: fix: front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -37,39 +37,49 @@ export default function getComponentWidthFromDrop({
     return component.meta.width;
   }
 
-  const draggingWidth = getDetailedComponentWidth({
+  const {
+    width: draggingWidth,
+    minimumWidth: minDraggingWidth,
+  } = getDetailedComponentWidth({
     component,
     components,
   });
 
-  const destinationWidth = getDetailedComponentWidth({
+  const {
+    width: destinationWidth,
+    occupiedWidth: draggingOccupiedWidth,
+  } = getDetailedComponentWidth({
     id: destination.id,
     components,
   });
 
-  let destinationCapacity =
-    destinationWidth.width - destinationWidth.occupiedWidth;
+  let destinationCapacity = Number(destinationWidth - draggingOccupiedWidth);
 
   if (Number.isNaN(destinationCapacity)) {
-    const grandparentWidth = getDetailedComponentWidth({
+    const {
+      width: grandparentWidth,
+      occupiedWidth: grandparentOccupiedWidth,
+    } = getDetailedComponentWidth({
       id: findParentId({
         childId: destination.id,
         layout: components,
       }),
       components,
     });
 
-    destinationCapacity =
-      grandparentWidth.width - grandparentWidth.occupiedWidth;
+    destinationCapacity = Number(grandparentWidth - grandparentOccupiedWidth);
   }
 
-  if (Number.isNaN(destinationCapacity) || Number.isNaN(draggingWidth.width)) {
-    return draggingWidth.width;
+  if (
+    Number.isNaN(destinationCapacity) ||
+    Number.isNaN(Number(draggingWidth))
+  ) {
+    return draggingWidth;

Review comment:
       yeah, agree.




----------------------------------------------------------------
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] eschutho commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693104859


   Thanks @ktmud. I think the test failing on Cypress may be a flaky test. I'm working on Cypress tests this week, and can fix it separately if someone feels this is ready to merge now. Otherwise, I'll take a look at the test tomorrow. 


----------------------------------------------------------------
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] nytai commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693601367


   @ktmud I think that button is only available to committers


----------------------------------------------------------------
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] eschutho closed pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho closed pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897


   


----------------------------------------------------------------
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] ktmud commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -50,7 +50,7 @@ export default function getComponentWidthFromDrop({
   let destinationCapacity =
     destinationWidth.width - destinationWidth.occupiedWidth;
 
-  if (Number.isNaN(destinationCapacity)) {
+  if (Number.isNaN(Number(destinationCapacity))) {

Review comment:
       I actually know nothing about this file, lol. Just took another look, for `destinationCapacity` at least, it's calculated from an arithmetic operation so it's definitely expected to be a number.




----------------------------------------------------------------
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] ktmud commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693600289


   You can also rerun failed tests on the "Checks" page:
   
   <img src="https://user-images.githubusercontent.com/335541/93380688-6b36a880-f814-11ea-9abd-748f4b93e094.png" width="300">
   


----------------------------------------------------------------
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] nytai commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693149037


   @eschutho unfortunately cypress is a required check. It seems the sqllab tabs test is particularly flaky -- it fails quite often on master builds. You can close and reopen the PR to trigger a CI run. I've rerun the cypress check 2 times now... let's see if it passes. If it doesn't pass it may be worth [temporarily] disabling it. 


----------------------------------------------------------------
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] eschutho commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -50,7 +50,7 @@ export default function getComponentWidthFromDrop({
   let destinationCapacity =
     destinationWidth.width - destinationWidth.occupiedWidth;
 
-  if (Number.isNaN(destinationCapacity)) {
+  if (Number.isNaN(Number(destinationCapacity))) {

Review comment:
       It looks to me that you still want to return the original value as defined if it's not a number. If I coerce them earlier, you're going to get NaN as a value. I'm reading through the code now to see if that will work, but you're probably more familiar with the code than I am. 




----------------------------------------------------------------
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] ktmud commented on a change in pull request #10897: fix: front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -37,39 +37,49 @@ export default function getComponentWidthFromDrop({
     return component.meta.width;
   }
 
-  const draggingWidth = getDetailedComponentWidth({
+  const {
+    width: draggingWidth,
+    minimumWidth: minDraggingWidth,
+  } = getDetailedComponentWidth({
     component,
     components,
   });
 
-  const destinationWidth = getDetailedComponentWidth({
+  const {
+    width: destinationWidth,
+    occupiedWidth: draggingOccupiedWidth,
+  } = getDetailedComponentWidth({
     id: destination.id,
     components,
   });
 
-  let destinationCapacity =
-    destinationWidth.width - destinationWidth.occupiedWidth;
+  let destinationCapacity = Number(destinationWidth - draggingOccupiedWidth);
 
   if (Number.isNaN(destinationCapacity)) {
-    const grandparentWidth = getDetailedComponentWidth({
+    const {
+      width: grandparentWidth,
+      occupiedWidth: grandparentOccupiedWidth,
+    } = getDetailedComponentWidth({
       id: findParentId({
         childId: destination.id,
         layout: components,
       }),
       components,
     });
 
-    destinationCapacity =
-      grandparentWidth.width - grandparentWidth.occupiedWidth;
+    destinationCapacity = Number(grandparentWidth - grandparentOccupiedWidth);
   }
 
-  if (Number.isNaN(destinationCapacity) || Number.isNaN(draggingWidth.width)) {
-    return draggingWidth.width;
+  if (
+    Number.isNaN(destinationCapacity) ||
+    Number.isNaN(Number(draggingWidth))
+  ) {
+    return draggingWidth;

Review comment:
       Looking good. I think the long-term solution would be to migrate this and all related files to TypeScript so we can be more confident in changes like this.




----------------------------------------------------------------
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] ktmud commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693630559


   > @ktmud I think that button is only available to committers
   
   😱 🤯 


----------------------------------------------------------------
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] eschutho commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -50,7 +50,7 @@ export default function getComponentWidthFromDrop({
   let destinationCapacity =
     destinationWidth.width - destinationWidth.occupiedWidth;
 
-  if (Number.isNaN(destinationCapacity)) {
+  if (Number.isNaN(Number(destinationCapacity))) {

Review comment:
       `destinationWidth.draggingWidth` is the only that still returns a value even though it may not be a number. 




----------------------------------------------------------------
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] eschutho commented on pull request #10897: fix: front end CI tests and test runner

Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #10897:
URL: https://github.com/apache/incubator-superset/pull/10897#issuecomment-693515624


   Thanks @nytai it looks like the checks did pass, but there have been some merges to master, so I rebased again to make sure that builds don't break on master. I'll keep checking on this. 


----------------------------------------------------------------
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] eschutho commented on a change in pull request #10897: Fix front end CI tests and test runner

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



##########
File path: superset-frontend/src/dashboard/util/getComponentWidthFromDrop.js
##########
@@ -37,39 +37,49 @@ export default function getComponentWidthFromDrop({
     return component.meta.width;
   }
 
-  const draggingWidth = getDetailedComponentWidth({
+  const {
+    width: draggingWidth,
+    minimumWidth: minDraggingWidth,
+  } = getDetailedComponentWidth({
     component,
     components,
   });
 
-  const destinationWidth = getDetailedComponentWidth({
+  const {
+    width: destinationWidth,
+    occupiedWidth: draggingOccupiedWidth,
+  } = getDetailedComponentWidth({
     id: destination.id,
     components,
   });
 
-  let destinationCapacity =
-    destinationWidth.width - destinationWidth.occupiedWidth;
+  let destinationCapacity = Number(destinationWidth - draggingOccupiedWidth);
 
   if (Number.isNaN(destinationCapacity)) {
-    const grandparentWidth = getDetailedComponentWidth({
+    const {
+      width: grandparentWidth,
+      occupiedWidth: grandparentOccupiedWidth,
+    } = getDetailedComponentWidth({
       id: findParentId({
         childId: destination.id,
         layout: components,
       }),
       components,
     });
 
-    destinationCapacity =
-      grandparentWidth.width - grandparentWidth.occupiedWidth;
+    destinationCapacity = Number(grandparentWidth - grandparentOccupiedWidth);
   }
 
-  if (Number.isNaN(destinationCapacity) || Number.isNaN(draggingWidth.width)) {
-    return draggingWidth.width;
+  if (
+    Number.isNaN(destinationCapacity) ||
+    Number.isNaN(Number(draggingWidth))
+  ) {
+    return draggingWidth;

Review comment:
       @ktmud does this look more readable to you? It keeps the original return value here, to play it safe.
   The value is coerced multiple times this way b/c it is defined multiple times depending on the condition.




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