juttle / juttle-viz

Juttle Visualization Library
Apache License 2.0
11 stars 4 forks source link

missing charts tests #29

Closed rlgomes closed 8 years ago

rlgomes commented 8 years ago

fixes #21

Will use this PR to increase test coverage of all of the charts under lib/charts and also unify the way the charts are tested and the way their tests are written. This includes:

rlgomes commented 8 years ago

@go-oleg @mnibecker early review on approach. I'm intending on also restructing the tests for each chart but right now just want to know how you feel about the small changes like the ones I made to the piechart to behave more like the barchart does with returning promies on draw and keeping track of the _currentData

rlgomes commented 8 years ago

@go-oleg fiddled some more but I think we should discuss with others about actually refactoring of the lib/charts and lib/data-targets so things are easily testable and also more consistent with each other. The consistency thing comes from the difference in the way the draw() method for some charts returns a promise vs others doesn't and actually how to deal with transitions (I took a wack at the options.duration thing but even this is starting to look very messy). See who else should be involved in the discussion here.

rlgomes commented 8 years ago

Closing this as after attempting this testing effort from the bottom up I've come to the conclusion (with @go-oleg 's help) that we should invert the order and instead write tests here at the View level to verify charts instantiate and render to the fakebrowser DOM the expected results.