From bb351c87053deb31d76d1cd3e01057477c6a442e Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 8 Feb 2021 17:28:31 -0500 Subject: [PATCH 1/7] refactor loop over describe in transition test --- test/jasmine/tests/transition_test.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/test/jasmine/tests/transition_test.js b/test/jasmine/tests/transition_test.js index e09f3578bb2..6dce2ff6396 100644 --- a/test/jasmine/tests/transition_test.js +++ b/test/jasmine/tests/transition_test.js @@ -12,7 +12,13 @@ var destroyGraphDiv = require('../assets/destroy_graph_div'); var delay = require('../assets/delay'); var mock = require('@mocks/animation'); -function runTests(transitionDuration) { +[0, 20].forEach(function(transitionDuration) { + // Run the whole set of tests twice: once with zero duration and once with + // nonzero duration since the behavior should be identical, but there's a + // very real possibility of race conditions or other timing issues. + // + // And of course, remember to put the async loop in a closure: + describe('Plots.transition (duration = ' + transitionDuration + ')', function() { 'use strict'; @@ -258,17 +264,8 @@ function runTests(transitionDuration) { .then(done, done.fail); }); }); -} +}); -for(var i = 0; i < 2; i++) { - var duration = i * 20; - // Run the whole set of tests twice: once with zero duration and once with - // nonzero duration since the behavior should be identical, but there's a - // very real possibility of race conditions or other timing issues. - // - // And of course, remember to put the async loop in a closure: - runTests(duration); -} describe('Plotly.react transitions:', function() { var gd; From bc903cc702b12fb14a7ca8fd10d3f8dd05c3f290 Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 8 Feb 2021 17:36:38 -0500 Subject: [PATCH 2/7] make one descibe block and use transition duration in test descriptions --- test/jasmine/tests/transition_test.js | 53 +++++++++++++-------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/test/jasmine/tests/transition_test.js b/test/jasmine/tests/transition_test.js index 6dce2ff6396..c6f7a00c4bb 100644 --- a/test/jasmine/tests/transition_test.js +++ b/test/jasmine/tests/transition_test.js @@ -12,32 +12,31 @@ var destroyGraphDiv = require('../assets/destroy_graph_div'); var delay = require('../assets/delay'); var mock = require('@mocks/animation'); -[0, 20].forEach(function(transitionDuration) { - // Run the whole set of tests twice: once with zero duration and once with - // nonzero duration since the behavior should be identical, but there's a - // very real possibility of race conditions or other timing issues. - // - // And of course, remember to put the async loop in a closure: +describe('Plots.transition', function() { + 'use strict'; - describe('Plots.transition (duration = ' + transitionDuration + ')', function() { - 'use strict'; - - var gd; + var gd; - beforeEach(function(done) { - gd = createGraphDiv(); + beforeEach(function(done) { + gd = createGraphDiv(); - var mockCopy = Lib.extendDeep({}, mock); + var mockCopy = Lib.extendDeep({}, mock); - Plotly.newPlot(gd, mockCopy.data, mockCopy.layout).then(done); - }); + Plotly.newPlot(gd, mockCopy.data, mockCopy.layout).then(done); + }); - afterEach(function() { - Plotly.purge(gd); - destroyGraphDiv(); - }); + afterEach(function() { + Plotly.purge(gd); + destroyGraphDiv(); + }); - it('resolves only once the transition has completed', function(done) { + // Run the whole set of tests twice: once with zero duration and once with + // nonzero duration since the behavior should be identical, but there's a + // very real possibility of race conditions or other timing issues. + // + // And of course, remember to put the async loop in a closure: + [0, 20].forEach(function(transitionDuration) { + it('with duration:' + transitionDuration + ', resolves only once the transition has completed', function(done) { var t1 = Date.now(); var traces = plotApiHelpers.coerceTraceIndices(gd, null); @@ -48,7 +47,7 @@ var mock = require('@mocks/animation'); }).then(done, done.fail); }); - it('emits plotly_transitioning on transition start', function(done) { + it('with duration:' + transitionDuration + ', emits plotly_transitioning on transition start', function(done) { var beginTransitionCnt = 0; var traces = plotApiHelpers.coerceTraceIndices(gd, null); @@ -61,7 +60,7 @@ var mock = require('@mocks/animation'); }).then(done, done.fail); }); - it('emits plotly_transitioned on transition end', function(done) { + it('with duration:' + transitionDuration + ', emits plotly_transitioned on transition end', function(done) { var trEndCnt = 0; var traces = plotApiHelpers.coerceTraceIndices(gd, null); @@ -74,7 +73,7 @@ var mock = require('@mocks/animation'); }).then(done, done.fail); }); - it('transitions an annotation', function(done) { + it('with duration:' + transitionDuration + ', transitions an annotation', function(done) { function annotationPosition() { var g = gd._fullLayout._infolayer.select('.annotation').select('.annotation-text-g'); var bBox = g.node().getBoundingClientRect(); @@ -102,7 +101,7 @@ var mock = require('@mocks/animation'); }).then(done, done.fail); }); - it('transitions an image', function(done) { + it('with duration:' + transitionDuration + ', transitions an image', function(done) { var jsLogo = 'https://images.plot.ly/language-icons/api-home/js-logo.png'; var pythonLogo = 'https://images.plot.ly/language-icons/api-home/python-logo.png'; @@ -137,7 +136,7 @@ var mock = require('@mocks/animation'); }).then(done, done.fail); }); - it('transitions a shape', function(done) { + it('with duration:' + transitionDuration + ', transitions a shape', function(done) { function getPath() { return gd._fullLayout._shapeUpperLayer.select('path').node(); } @@ -195,7 +194,7 @@ var mock = require('@mocks/animation'); }); - it('transitions a transform', function(done) { + it('with duration:' + transitionDuration + ', transitions a transform', function(done) { Plotly.restyle(gd, { 'transforms[0]': { enabled: true, @@ -232,7 +231,7 @@ var mock = require('@mocks/animation'); // This doesn't really test anything that the above tests don't cover, but it combines // the behavior and attempts to ensure chaining and events happen in the correct order. - it('transitions may be chained', function(done) { + it('with duration:' + transitionDuration + ', transitions may be chained', function(done) { var currentlyRunning = 0; var beginCnt = 0; var endCnt = 0; From 832ebc2e649fbe40ceb745b65eac21d817188baf Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 8 Feb 2021 17:44:19 -0500 Subject: [PATCH 3/7] add flaky tags --- test/jasmine/tests/animate_test.js | 4 ++-- test/jasmine/tests/cartesian_interact_test.js | 10 +++++----- test/jasmine/tests/select_test.js | 4 ++-- test/jasmine/tests/transition_test.js | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index 90d8dd20ad4..01ce4a61249 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -750,7 +750,7 @@ describe('Animating multiple axes', function() { .then(done, done.fail); }); - it('updates ranges of secondary axes (date + category case)', function(done) { + it('@flaky updates ranges of secondary axes (date + category case)', function(done) { Plotly.newPlot(gd, [ {x: ['2018-01-01', '2019-01-01', '2020-01-01'], y: [1, 2, 3]}, {x: ['a', 'b', 'c'], y: [1, 2, 3], xaxis: 'x2', yaxis: 'y2'} @@ -950,7 +950,7 @@ describe('animating scatter traces', function() { }).then(done, done.fail); }); - it('should animate axis ranges using the less number of steps', function(done) { + it('@flaky should animate axis ranges using the less number of steps', function(done) { // sanity-check that scatter points and bars are still there function _assertNodeCnt() { var gd3 = d3Select(gd); diff --git a/test/jasmine/tests/cartesian_interact_test.js b/test/jasmine/tests/cartesian_interact_test.js index cfa829c18e0..8ca5c3a5e7d 100644 --- a/test/jasmine/tests/cartesian_interact_test.js +++ b/test/jasmine/tests/cartesian_interact_test.js @@ -960,7 +960,7 @@ describe('axis zoom/pan and main plot zoom', function() { var yr0 = [-0.211, 3.211]; var specs = [{ - desc: '@flaky zoombox on xy', + desc: 'zoombox on xy', drag: ['xy', 'nsew', 30, 30], exp: [ [['xaxis', 'xaxis2', 'xaxis3'], [1.457, 2.328]], @@ -1094,7 +1094,7 @@ describe('axis zoom/pan and main plot zoom', function() { var msg = 'after ' + s.desc; var msg2 = ['after dblclick on subplot', s.dblclickSubplot, msg].join(' '); - it(s.desc, function(done) { + it('@flaky ' + s.desc, function(done) { makePlot(data, layout, s).then(function() { assertRanges('base', [ [['xaxis', 'xaxis2', 'xaxis3'], xr0], @@ -1194,7 +1194,7 @@ describe('axis zoom/pan and main plot zoom', function() { specs.forEach(function(s) { var msg = 'after ' + s.desc; - it(s.desc, function(done) { + it('@flaky ' + s.desc, function(done) { makePlot(data, layout, s).then(function() { assertRanges('base', [ [['xaxis'], xr0], @@ -1331,7 +1331,7 @@ describe('axis zoom/pan and main plot zoom', function() { specs.forEach(function(s) { var msg = 'after ' + s.desc; - it(s.desc, function(done) { + it('@flaky ' + s.desc, function(done) { makePlot(data, layout, s).then(function() { assertRanges('base', [ [['yaxis', 'xaxis2'], rm0], @@ -1412,7 +1412,7 @@ describe('axis zoom/pan and main plot zoom', function() { var msg2 = ['after dblclick on subplot', s.dblclickSubplot, msg].join(' '); var spmatch = s.dblclickSubplot.match(constants.SUBPLOT_PATTERN); - it(s.desc, function(done) { + it('@flaky ' + s.desc, function(done) { makePlot(data, layout, s).then(function() { assertRanges('base', [ [['xaxis', 'yaxis'], rng0.xy], diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js index 0e8eb31b29c..ee101c4ed5d 100644 --- a/test/jasmine/tests/select_test.js +++ b/test/jasmine/tests/select_test.js @@ -1420,7 +1420,7 @@ describe('Test select box and lasso in general:', function() { .then(done, done.fail); }); - it('should cleanly clear and restart selections on double click when add/subtract mode on', function(done) { + it('@flaky should cleanly clear and restart selections on double click when add/subtract mode on', function(done) { var gd = createGraphDiv(); var fig = Lib.extendDeep({}, require('@mocks/0.json')); @@ -2456,7 +2456,7 @@ describe('Test select box and lasso per trace:', function() { }); [false].forEach(function(hasCssTransform) { - it('should work for bar traces, hasCssTransform: ' + hasCssTransform, function(done) { + it('@flaky should work for bar traces, hasCssTransform: ' + hasCssTransform, function(done) { var assertPoints = makeAssertPoints(['curveNumber', 'x', 'y']); var assertSelectedPoints = makeAssertSelectedPoints(); var assertRanges = makeAssertRanges(); diff --git a/test/jasmine/tests/transition_test.js b/test/jasmine/tests/transition_test.js index c6f7a00c4bb..fb7f823a753 100644 --- a/test/jasmine/tests/transition_test.js +++ b/test/jasmine/tests/transition_test.js @@ -840,7 +840,7 @@ describe('Plotly.react transitions:', function() { .then(done, done.fail); }); - it('should not transition layout when axis auto-ranged value do not changed', function(done) { + it('@flaky should not transition layout when axis auto-ranged value do not changed', function(done) { var data = [{y: [1, 2, 1]}]; var layout = {transition: {duration: 10}}; From 27c0ac49a0d3f97dde9a256bc8ce76e0f1e02619 Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 8 Feb 2021 21:44:31 -0500 Subject: [PATCH 4/7] early return with void _subplots e.g. afterAll in transition test --- src/plots/cartesian/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plots/cartesian/index.js b/src/plots/cartesian/index.js index 04b38ed6735..2c83676f409 100644 --- a/src/plots/cartesian/index.js +++ b/src/plots/cartesian/index.js @@ -126,6 +126,7 @@ exports.finalizeSubplots = function(layoutIn, layoutOut) { */ exports.plot = function(gd, traces, transitionOpts, makeOnCompleteCallback) { var fullLayout = gd._fullLayout; + if(!fullLayout._subplots) return; var subplots = fullLayout._subplots.cartesian; var calcdata = gd.calcdata; var i; From 3c6ec3a9b58373a8baf6de3b22d2468c16a97dbe Mon Sep 17 00:00:00 2001 From: archmoj Date: Mon, 8 Feb 2021 21:58:18 -0500 Subject: [PATCH 5/7] add missing gl tag to splom select test --- test/jasmine/tests/splom_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/splom_test.js b/test/jasmine/tests/splom_test.js index 6845848349d..7ec1f442c99 100644 --- a/test/jasmine/tests/splom_test.js +++ b/test/jasmine/tests/splom_test.js @@ -1825,7 +1825,7 @@ describe('Test splom select:', function() { .then(done, done.fail); }); - it('should be able to select and then clear using API', function(done) { + it('@gl should be able to select and then clear using API', function(done) { function _assert(msg, exp) { return function() { var uid = gd._fullData[0].uid; From a5f20876ad5c938d3fda2820b871e44e571d86d7 Mon Sep 17 00:00:00 2001 From: archmoj Date: Tue, 9 Feb 2021 13:52:27 -0500 Subject: [PATCH 6/7] add early return to functions called by setTimeout when gd._fullLayout is undefined --- src/plots/cartesian/index.js | 1 - src/plots/plots.js | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/plots/cartesian/index.js b/src/plots/cartesian/index.js index 2c83676f409..04b38ed6735 100644 --- a/src/plots/cartesian/index.js +++ b/src/plots/cartesian/index.js @@ -126,7 +126,6 @@ exports.finalizeSubplots = function(layoutIn, layoutOut) { */ exports.plot = function(gd, traces, transitionOpts, makeOnCompleteCallback) { var fullLayout = gd._fullLayout; - if(!fullLayout._subplots) return; var subplots = fullLayout._subplots.cartesian; var calcdata = gd.calcdata; var i; diff --git a/src/plots/plots.js b/src/plots/plots.js index 087f12adf7b..58138376abc 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2720,6 +2720,7 @@ plots.transitionFromReact = function(gd, restyleFlags, relayoutFlags, oldFullLay } function transitionAxes() { + if(!gd._fullLayout) return; for(var j = 0; j < basePlotModules.length; j++) { if(basePlotModules[j].transitionAxes) { basePlotModules[j].transitionAxes(gd, axEdits, axisTransitionOpts, makeCallback); @@ -2728,6 +2729,7 @@ plots.transitionFromReact = function(gd, restyleFlags, relayoutFlags, oldFullLay } function transitionTraces() { + if(!gd._fullLayout) return; for(var j = 0; j < basePlotModules.length; j++) { basePlotModules[j].plot(gd, transitionedTraces, traceTransitionOpts, makeCallback); } From 85ad8744aeee58b7168669f7ecc79d81b2d6eed0 Mon Sep 17 00:00:00 2001 From: archmoj Date: Tue, 9 Feb 2021 14:09:21 -0500 Subject: [PATCH 7/7] add three more early returns for destroyed plot --- src/components/legend/draw.js | 1 + src/plots/cartesian/dragbox.js | 1 + src/plots/plots.js | 1 + 3 files changed, 3 insertions(+) diff --git a/src/components/legend/draw.js b/src/components/legend/draw.js index 0d4d9472f5b..f91b813c6c4 100644 --- a/src/components/legend/draw.js +++ b/src/components/legend/draw.js @@ -369,6 +369,7 @@ function clickOrDoubleClick(gd, legend, legendItem, numClicks, evt) { if(numClicks === 1) { legend._clickTimeout = setTimeout(function() { + if(!gd._fullLayout) return; handleClick(legendItem, gd, numClicks); }, gd._context.doubleClickDelay); } else if(numClicks === 2) { diff --git a/src/plots/cartesian/dragbox.js b/src/plots/cartesian/dragbox.js index 8ca85e25ed6..e4632e10b31 100644 --- a/src/plots/cartesian/dragbox.js +++ b/src/plots/cartesian/dragbox.js @@ -535,6 +535,7 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) { // then replot after a delay to make sure // no more scrolling is coming redrawTimer = setTimeout(function() { + if(!gd._fullLayout) return; scrollViewBox = [0, 0, pw, ph]; dragTail(); }, REDRAWDELAY); diff --git a/src/plots/plots.js b/src/plots/plots.js index 58138376abc..4e4ae1574b7 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -51,6 +51,7 @@ plots.redrawText = function(gd) { return new Promise(function(resolve) { setTimeout(function() { + if(!gd._fullLayout) return; Registry.getComponentMethod('annotations', 'draw')(gd); Registry.getComponentMethod('legend', 'draw')(gd); Registry.getComponentMethod('colorbar', 'draw')(gd);