Short version:
Someone contributed wrote a bunch of code to
Dygraphs. It broke an existing behavior, but also inadvertently (or advertently) fixed another one that was broken before his patch was submitted. The new broken behavior was more of a problem than the old broken behavior, so Nealie provided a fix which restored the old broken behavior. And then I went ahead and did a bunch of tests. And as always,
patches are welcome!
Long version:
Nealie submitted a rather comprehensive
patch to Dygraphs that changed the way zooming events behaved. Here's the use case that he inadvertently broke:
- Open http://dygraphs.com/tests/zoom.html
- Use the mouse to zoom in on a graph along both the X and Y axes.
- Perform an unzoom by clicking the “Unzoom” button. This effectively calls
g.updateOptions({dateRange: null, valueWindow: null})
- Expected behavior: both the original x and y ranges are restored.
- Actual behavior: Only the x-axis range is restored.
Now, here’s the use case that was broken before his patch, and subsequently fixed:
- Open http://dygraphs.com/tests/zoom.html
- Use the mouse to zoom in on a graph along both the X and Y axes.
- Make the following call from a javascript console:
g.updateOptions({})
- Expected behavior: this should effectively be a no-op.
- Actual behavior: The y-axis restores to the original range.
In other words, Nealie’s patch made
g.updateOptions({}) work just fine, while breaking
g.updateOptions({dateRange: null, valueWindow: null});
I figured this out by basically looking at a series of commits and checkout then out at different times along the main code path. You can see my work
here. If you read that document, you’ll notice that when performing the same zoom operations via
g.updateOptions({…}), neither bug appears. It’s only when a zoom is performed via mouse operations.
Much of this could have been avoided if we had automated tests. Fortunately, I’m working on that, and in fact an initial attempt at them is now in a
pull request. It uses the
js-test-driver framework. If we had these automated tests in place beforehand, then it might be safe to say that neither bug would have made it in to the main branch.
For instance, here’s a simple test that verifies that verifies calls to
updateOptions should do what I expect:
RangeTestCase.prototype.testRangeSetOperations = function() {
var graph = document.getElementById("graph");
var g = new Dygraph(graph, ZERO_TO_FIFTY, { });
assertEquals([10, 20], g.xAxisRange());
assertEquals([0, 55], g.yAxisRange(0));
g.updateOptions({ dateWindow : [ 12, 18 ] });
assertEquals([12, 18], g.xAxisRange());
assertEquals([0, 55], g.yAxisRange(0));
g.updateOptions({ valueRange : [ 10, 40 ] });
assertEquals([12, 18], g.xAxisRange());
assertEquals([10, 40], g.yAxisRange(0));
g.updateOptions({ });
assertEquals([12, 18], g.xAxisRange());
assertEquals([10, 40], g.yAxisRange(0));
g.updateOptions({ dateWindow : null, valueRange : null });
assertEquals([10, 20], g.xAxisRange());
assertEquals([0, 55], g.yAxisRange(0));
};
To be fair, that test doesn’t really test the use cases above, where the mouse operations are the cause of broken behavior. I haven’t written those tests yet, but here’s one I did write, which verifies the behavior of a mouse double-click:
RangeTestCase.prototype.testDoubleClick = function() {
var graph = document.getElementById("graph");
var g = new Dygraph(graph, ZERO_TO_FIFTY, { });
assertEquals([10, 20], g.xAxisRange());
assertEquals([0, 55], g.yAxisRange(0));
g.updateOptions({ dateWindow : [ 12, 18 ] });
g.updateOptions({ valueRange : [ 10, 40 ] });
assertEquals([12, 18], g.xAxisRange());
assertEquals([10, 40], g.yAxisRange(0));
var evt = document.createEvent('MouseEvents');
evt.initMouseEvent(
'dblclick', true, true, document.defaultView,
2, 0, 0, 0, 0
0, false, false, false, false, 0, null);
g.canvas_.dispatchEvent(evt);
assertEquals([10, 20], g.xAxisRange());
assertEquals([0, 55], g.yAxisRange(0));
}
You get the idea.
If you read those tests carefully, you would probably have four criticisms:
- Accessing to the private attribute
canvas_ should be discouraged.
- All those magic numbers used for initializing the event.
- Having to actually dispatch the event via the DOM.
- Questioning browser compatibility - what browsers would that work on?
Here are my answers:
- Yes, you’re right. That should be fixed.
- Yes, you’re right. That should read like
DygraphTestUtils.dispatchDoubleClick(g);
- You’re right about that one also. Right now the API doesn’t quite make that possible, but it could be worked on if necessary. (Psst … patches welcome!)
- Yeah, I have no idea if that would work on anything other than Chrome. One thing at a time.
Finally, just what will we do about this behavior? Here's what I think:
First, I think we should accept Nealie's bug fix. Second, figure out why and how the both cases can be fixed. Third, get some comprehensive automated tests in place, make the tests easy to write and the framework easy to run. Fourth, and probably most important, change the submission guidelines to require a) automated tests to be run as part of the patch submission process and b) require new tests to be part of patches of a certain level of complexity.
