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.

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

Here is the first commit that supports automated testing #60

Merged
merged 8 commits into from Apr 28, 2011

Conversation

Projects
None yet
2 participants
@kberg
Collaborator

kberg commented Apr 25, 2011

I'll put more stuff in there soon -- real tests, and whatnot.

Something that I don't love about this is that the contexts, which must be captured, are private (since they are named canvas_ctx_, and hidden_ctx_.) So my tests at the moment access hidden_ctx_. Thoughts?

your test.</li>
</ol>
</li>
<li>Run generate-combined.sh before every run since it's required (can we fix

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

why is this necessary? If you include the four JS files that all the tests/*.html tests do, then you should be fine.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

It's necessary because if it's not compiled, jstestdriver complains that there are parse errors in that file.

@@ -0,0 +1,565 @@
/*
* Copyright 2009 Google Inc.

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

Is auto_tests/lib a part of js test driver? If so, could we name it auto_tests/jstestdriver, not auto_tests/lib?

@@ -0,0 +1,23 @@
<!-- A local source file that allows me to test locally. -->

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

me -> a dygraphs developer?

<!-- A local source file that allows me to test locally. -->
<html>
<head>
<script type="text/javascript" src="../../dygraph-combined.js"></script>

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

replace w/ includes of rgbcolor.js and strftime-min.js to avoid needing to generate-combined.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

Thank you. That's the advice I was looking for. :)

@kberg

This comment has been minimized.

Collaborator

kberg commented Apr 27, 2011

auto_tests/lib contains a variety of additional components required to run automated tests. The .jar file is for running via command-line. The other two are required to run tests inside misc/local.html. They're included with js-test-driver, but not accessible from inside that .jar. Or is it? Let me see what's inside there.

@@ -0,0 +1,103 @@
// Copyright (c) 2011 Google, Inc.

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

Does this have to be copyright Google? I'm not sure what the relevant rules are, here.

// found = 1 when prior loop found p1.
// found = 2 when prior loop found p2.
var priorFound = 0;
for (var callNum in proxy.calls__) {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

is proxy.calls_ an array? If so, you should iterate over like so:

for (var i = 0; i < proxy.calls__.length; i++) {
var call = proxy.calls__[callNum];
}

Otherwise it will break when using JS libraries like prototype. (you should avoid for..in whenever possible)

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

thanks fixed.

* attrs is meant to be used when you want to track things like
* color and stroke width.
*/
CanvasAssertions.assertLineDrawn = function(proxy, p1, p2, attrs) {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

Say I did something like:

ctx.moveTo(100, 100);
ctx.lineTo(200, 100);
ctx.lineTo(200, 200);
ctx.stroke();

Then would assertLineDrawn(ctx, {100,100}, {200,200}) fail? It should.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

It would absolutely fail. It would also fail in this case:

ctx.moveTo(100, 100);
ctx.lineTo(150, 150);
ctx.lineTo(200, 200);
ctx.stroke();

even though that's essentially the same.

More assertions will come over time, things like "count segments", et cetera.

// THE SOFTWARE.
/**
* @fileoverview A general purpose object proxy that logs all method calls.

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

please document which browsers this works in.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

So that's a good point. Right now this just has been tested in Chrome/linux/mac. Should I put the compatibility comment in README.html instead?

*
* @author konigsberg@google.com (Robert Konigsberg)
*/
var DEAD_SIMPLE_DATA = [[ 20061010, 2100 ]];

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

ping on the comment about not using this date format. Unless you intend for it to be a number, in which case it's an odd number to choose :)

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

I agree. I made the change in my greenfield work. I'll move it over here too.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

(also I didn't much care about the x-axis.)

assertEquals([0, 50], g.yAxisRange(0));
};
function assertEqualsDelta(msg, expected, actual, delta) {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

docstring here. Specifically, please specify whether the delta is inclusive (<=) or exclusive (<)

/**
* Test that valueRange matches the y-axis range specifically.
*
* This is based on the assumption that 20 pixels are dedicated to the

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

Please set this 20 explicitly (unless you want a test that checks whether the default is equal to this).

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

You mentioned to me somewhere else that there might be an option I can set for this. What is that? I would like to hard-code it.

* axis label and tick marks.
* TODO(konigsberg): change yAxisLabelWidth to 0 (or 20) and try again.
*/
SanityTestCase.prototype.testToDomYCoord = function() {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

Another useful test would verify that toDomCoords and toDataCoords are actually inverses of one another.

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

Agreed. I will add that as a TODO as this is still proof of concept. (Perhaps you would like to consider that your first test. :))

Also, perhaps we should move all tests like that to their own package (e.g. not sanity.) Also a todo.

Dygraph.getContext = _origFunc;
};
SimpleDrawingTestCase.prototype.testDrawSimple = function() {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

this test doesn't test anything!

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

Oops, works in progress. Sorry.

var g = new Dygraph(graph, ZERO_TO_FIFTY, {});
}
SimpleDrawingTestCase.prototype.testDrawSimpleRangeEquals = function() {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

same here

var g = new Dygraph(graph, ZERO_TO_FIFTY, opts);
var htx = g.hidden_ctx_;
CanvasAssertions.assertLineDrawn(htx, [56,300], [475,5.8], {

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

These values are really magical. Could you use toDomCoords to generate them from (0,0) and (50,50)? Or do you think that would diminish the value of the test?

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

That's a good question. I think having these magic values once in a while doesn't make the test brittle. In fact I think they're more important than toDomBlahBlah.

However, I could see adding some assertions that show where the values come from. What do you think of that?

server: http://localhost:9876
load:
- dygraph-combined.js

This comment has been minimized.

@danvk

danvk Apr 27, 2011

Owner

-> rgbcolor/rgbcolor.js
-> strftime/strftime-min.js

??

This comment has been minimized.

@kberg

kberg Apr 27, 2011

Collaborator

Correct.

Robert Konigsberg added some commits Apr 27, 2011

Robert Konigsberg
Moving the delta tests to MoreAsserts. MoreAsserts is actually going …
…to go away when this code goes in to jstestdriver. I don't suggest you ... don't look too hard at MoreAsserts, but don't look too hard. A proper review will be done on the jstestdriver side.

@danvk danvk merged commit 1ca7887 into danvk:master Apr 28, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment