Skip to content

Transform removal from react fix #2805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 11, 2018
Merged

Transform removal from react fix #2805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
016f6f9
use .catch(failTest).then(done) pattern
etpinard Jul 11, 2018
acc98b2
clear _indexToPoints in case it got relinked
etpinard Jul 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2514,6 +2514,8 @@ plots.doCalcdata = function(gd, traces) {

if(trace.visible === true) {

// clear existing ref in case it got relinked
delete trace._indexToPoints;
// keep ref of index-to-points map object of the *last* enabled transform,
// this index-to-points map object is required to determine the calcdata indices
// that correspond to input indices (e.g. from 'selectedpoints')
Expand Down
23 changes: 23 additions & 0 deletions test/jasmine/tests/transform_filter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,4 +1274,27 @@ describe('filter transforms interactions', function() {
.then(done);
});

it('should clear indexToPoints on removal', function(done) {
var gd = createGraphDiv();

Plotly.react(gd, [{
y: [1, 2, 3, 1, 2, 3],
transforms: [{
type: 'filter',
target: 'y',
operation: '<',
value: 3
}]
}])
.then(function() {
expect(gd._fullData[0]._indexToPoints).toEqual({0: [0], 1: [1], 2: [3], 3: [4]});
return Plotly.react(gd, [{ y: [1, 2, 3, 1, 2, 3] }]);
})
.then(function() {
expect(gd._fullData[0]._indexToPoints).toBeUndefined();
})
.catch(failTest)
.then(done);
});

});
141 changes: 75 additions & 66 deletions test/jasmine/tests/transform_multi_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var Lib = require('@src/lib');

var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var failTest = require('../assets/fail_test');
var customAssertions = require('../assets/custom_assertions');
var supplyAllDefaults = require('../assets/supply_defaults');

Expand Down Expand Up @@ -434,9 +435,9 @@ describe('multiple transforms:', function() {
expect(gd._fullData[0].transforms[0]._indexToPoints).toEqual({0: [1], 1: [3], 2: [4]});
expect(gd._fullData[0].transforms[1]._indexToPoints).toEqual({0: [1, 3], 1: [4]});
expect(gd._fullData[0].transforms[2]._indexToPoints).toEqual({0: [4]});

done();
});
})
.catch(failTest)
.then(done);
});


Expand All @@ -455,9 +456,9 @@ describe('multiple transforms:', function() {
expect(gd._fullData[1].y).toEqual([2, 3]);

assertDims([2, 2]);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.plot should plot the transform traces (reverse case)', function(done) {
Expand All @@ -477,9 +478,9 @@ describe('multiple transforms:', function() {
expect(gd._fullData[1].y).toEqual([2, 3]);

assertDims([2, 2]);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.restyle should work', function(done) {
Expand Down Expand Up @@ -529,9 +530,9 @@ describe('multiple transforms:', function() {
['rgb(0, 128, 0)', 'rgb(255, 0, 0)'],
[0.4, 0.4]
);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.extendTraces should work', function(done) {
Expand Down Expand Up @@ -573,9 +574,9 @@ describe('multiple transforms:', function() {
return Plotly.deleteTraces(gd, [0]);
}).then(function() {
assertDims([]);

done();
});
})
.catch(failTest)
.then(done);
});

it('toggling trace visibility should work', function(done) {
Expand All @@ -595,12 +596,12 @@ describe('multiple transforms:', function() {
return Plotly.restyle(gd, 'visible', [true, true]);
}).then(function() {
assertDims([2, 2, 2, 2]);

done();
});
})
.catch(failTest)
.then(done);
});

it('executes filter and aggregate in the order given', function() {
it('executes filter and aggregate in the order given', function(done) {
// filter and aggregate do not commute!

var trace1 = {
Expand All @@ -624,18 +625,21 @@ describe('multiple transforms:', function() {
var trace2 = Lib.extendDeep({}, trace1);
trace2.transforms.reverse();

Plotly.newPlot(gd, [trace1, trace2]);
Plotly.newPlot(gd, [trace1, trace2]).then(function() {
var trace1Out = gd._fullData[0];
expect(trace1Out.x).toEqual([2]);
expect(trace1Out.y).toEqual([5]);

var trace1Out = gd._fullData[0];
expect(trace1Out.x).toEqual([2]);
expect(trace1Out.y).toEqual([5]);
var trace2Out = gd._fullData[1];
expect(trace2Out.x).toEqual([4, -5]);
expect(trace2Out.y).toEqual([5, 4]);

var trace2Out = gd._fullData[1];
expect(trace2Out.x).toEqual([4, -5]);
expect(trace2Out.y).toEqual([5, 4]);
})
.catch(failTest)
.then(done);
});

it('always executes groupby before aggregate', function() {
it('always executes groupby before aggregate', function(done) {
// aggregate and groupby wouldn't commute, but groupby always happens first
// because it has a `transform`, and aggregate has a `calcTransform`

Expand All @@ -658,29 +662,31 @@ describe('multiple transforms:', function() {
var trace2 = Lib.extendDeep({}, trace1);
trace2.transforms.reverse();

Plotly.newPlot(gd, [trace1, trace2]);

var t1g1 = gd._fullData[0];
var t1g2 = gd._fullData[1];
var t2g1 = gd._fullData[2];
var t2g2 = gd._fullData[3];

expect(t1g1.x).toEqual([1, 2]);
expect(t1g1.y).toEqual([2, 4]);
// group 2 has its aggregations switched, since group 2 comes first
expect(t1g2.x).toEqual([3, 9]);
expect(t1g2.y).toEqual([6, 9]);

// if we had done aggregation first, we'd implicitly get the first val
// for each of the groupby groups, which is [1, 1]
// so we'd only make 1 output trace, and it would look like:
// {x: [10, 5], y: [20/3, 5]}
// (and if we got some other groupby groups values, the most it could do
// is break ^^ into two separate traces)
expect(t2g1.x).toEqual(t1g1.x);
expect(t2g1.y).toEqual(t1g1.y);
expect(t2g2.x).toEqual(t1g2.x);
expect(t2g2.y).toEqual(t1g2.y);
Plotly.newPlot(gd, [trace1, trace2]).then(function() {
var t1g1 = gd._fullData[0];
var t1g2 = gd._fullData[1];
var t2g1 = gd._fullData[2];
var t2g2 = gd._fullData[3];

expect(t1g1.x).toEqual([1, 2]);
expect(t1g1.y).toEqual([2, 4]);
// group 2 has its aggregations switched, since group 2 comes first
expect(t1g2.x).toEqual([3, 9]);
expect(t1g2.y).toEqual([6, 9]);

// if we had done aggregation first, we'd implicitly get the first val
// for each of the groupby groups, which is [1, 1]
// so we'd only make 1 output trace, and it would look like:
// {x: [10, 5], y: [20/3, 5]}
// (and if we got some other groupby groups values, the most it could do
// is break ^^ into two separate traces)
expect(t2g1.x).toEqual(t1g1.x);
expect(t2g1.y).toEqual(t1g1.y);
expect(t2g2.x).toEqual(t1g2.x);
expect(t2g2.y).toEqual(t1g2.y);
})
.catch(failTest)
.then(done);
});
});

Expand All @@ -699,8 +705,9 @@ describe('invalid transforms', function() {
transforms: [{}]
}]).then(function() {
expect(gd._fullData[0].transforms.length).toEqual(1);
done();
});
})
.catch(failTest)
.then(done);
});
});

Expand Down Expand Up @@ -761,9 +768,9 @@ describe('multiple traces with transforms:', function() {
expect(gd._fullData[2].y).toEqual([3, 5, 2]);

assertDims([2, 3, 3]);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.restyle should work', function(done) {
Expand Down Expand Up @@ -816,9 +823,9 @@ describe('multiple traces with transforms:', function() {
['rgb(0, 128, 0)', 'rgb(0, 128, 0)', 'rgb(255, 0, 0)'],
[0.4, 0.6, 0.6]
);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.extendTraces should work', function(done) {
Expand All @@ -843,9 +850,9 @@ describe('multiple traces with transforms:', function() {
}, [0]);
}).then(function() {
assertDims([5, 4, 4]);

done();
});
})
.catch(failTest)
.then(done);
});

it('Plotly.deleteTraces should work', function(done) {
Expand All @@ -863,9 +870,9 @@ describe('multiple traces with transforms:', function() {
return Plotly.deleteTraces(gd, [0]);
}).then(function() {
assertDims([]);

done();
});
})
.catch(failTest)
.then(done);
});

it('toggling trace visibility should work', function(done) {
Expand All @@ -891,9 +898,10 @@ describe('multiple traces with transforms:', function() {
return Plotly.restyle(gd, 'visible', 'legendonly', [0]);
}).then(function() {
assertDims([3, 3]);
})
.catch(failTest)
.then(done);

done();
});
});
});

Expand Down Expand Up @@ -959,6 +967,7 @@ describe('restyle applied on transforms:', function() {
expect(gd.data[0].transforms).toBeUndefined(msg);
expect(gd._fullData[0].y).toEqual([2, 1, 2], msg);
})
.catch(failTest)
.then(done);
});

Expand Down