Re: Move shared testing code into test-util.js (issue 6772044)

[a...@chromium.org]

https://codereview.appspot.com/6772044/

[usrb...@yahoo.com]

I agree that no matter what, a lot of these functions are better broken
out into a utility library, but if the goal is to be able to use those
functions to share code with an output-checking script, see the next
section.

----

I don't know why I didn't notice it before, but you can actually easily
add
output checking to the current structure:

#--cut--

git checkout 4508897258b4781e # recent HEAD for branch master
git checkout -b issue6772044-test-output

cat > test-output.diff <<"END"
diff --git a/test/feature/out.js b/test/feature/out.js
new file mode 100644
index 0000000..aad1052
--- /dev/null
+++ b/test/feature/out.js
@@ -0,0 +1,7 @@
+// Test output.
+/*
+--cut--
+var a;
+--cut--
+*/
+var a;
diff --git a/test/testfeatures.js b/test/testfeatures.js
index 7a0a31a..87bf786 100644
--- a/test/testfeatures.js
+++ b/test/testfeatures.js
@@ -120,6 +120,7 @@ function testScript(filePath) {
var skip = false;
var shouldCompile = true;
var expectedErrors = [];
+ var testOutput = false;
forEachPrologLine(source, function(line) {
var m;
if (line.indexOf('// Only in browser.') === 0) {
@@ -132,6 +133,8 @@ function testScript(filePath) {
traceur.options.fromString(m[1]);
} else if ((m = /\/\/ Error:\s*(.+)/.exec(line))) {
expectedErrors.push(m[1]);
+ } else if (line.indexOf('// Test output.') === 0) {
+ testOutput = true;
}
});

@@ -175,6 +178,24 @@ function testScript(filePath) {
return false;
}

+ if (testOutput) {
+ var f = function(k, v) {
+ return k === 'location' ? undefined : v;
+ }
+ var SourceFile = traceur.syntax.SourceFile;
+ var compileFile = traceur.codegeneration.Compiler.compileFile;
+
+ var s1 = new SourceFile(filePath, source.split('--cut--')[1]);
+ var t1 = compileFile(reporter, s1, filePath);
+ var o1 = JSON.stringify(t1, f, 2);
+ var o2 = JSON.stringify(tree, f, 2);
+ if (o1 === o2)
+ return true;
+
+ failScript(filePath, 'Output mismatch.');
+ return false;
+ }
+
var TreeWriter = traceur.outputgeneration.TreeWriter;
var javascript = TreeWriter.write(tree, false);
END

git apply test-output.diff
node test/testfeatures.js

#--cut--


https://codereview.appspot.com/6772044/

[usrb...@yahoo.com]

I got caught again by the publish + mail drafts UI trap.


https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js
File test/testfeatures.js (left):

https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js#oldcode299
test/testfeatures.js:299: }
Do you plan to eventually do something like

var asserts = testUtil.asserts;

to allow the for loop above to work unchanged?

https://codereview.appspot.com/6772044/

[a...@chromium.org]

This patch was a step in the direction of moving more tests to run in
node as well as in browsers.

https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js
File test/testfeatures.js (left):

https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js#oldcode299
test/testfeatures.js:299: }

On 2012/11/11 13:28:55, usrbincc wrote:
> Do you plan to eventually do something like

> var asserts = testUtil.asserts;

> to allow the for loop above to work unchanged?

At some point I would like to get rid of the dependency on Closure
Library for testing. It is too large for our need. Once we have
something else we can revisit this.

https://codereview.appspot.com/6772044/

[usrb...@yahoo.com]

Sorry about that. I actually entirely skipped by the feature_test.html
diff, because I hardly use the html tests. It makes much more sense now.

I think it should work fine. It looks like the only things that you need
are the asserts, a 'fail' function, and something to run all the tests.

// Simplest possible implementation
function fail(msg) {
console.error(msg);
}
Object.keys(window).filter(function(k) {
return k.match(/^test/);
}).forEach(function(k) {
window[k]();
});

As long as the new tests don't have to be as pretty as the old ones, it
should be fairly straightforward for the individual tests. It's the
test/alltests.html code that I'm not too sure about.

I don't do much advanced DOM, but I think just loading the files in
the testPaths array serially in an iframe and using exceptions or
maybe passing a callback (to pass test results back to the main page)
into the iframe (don't know if it's possible) would work. Or maybe I
should just look at how closure does it. I'm trying to avoid that for
now, though. Closure, as you said (in a different context) is too
large.

https://codereview.appspot.com/6772044/

[a...@chromium.org]

This is off topic.

The Closure tests does a lot more than this but I agree that at the core
this is the important part.

Lets focus on this patch. Is it good enough or are there things that
needs to be done before committed?

https://codereview.appspot.com/6772044/

[a...@chromium.org]

https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js
File test/testfeatures.js (left):

https://codereview.appspot.com/6772044/diff/1/test/testfeatures.js#oldcode299
test/testfeatures.js:299: }
On 2012/11/11 13:28:55, usrbincc wrote:
> Do you plan to eventually do something like

> var asserts = testUtil.asserts;

> to allow the for loop above to work unchanged?

The Closure asserts are a lot richer so I don't want to replace those
yet.

https://codereview.appspot.com/6772044/

[usrb...@yahoo.com]

Right, focus, focus.

The architectural change LGTM. It's code duplication, and should be
eliminated.

It looks like you want to think on it a little before making the break
with Closure Library, so I'll wait for a later patch to get into that.


https://codereview.appspot.com/6772044/

[usrb...@yahoo.com]