Dartest CL - Please review

client/testing/unittest/unittest_dartest.dart

Index: client/testing/unittest/unittest_dartest.dart
===================================================================
--- client/testing/unittest/unittest_dartest.dart	(revision 0)
+++ client/testing/unittest/unittest_dartest.dart	(revision 0)
@@ -0,0 +1,93 @@
+// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/**
+ * A simple unit test library for running tests in a browser.

[Bob Nystrom, 2011/12/14 00:15:30]

Update this comment to describe what this entrypoint is for.

[shauvik, 2011/12/16 07:19:27]

Done.

+ */
+#library("unittest");
+
+#import("dart:dom");
+
+#source("shared.dart");
+
+/** Getter for getting tests */

[Bob Nystrom, 2011/12/14 00:15:30]

Obvious. Remove or make it say something I don't already know. :)

[shauvik, 2011/12/16 07:19:27]

Ok - added more detail :)

On 2011/12/14 00:15:30, Bob Nystrom wrote:

+List<TestCase> get tests() => _tests;
+
+int testsRun = 0, testsFailed = 0, testsErrors = 0;

[Bob Nystrom, 2011/12/14 00:15:30]

We don't usually declare multiple variables on one line. I'd split this up.

[shauvik, 2011/12/16 07:19:27]

Done.

+int get numTestsRun() => testsRun;
+int get numTestsFailed() => testsFailed;
+int get numTestsErrors() => testsErrors;
+
+bool previousAsyncTest = false;
+
+Function uiUpdator = null;

[Bob Nystrom, 2011/12/14 00:15:30]

"updateUI".

[shauvik, 2011/12/16 07:19:27]

Done.

+set uiUpdateFunc(Function updateFunc) => uiUpdator = updateFunc;

[Bob Nystrom, 2011/12/14 00:15:30]

Remove, you can just do "updateUI = ..."

[shauvik, 2011/12/16 07:19:27]

Done.

+
+Function dartestLogger = null;
+set dartestLoggingFunc(Function logFunc) => dartestLogger = logFunc;

[Bob Nystrom, 2011/12/14 00:15:30]

Remove.

[shauvik, 2011/12/16 07:19:27]

Done.

+
+_platformDefer(void callback()) {
+  _testRunner = runDARTests;
+  // DARTest ignores the callback. Tests are launched from UI
+}
+
+// Update test results
+updateTestStats(TestCase test) {
+  assert(test.result != null);
+  if(test.startDate != null){
+    test.runningTime = (new Date.now()).difference(test.startDate);
+  }
+  testsRun++;
+  switch (test.result) {
+    case 'fail': testsFailed++; break;
+    case 'error': testsErrors++; break;   
+  } 
+  uiUpdator(test);
+}
+
+// Run tests sequentially
+runDARTests(){

[Bob Nystrom, 2011/12/14 00:15:30]

Dart shouldn't be all caps. "runDartests" or "runDartTests".

[shauvik, 2011/12/16 07:19:27]

Done.

+  if(previousAsyncTest){

[Bob Nystrom, 2011/12/14 00:15:30]

Space between "if" and "(". Also between ")" and "{". Here and elsewhere.

[shauvik, 2011/12/16 07:19:27]

Done.

+    updateTestStats(_tests[_currentTest-1]);

[Bob Nystrom, 2011/12/14 00:15:30]

Space around "-".

[shauvik, 2011/12/16 07:19:27]

Done.

+    previousAsyncTest = false;
+  }
+  if(_currentTest < _tests.length) {
+    final testCase = _tests[_currentTest];
+    dartestLogger('Running test:'+testCase.description);

[Bob Nystrom, 2011/12/14 00:15:30]

Space around "+".

[shauvik, 2011/12/16 07:19:27]

Done.

+    testCase.startingDate = new Date.now();
+    _runTest(testCase);
+    if (!testCase.isComplete && testCase.callbacks > 0) {
+      previousAsyncTest = true;
+      return;
+    }
+    updateTestStats(testCase);
+    _currentTest++;
+    window.setTimeout(runDARTests, 0);
+  }
+}
+
+_platformStartTests() {
+  //Support for VM and command line coming soon!

[Bob Nystrom, 2011/12/14 00:15:30]

Make this a TODO comment:

// TODO(shauvik): Support for VM and command line coming soon!

That way it's easy to grep for it.

[shauvik, 2011/12/16 07:19:27]

Done.

+  window.console.log("Warning: Running DARTest from VM or Command-line.");
+}
+
+_platformInitialize() {
+  //Do nothing

[Bob Nystrom, 2011/12/14 00:15:30]

Space after "//".

[shauvik, 2011/12/16 07:19:27]

Done.

+}
+
+_platformCompleteTests(int testsPassed, int testsFailed, int testsErrors) {
+  //Do nothing
+}
+
+String getTestResultsCSV() {

[Bob Nystrom, 2011/12/14 00:15:30]

"getTestResultsCsv".

[shauvik, 2011/12/16 07:19:27]

Done.

+  StringBuffer out = new StringBuffer();

[Bob Nystrom, 2011/12/14 00:15:30]

Just use "final" instead of a type annotation here.

[shauvik, 2011/12/16 07:19:27]

Done.

+  for(TestCase test in _tests) {

[Bob Nystrom, 2011/12/14 00:15:30]

Space after "for".

[shauvik, 2011/12/16 07:19:27]

Done.

+    String result = 'none';
+    if(test.result != null) {
+      result = test.result.toUpperCase();
+    }
+    out.add('${test.id}, "${test.description}", $result\n');
+  }
+  return out.toString();
+}