Skip to content

use new jasmine pattern for catching errors in async code - #5372

Merged
archmoj merged 2 commits into
masterfrom
drop-failTest
Jan 5, 2021
Merged

use new jasmine pattern for catching errors in async code#5372
archmoj merged 2 commits into
masterfrom
drop-failTest

Conversation

@archmoj

@archmoj archmoj commented Jan 5, 2021

Copy link
Copy Markdown
Contributor

Addressing #5340 (comment)
The first pass to retire failTest.

@plotly/plotly_js

@alexcjohnson

alexcjohnson commented Jan 5, 2021

Copy link
Copy Markdown
Collaborator

Have you verified that done.fail gives similar feedback to what we got from failTest when an exception is thrown? And is there a reason that pattern is preferable to .then(done, done) that the docs imply should do the same thing in Jasmine 3?

@archmoj
Merge remote-tracking branch 'origin/master' into drop-failTest
13ef2bc 
to resolve conflicts in
test/jasmine/tests/bar_test.js
test/jasmine/tests/contourgl_test.js
test/jasmine/tests/funnel_test.js
test/jasmine/tests/pointcloud_test.js
test/jasmine/tests/sankey_test.js
test/jasmine/tests/shapes_test.js
test/jasmine/tests/waterfall_test.js
@archmoj

archmoj commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Have you verified that done.fail gives similar feedback to what we got from failTest when an exception is thrown? And is there a reason that pattern is preferable to .then(done, done) that the docs imply should do the same thing in Jasmine 3?

RE1:
I verified and it prints out the error in case of exception.

RE2:
It looks like we could use the .then(don, done) instead but then this case would be confusing:

plotly.js/test/jasmine/tests/animate_test.js

Line 183 in 444be40

Plotly.animate(gd, ['foobar'], animOpts).then(failTest).then(done, done);

So it seems preferable to keep done.fail as is.

@alexcjohnson

alexcjohnson commented Jan 5, 2021

Copy link
Copy Markdown
Collaborator

True, done.fail is more explicit. Also possibly .then(done, done) would look like a pass if we (or any dep) ever throw a non-error, ie throw 'whoops!' rather than throw new Error('whoops!')

So great, let's keep done.fail.

That Plotly.animate test is certainly confusing in any case, and it makes me wonder whether we're really rejecting the way we want to. Looks like it's this one:

plotly.js/src/plot_api/plot_api.js

Lines 3471 to 3474 in 444be40

if(frame.type === 'byname' && !trans._frameHash[frame.data.name]) {
Lib.warn('animate failure: frame not found: "' + frame.data.name + '"');
reject();
return;

Most people seem to recommend rejecting with an Error which seems like it would be more useful, but if we did that we'd need to change the test to something like:

Plotly.animate(gd, ['foobar'], animOpts).then(done.fail, () => { done() });

Anyway not worth spending time on now, Plotly.animate is no longer something we're recommending people use anymore. But if we have a future need to reject a promise I think we should do so with an Error.

@alexcjohnson

alexcjohnson commented Jan 5, 2021

Copy link
Copy Markdown
Collaborator

After this PR are there any uses of failTest left, or can we delete it entirely?

@archmoj

archmoj commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

After this PR are there any uses of failTest left, or can we delete it entirely?

There are just a few cases. But let's attempt drop failTest in another PR.

alexcjohnson
alexcjohnson approved these changes Jan 5, 2021
View reviewed changes

@alexcjohnson alexcjohnson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

@archmoj
archmoj merged commit 45f4513 into master Jan 5, 2021
@archmoj
archmoj deleted the drop-failTest branch January 5, 2021 16:29
@archmoj archmoj added this to the 1.59.0 milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@archmoj @alexcjohnson