Return and handle promises in tests #5340
Conversation
- also ensure set futureData to null before adding events
|
Whew, there's a lot here! I don't mind us updating these tests to support the possibility that these operations may become async in the future, it is the usage we want to promote for our users. But I don't see it as particularly high priority, and for the vast majority of these tests if the operation did become async the test would simply fail and could be addressed at that time. The cases I'd be worried about are when we're testing that something didn't happen, like: Plotly.newPlot(gd, someFigureThatPreviouslyHadABug);
expect(Loggers.error).not.toHaveBeenCalled();In that case, if the bug were to return but the plot became async, the test would still pass because the error would be thrown after we checked. But as long as we're also testing that something did change as a result of the operation, the test will fail as soon as the operation becomes async and we'll be alerted to fix it then. |
archmoj
force-pushed
the
ensure-return-promise
branch
from
4a5cc96 to
f023347
Compare
archmoj
force-pushed
the
ensure-return-promise
branch
from
f023347 to
5769cde
Compare
| 'translate(0,' + -dataScroll + ')'); | ||
| expect(scrollBar.getAttribute('width')).toBeGreaterThan(0); | ||
| expect(scrollBar.getAttribute('height')).toBeGreaterThan(0); | ||
| done(); |
There was a problem hiding this comment.
I guess this wants the new Promise / resolve pattern you used a few other places. Also there's a second Plotly.relayout above that should get its own async step.
There was a problem hiding this comment.
Attempt addressing in 3a313e7.
There was a problem hiding this comment.
Hmm.. after that change the tests started failing. So I revert the changes to this test in 5a127f0 but we may come back to improve those in a separate PR.
| Plotly.plot(gd, reversedMockCopy).then(function() { | ||
| Plotly.react(gd, mockCopy) | ||
| .then(function() { | ||
| return Plotly.plot(gd, reversedMockCopy); |
There was a problem hiding this comment.
We should get rid of Plotly.plot in v2 - Etienne liked to use it in tests instead of newPlot when we know the div we're plotting into is new, as it saves some time purging previous items, esp. gl contexts IIRC. But if it's used like this on a div that already has a plot the effects are extremely confusing, and I don't think we've had a legitimate use of this behavior (appending its traces to the existing graph) for a long long time.
There was a problem hiding this comment.
do you think this is something we could just do or do we need to make more noise about it being deprecated somehow?
There was a problem hiding this comment.
Note: it looks like we have zero instances of Plotly.plot() in our current docs, so that suggests we could just drop it.
There was a problem hiding this comment.
Yep - Plotly.plot behaves exactly like Plotly.newPlot except when it does confusing things that are much better done with Plotly.react. Thanks for verifying that we don't use it in our docs!
| 'yaxis.title': 'Y', | ||
| 'polar.radialaxis.title': 'Radial' | ||
| }); | ||
| done(); |
There was a problem hiding this comment.
also may be better with the new Promise / resolve pattern.
This reverts commit bbe25ef.
In respect to #5336 (comment).
As a result of this PR we could now drop most of the
noCIflags and runmapboxandparcoordstests on the CI.The number of auto retry is also reduced from 5 to 1.
@alexcjohnson @antoinerg
cc: @plotly/plotly_js