Hover text bug

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃

[cldougl]

• fix for hover label bug: Cartesian plot with 'hoverinfo': 'text' or 'y+text' and 'hovermode': 'x' does not show hover text #43
• add hover tests

[etpinard]

@cldougl you might want to try http://nategood.com/sublime-text-strip-whitespace-save

to avoid those nasty diff lines.

[cldougl]

will do

[etpinard]

@alexcjohnson this has me thinking that perhaps flaglist attributes should be stored in fullData as arrays instead of strings, to make comparison more seamless. Moreover, maybe the all flag should be expected to e.g. x+y+text+name in the default step.

Alternatively, we could cook a nice general lib function isInFlagList that would take care of the logic on demand.

[alexcjohnson]

I like isInFlagList better (I like it a lot, in fact 👍 ) - fullData isn't supposed to change things from data unless they're invalid, it's just supposed to fill in the blanks.

[etpinard]

Great. Let's keep that in mind when we get to #34. There are only a few flaglist attributes at the moment, but more are coming.

[etpinard]

💥 nice <tspan>s

[bpostlethwaite]

It looks like we could have 1 beforeEach and multiple its?
pseudocode

varmockData,mockLayoutdescribe('hover tests',function(){mockData=mockData()mockLayout=mockLayout()beforeEach(function(done){Plotly.plot(gd,mockData,mockLayout).then(done)})it('responds to hover',function(){// test that uses mockData and mockLayout }it('resonds to hover x',function(){// test ... }})

We can put the var declarations global scope and then assign fresh copies in the single BeforeEach. No need for multiple describes and beforeEach's. If that is dooable is should really clear up the test.

For comparison check out the many Jest (Jasmine fork) tests in Filewell. We have a set of patterns that might be useful. https://github.com/plotly/streambed/blob/master/shelly/filewell/static/filewell/src/stores/__tests__/DirectoryStore-test.js

Here there is one BeforeEach to refresh state before each test and a few describe blocks that break the test into logical chunks. If we are just testing hover here one describe is probably enough

Check around the filewell src tree, every folder has a __test__ folder with Jest tests.

[etpinard]

Nop. That won't because we call Plotly.plot with different data each time.

We could only force update hovermode in gd directly without a plot call and test the labels then. But that feels kind of dirty.

[bpostlethwaite]

ah aight - the only other suggestion I can think of is to remove the multiple BeforeEach and Describes and do something like

it('does hover things',function(done){Plotly.plot(newGD(),newData(),newLayout()).then(function(){expect(thingsToHappen()).toBe(true)done()})})

where you get shorter code but have 1 level of nesting in the tests.

[bpostlethwaite]

looks pretty good to me. I think we can refactor the test to simplify it somewhat. Let me know if I can help with that. We have written hundreds of these damn things in Filewell and have a set of test helpers and patterns for dealing with State refresh

[bpostlethwaite]

Okay previous suggestion was not going to work as @etpinard pointed out. Made another suggestion. It may or may not be worth it, your call. Looks 💃 to me!

Nice work!

[cldougl]

Thanks for the tests suggestions/explanations @bpostlethwaite ! I have both options worked out- @etpinard or @mdtusz do you have a preference on which format we use for this (ben's nested suggestion or the current nested describes)?

[cldougl]

@bpostlethwaite@etpinard false alarm- the new (done) pattern doesn't actually work (same problem as yesterday you can test expect(d3.selectAll('g.axistext').select('text').html()).toEqual(''); or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('1');) or expect(d3.selectAll('g.axistext').select('text').html()).toEqual('a'); and the test will pass. We should just leave them as is with the beforeEach()

[etpinard]

💃