From 195cf1912f0dfdcab070a2475201749b83979449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 16 Aug 2018 11:46:00 -0400 Subject: [PATCH 1/5] add ability to run just 'jasmine' or just 'image' noci tests --- tasks/noci_test.sh | 48 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/tasks/noci_test.sh b/tasks/noci_test.sh index 6d208a56278..651cd46eecf 100755 --- a/tasks/noci_test.sh +++ b/tasks/noci_test.sh @@ -1,12 +1,25 @@ #! /bin/bash +# +# Run tests that aren't ran on CI (yet) +# +# to run all no-ci tests +# $ (plotly.js) ./tasks/noci_test.sh +# +# to run jasmine no-ci tests +# $ (plotly.js) ./tasks/noci_test.sh jasmine + +# to run image no-ci tests +# $ (plotly.js) ./tasks/noci_test.sh image +# +# ----------------------------------------------- EXIT_STATE=0 root=$(dirname $0)/.. -# tests that aren't run on CI (yet) - # jasmine specs with @noCI tag -npm run test-jasmine -- --tags=noCI,noCIdep --nowatch || EXIT_STATE=$? +test_jasmine () { + npm run test-jasmine -- --tags=noCI,noCIdep --nowatch || EXIT_STATE=$? +} # mapbox image tests take too much resources on CI # @@ -15,12 +28,27 @@ npm run test-jasmine -- --tags=noCI,noCIdep --nowatch || EXIT_STATE=$? # 'old' image server # # cone traces don't render correctly in the imagetest container -$root/../orca/bin/orca.js graph \ - $root/test/image/mocks/mapbox_* \ - $root/test/image/mocks/gl3d_cone* \ - --plotly $root/build/plotly.js \ - --mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \ - --output-dir $root/test/image/baselines/ \ - --verbose +test_image () { + $root/../orca/bin/orca.js graph \ + $root/test/image/mocks/mapbox_* \ + $root/test/image/mocks/gl3d_cone* \ + --plotly $root/build/plotly.js \ + --mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \ + --output-dir $root/test/image/baselines/ \ + --verbose || EXIT_STATE=$? +} + +case $1 in + jasmine) + test_jasmine + ;; + image) + test_image + ;; + *) + test_jasmine + test_image + ;; +esac exit $EXIT_STATE From 89fae052fd1ca2c693d3e22dc2ed31daea06e1ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 16 Aug 2018 11:52:14 -0400 Subject: [PATCH 2/5] replace fail -> failTest --- test/jasmine/tests/snapshot_test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/jasmine/tests/snapshot_test.js b/test/jasmine/tests/snapshot_test.js index b48a0f2858e..da84c2fd0d2 100644 --- a/test/jasmine/tests/snapshot_test.js +++ b/test/jasmine/tests/snapshot_test.js @@ -3,7 +3,7 @@ var Plotly = require('@lib/index'); var d3 = require('d3'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); -var fail = require('../assets/fail_test'); +var failTest = require('../assets/fail_test'); var subplotMock = require('../../image/mocks/multiple_subplots.json'); var annotationMock = require('../../image/mocks/annotations.json'); @@ -297,7 +297,7 @@ describe('Plotly.Snapshot', function() { expect(legendPointElements.length).toEqual(1); expect(legendPointElements[0].style.fill.substr(0, 6)).toEqual('url(\"#'); }) - .catch(fail) + .catch(failTest) .then(done); }); @@ -317,7 +317,7 @@ describe('Plotly.Snapshot', function() { expect(el.getAttribute('height')).toBe('1000', 'height'); expect(el.getAttribute('viewBox')).toBe('0 0 300 400', 'viewbox'); }) - .catch(fail) + .catch(failTest) .then(done); }); }); From b32aac06ce2033224f76931e216f41a609f1b286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 16 Aug 2018 12:37:57 -0400 Subject: [PATCH 3/5] fix toSVG for contour legend items & colorbars --- src/snapshot/tosvg.js | 9 +- test/jasmine/tests/snapshot_test.js | 131 +++++++++++++++++++--------- 2 files changed, 96 insertions(+), 44 deletions(-) diff --git a/src/snapshot/tosvg.js b/src/snapshot/tosvg.js index 51583764730..ae320d71ac7 100644 --- a/src/snapshot/tosvg.js +++ b/src/snapshot/tosvg.js @@ -116,15 +116,20 @@ module.exports = function toSVG(gd, format, scale) { } }); - svg.selectAll('.point,.scatterpts').each(function() { + svg.selectAll('.point, .scatterpts, .legendfill>path, .legendlines>path, .cbfill').each(function() { var pt = d3.select(this); - var fill = this.style.fill; // similar to font family styles above, // we must remove " after the SVG DOM has been serialized + var fill = this.style.fill; if(fill && fill.indexOf('url(') !== -1) { pt.style('fill', fill.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); } + + var stroke = this.style.stroke; + if(stroke && stroke.indexOf('url(') !== -1) { + pt.style('stroke', stroke.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); + } }); if(format === 'pdf' || format === 'eps') { diff --git a/test/jasmine/tests/snapshot_test.js b/test/jasmine/tests/snapshot_test.js index da84c2fd0d2..90c875e535d 100644 --- a/test/jasmine/tests/snapshot_test.js +++ b/test/jasmine/tests/snapshot_test.js @@ -1,4 +1,5 @@ var Plotly = require('@lib/index'); +var Lib = require('@src/lib'); var d3 = require('d3'); var createGraphDiv = require('../assets/create_graph_div'); @@ -250,55 +251,101 @@ describe('Plotly.Snapshot', function() { }); }); - it('should handle quoted style properties', function(done) { - Plotly.plot(gd, [{ - y: [1, 2, 1], - marker: { - gradient: { - type: 'radial', - color: '#fff' - }, - color: ['red', 'blue', 'green'] - } - }], { - font: { family: 'Times New Roman' }, - showlegend: true - }) - .then(function() { - d3.selectAll('text').each(function() { - expect(this.style.fontFamily).toEqual('\"Times New Roman\"'); - }); + describe('should handle quoted style properties', function() { + function checkURL(actual, msg) { + // which is enough tot check that toSVG did its job right + expect((actual || '').substr(0, 6)).toBe('url(\"#', msg); + } - d3.selectAll('.point,.scatterpts').each(function() { - expect(this.style.fill.substr(0, 6)).toEqual('url(\"#'); - }); + it('- marker-gradient case', function(done) { + Plotly.plot(gd, [{ + y: [1, 2, 1], + marker: { + gradient: { + type: 'radial', + color: '#fff' + }, + color: ['red', 'blue', 'green'] + } + }], { + font: { family: 'Times New Roman' }, + showlegend: true + }) + .then(function() { + d3.selectAll('text').each(function() { + expect(this.style.fontFamily).toEqual('\"Times New Roman\"'); + }); + + d3.selectAll('.point,.scatterpts').each(function() { + checkURL(this.style.fill); + }); + + return Plotly.Snapshot.toSVG(gd); + }) + .then(function(svg) { + var svgDOM = parser.parseFromString(svg, 'image/svg+xml'); + var i; + + var textElements = svgDOM.getElementsByTagName('text'); + expect(textElements.length).toEqual(12); + + for(i = 0; i < textElements.length; i++) { + expect(textElements[i].style.fontFamily).toEqual('\"Times New Roman\"'); + } - return Plotly.Snapshot.toSVG(gd); - }) - .then(function(svg) { - var svgDOM = parser.parseFromString(svg, 'image/svg+xml'); - var i; + var pointElements = svgDOM.getElementsByClassName('point'); + expect(pointElements.length).toEqual(3); + + for(i = 0; i < pointElements.length; i++) { + checkURL(pointElements[i].style.fill); + } - var textElements = svgDOM.getElementsByTagName('text'); - expect(textElements.length).toEqual(12); + var legendPointElements = svgDOM.getElementsByClassName('scatterpts'); + expect(legendPointElements.length).toEqual(1); + checkURL(legendPointElements[0].style.fill); + }) + .catch(failTest) + .then(done); + }); - for(i = 0; i < textElements.length; i++) { - expect(textElements[i].style.fontFamily).toEqual('\"Times New Roman\"'); - } + it('- legend with contour items case', function(done) { + var fig = Lib.extendDeep({}, require('@mocks/contour_legend.json')); + var fillItemIndices = [0, 4, 5]; - var pointElements = svgDOM.getElementsByClassName('point'); - expect(pointElements.length).toEqual(3); + Plotly.plot(gd, fig) + .then(function() { return Plotly.Snapshot.toSVG(gd); }) + .then(function(svg) { + var svgDOM = parser.parseFromString(svg, 'image/svg+xml'); - for(i = 0; i < pointElements.length; i++) { - expect(pointElements[i].style.fill.substr(0, 6)).toEqual('url(\"#'); - } + var fillItems = svgDOM.getElementsByClassName('legendfill'); + for(var i = 0; i < fillItemIndices.length; i++) { + checkURL(fillItems[fillItemIndices[i]].firstChild.style.fill, 'fill gradient ' + i); + } - var legendPointElements = svgDOM.getElementsByClassName('scatterpts'); - expect(legendPointElements.length).toEqual(1); - expect(legendPointElements[0].style.fill.substr(0, 6)).toEqual('url(\"#'); - }) - .catch(failTest) - .then(done); + var lineItems = svgDOM.getElementsByClassName('legendlines'); + checkURL(lineItems[1].firstChild.style.stroke, 'stroke gradient'); + }) + .catch(failTest) + .then(done); + }); + + it('- colorbar case', function(done) { + var fig = Lib.extendDeep({}, require('@mocks/16.json')); + + Plotly.plot(gd, fig) + .then(function() { return Plotly.Snapshot.toSVG(gd); }) + .then(function(svg) { + var svgDOM = parser.parseFromString(svg, 'image/svg+xml'); + + var fillItems = svgDOM.getElementsByClassName('cbfill'); + expect(fillItems.length).toBe(1, '# of colorbars'); + for(var i = 0; i < fillItems.length; i++) { + checkURL(fillItems[i].style.fill, 'fill gradient ' + i); + } + }) + .catch(failTest) + .then(done); + }); }); it('should adapt *viewBox* attribute under *scale* option', function(done) { From 1c8017ef88c3a04d1bbc555b72a4a42ca0848847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 16 Aug 2018 13:14:24 -0400 Subject: [PATCH 4/5] set url to gradient definition as an 'attribute', not in 'style' - so that we don't have to fiddle with quotes inside of quotes in Snapshot.toSVG - clear corresponding 'style' setting to avoid potential conflicts - adapt toSVG tests --- src/components/drawing/index.js | 6 +++++- src/snapshot/tosvg.js | 16 ---------------- test/jasmine/tests/snapshot_test.js | 15 ++++++++------- 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 443f882d95c..88841fc20b2 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -334,7 +334,11 @@ drawing.gradient = function(sel, gd, gradientID, type, colorscale, prop) { }); }); - sel.style(prop, 'url(#' + fullID + ')') + // Set url to gradient definition as an 'attribute', so that + // we don't have to fiddle with quotes inside of quotes in Snapshot.toSVG. + // Clear corresponding 'style' setting to avoid potential conflicts. + sel.attr(prop, 'url(#' + fullID + ')') + .style(prop, null) .style(prop + '-opacity', null); }; diff --git a/src/snapshot/tosvg.js b/src/snapshot/tosvg.js index ae320d71ac7..55a7a8f1629 100644 --- a/src/snapshot/tosvg.js +++ b/src/snapshot/tosvg.js @@ -116,22 +116,6 @@ module.exports = function toSVG(gd, format, scale) { } }); - svg.selectAll('.point, .scatterpts, .legendfill>path, .legendlines>path, .cbfill').each(function() { - var pt = d3.select(this); - - // similar to font family styles above, - // we must remove " after the SVG DOM has been serialized - var fill = this.style.fill; - if(fill && fill.indexOf('url(') !== -1) { - pt.style('fill', fill.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); - } - - var stroke = this.style.stroke; - if(stroke && stroke.indexOf('url(') !== -1) { - pt.style('stroke', stroke.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); - } - }); - if(format === 'pdf' || format === 'eps') { // these formats make the extra line MathJax adds around symbols look super thick in some cases // it looks better if this is removed entirely. diff --git a/test/jasmine/tests/snapshot_test.js b/test/jasmine/tests/snapshot_test.js index 90c875e535d..21c8985c0c8 100644 --- a/test/jasmine/tests/snapshot_test.js +++ b/test/jasmine/tests/snapshot_test.js @@ -254,7 +254,7 @@ describe('Plotly.Snapshot', function() { describe('should handle quoted style properties', function() { function checkURL(actual, msg) { // which is enough tot check that toSVG did its job right - expect((actual || '').substr(0, 6)).toBe('url(\"#', msg); + expect((actual || '').substr(0, 5)).toBe('url(#', msg); } it('- marker-gradient case', function(done) { @@ -277,7 +277,7 @@ describe('Plotly.Snapshot', function() { }); d3.selectAll('.point,.scatterpts').each(function() { - checkURL(this.style.fill); + checkURL(d3.select(this).attr('fill')); }); return Plotly.Snapshot.toSVG(gd); @@ -297,12 +297,12 @@ describe('Plotly.Snapshot', function() { expect(pointElements.length).toEqual(3); for(i = 0; i < pointElements.length; i++) { - checkURL(pointElements[i].style.fill); + checkURL(pointElements[i].getAttribute('fill')); } var legendPointElements = svgDOM.getElementsByClassName('scatterpts'); expect(legendPointElements.length).toEqual(1); - checkURL(legendPointElements[0].style.fill); + checkURL(legendPointElements[0].getAttribute('fill')); }) .catch(failTest) .then(done); @@ -319,11 +319,12 @@ describe('Plotly.Snapshot', function() { var fillItems = svgDOM.getElementsByClassName('legendfill'); for(var i = 0; i < fillItemIndices.length; i++) { - checkURL(fillItems[fillItemIndices[i]].firstChild.style.fill, 'fill gradient ' + i); + var path = fillItems[fillItemIndices[i]].firstChild; + checkURL(path.getAttribute('fill'), 'fill gradient ' + i); } var lineItems = svgDOM.getElementsByClassName('legendlines'); - checkURL(lineItems[1].firstChild.style.stroke, 'stroke gradient'); + checkURL(lineItems[1].firstChild.getAttribute('stroke'), 'stroke gradient'); }) .catch(failTest) .then(done); @@ -340,7 +341,7 @@ describe('Plotly.Snapshot', function() { var fillItems = svgDOM.getElementsByClassName('cbfill'); expect(fillItems.length).toBe(1, '# of colorbars'); for(var i = 0; i < fillItems.length; i++) { - checkURL(fillItems[i].style.fill, 'fill gradient ' + i); + checkURL(fillItems[i].getAttribute('fill'), 'fill gradient ' + i); } }) .catch(failTest) From 0557bb1f5fc878e3e926a58c7803f395ef8a602b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Thu, 16 Aug 2018 14:30:43 -0400 Subject: [PATCH 5/5] Revert "set url to gradient definition as an 'attribute', not in 'style'" - see https://github.com/plotly/plotly.js/pull/2914#issuecomment-413636074 for more info. This reverts commit 1c8017ef88c3a04d1bbc555b72a4a42ca0848847. --- src/components/drawing/index.js | 6 +----- src/snapshot/tosvg.js | 16 ++++++++++++++++ test/jasmine/tests/snapshot_test.js | 15 +++++++-------- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 88841fc20b2..443f882d95c 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -334,11 +334,7 @@ drawing.gradient = function(sel, gd, gradientID, type, colorscale, prop) { }); }); - // Set url to gradient definition as an 'attribute', so that - // we don't have to fiddle with quotes inside of quotes in Snapshot.toSVG. - // Clear corresponding 'style' setting to avoid potential conflicts. - sel.attr(prop, 'url(#' + fullID + ')') - .style(prop, null) + sel.style(prop, 'url(#' + fullID + ')') .style(prop + '-opacity', null); }; diff --git a/src/snapshot/tosvg.js b/src/snapshot/tosvg.js index 55a7a8f1629..ae320d71ac7 100644 --- a/src/snapshot/tosvg.js +++ b/src/snapshot/tosvg.js @@ -116,6 +116,22 @@ module.exports = function toSVG(gd, format, scale) { } }); + svg.selectAll('.point, .scatterpts, .legendfill>path, .legendlines>path, .cbfill').each(function() { + var pt = d3.select(this); + + // similar to font family styles above, + // we must remove " after the SVG DOM has been serialized + var fill = this.style.fill; + if(fill && fill.indexOf('url(') !== -1) { + pt.style('fill', fill.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); + } + + var stroke = this.style.stroke; + if(stroke && stroke.indexOf('url(') !== -1) { + pt.style('stroke', stroke.replace(DOUBLEQUOTE_REGEX, DUMMY_SUB)); + } + }); + if(format === 'pdf' || format === 'eps') { // these formats make the extra line MathJax adds around symbols look super thick in some cases // it looks better if this is removed entirely. diff --git a/test/jasmine/tests/snapshot_test.js b/test/jasmine/tests/snapshot_test.js index 21c8985c0c8..90c875e535d 100644 --- a/test/jasmine/tests/snapshot_test.js +++ b/test/jasmine/tests/snapshot_test.js @@ -254,7 +254,7 @@ describe('Plotly.Snapshot', function() { describe('should handle quoted style properties', function() { function checkURL(actual, msg) { // which is enough tot check that toSVG did its job right - expect((actual || '').substr(0, 5)).toBe('url(#', msg); + expect((actual || '').substr(0, 6)).toBe('url(\"#', msg); } it('- marker-gradient case', function(done) { @@ -277,7 +277,7 @@ describe('Plotly.Snapshot', function() { }); d3.selectAll('.point,.scatterpts').each(function() { - checkURL(d3.select(this).attr('fill')); + checkURL(this.style.fill); }); return Plotly.Snapshot.toSVG(gd); @@ -297,12 +297,12 @@ describe('Plotly.Snapshot', function() { expect(pointElements.length).toEqual(3); for(i = 0; i < pointElements.length; i++) { - checkURL(pointElements[i].getAttribute('fill')); + checkURL(pointElements[i].style.fill); } var legendPointElements = svgDOM.getElementsByClassName('scatterpts'); expect(legendPointElements.length).toEqual(1); - checkURL(legendPointElements[0].getAttribute('fill')); + checkURL(legendPointElements[0].style.fill); }) .catch(failTest) .then(done); @@ -319,12 +319,11 @@ describe('Plotly.Snapshot', function() { var fillItems = svgDOM.getElementsByClassName('legendfill'); for(var i = 0; i < fillItemIndices.length; i++) { - var path = fillItems[fillItemIndices[i]].firstChild; - checkURL(path.getAttribute('fill'), 'fill gradient ' + i); + checkURL(fillItems[fillItemIndices[i]].firstChild.style.fill, 'fill gradient ' + i); } var lineItems = svgDOM.getElementsByClassName('legendlines'); - checkURL(lineItems[1].firstChild.getAttribute('stroke'), 'stroke gradient'); + checkURL(lineItems[1].firstChild.style.stroke, 'stroke gradient'); }) .catch(failTest) .then(done); @@ -341,7 +340,7 @@ describe('Plotly.Snapshot', function() { var fillItems = svgDOM.getElementsByClassName('cbfill'); expect(fillItems.length).toBe(1, '# of colorbars'); for(var i = 0; i < fillItems.length; i++) { - checkURL(fillItems[i].getAttribute('fill'), 'fill gradient ' + i); + checkURL(fillItems[i].style.fill, 'fill gradient ' + i); } }) .catch(failTest)