fix: front end CI tests and test runner #10897
Conversation
| @@ -50,7 +50,7 @@ export default function getComponentWidthFromDrop({ | |||
| let destinationCapacity = | |||
| destinationWidth.width - destinationWidth.occupiedWidth; | |||
|
|
|||
| if (Number.isNaN(destinationCapacity)) { | |||
| if (Number.isNaN(Number(destinationCapacity))) { | |||
There was a problem hiding this comment.
Can we move these Number coercing above to where the variables are defined?
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
destinationWidth.draggingWidth is the only that still returns a value even though it may not be a number.
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
eschutho
force-pushed
the
elizabeth/fix-ci
branch
2 times, most recently
from
be90d03 to
c274611
Compare
| Number.isNaN(destinationCapacity) || | ||
| Number.isNaN(Number(draggingWidth)) | ||
| ) { | ||
| return draggingWidth; |
There was a problem hiding this 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.
There was a problem hiding this 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.
eschutho
changed the title
|
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. |
|
@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. |
eschutho
force-pushed
the
elizabeth/fix-ci
branch
from
4f9ec34 to
531c36c
Compare
|
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. |
|
restarting checks. |
|
You can also rerun failed tests on the "Checks" page:
|
|
@ktmud I think that button is only available to committers |
😱 🤯 |
mistercrunch
added
🏷️ bot
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
TEST PLAN
Manually check that tests are running: https://github.com/apache/incubator-superset/actions/runs/256530135
ADDITIONAL INFORMATION