Unit test improvements.

pkg/unittest/lib/unittest.dart

Index: pkg/unittest/lib/unittest.dart
===================================================================
--- pkg/unittest/lib/unittest.dart	(revision 22094)
+++ pkg/unittest/lib/unittest.dart	(working copy)
@@ -181,8 +181,16 @@
 
 Configuration _config;
 
-/** [Configuration] used by the unittest library. */
-Configuration get unittestConfiguration => _config;
+/**
+ * [Configuration] used by the unittest library. Note that if a 
+ * configuration has not been set, calling this getter will create
+ * a default configuration. */

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

multiline comment format fix: move */ to it's own line.

[gram, 2013/04/29 21:24:18]

Done.

+Configuration get unittestConfiguration {
+  if (_config == null) {
+    _config = new Configuration();

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

this is already done just before onInit, why do we need to set it here? 

Is it so people can adjust the default values? If so, then we can either:
 - make them create a new configuration object, or
 - remove the allocation from below and read unittestConfiguration instead.

[gram, 2013/04/29 21:24:18]

Yes, it is so they can set values. I don't want them to have to do an explicit
allocation if we can do a default one for them, which is why I avoided the first
option. I'm not sure I understand your second option; can you elaborate?

[Siggi Cherem (dart-lang), 2013/04/30 00:42:33]

by option 2 I meant to consider replacing lines 832-835:

 if (_config == null) {
   unittestConfiguration = new Configuration();
 }
 _config.onInit();

by:
  unittestConfiguration.onInit();

[gram, 2013/04/30 01:00:09]

Done.

+  }
+  return _config;
+}
 
 /**
  * Sets the [Configuration] used by the unittest library.
@@ -859,3 +867,51 @@
 
 /** Signature for a test function. */
 typedef dynamic TestFunction();
+
+// Stack formatting utility. Strips extraneous content from a stack trace.
+// Each original line is of the form #<n> <function> (<location url>).
+final _nativeFrameRegExp = new RegExp(
+    r'^#\d+\s+([^\s].*) (\(.+:\d+:\d+\))$');
+
+String _formatStack(stack) {

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

as we were discussing offline. why can't we use the stack_trace package here?

Where do these stack traces work? in dart, dartium, dart2js? Is the stack trace
format the same in all of them?

[gram, 2013/04/29 21:24:18]

In the latest version they work in Firefox, Dartium, Chrome and VM (I haven't
tested IE or Opera). As we discussed, I don't want to take a dependency on a
package (yet), and most of the processing I am doing is specific to unit test.

+  var lines;
+  if (stack is StackTrace) {
+    lines = stack.toString().split('\n');
+  } else if (stack is String) {
+    lines = stack.split('\n');
+  } else {
+    return stack.toString();
+  }
+  
+  // We remove all entries that have a location in unittest.
+  // We strip out anything before _nextBatch too.
+  var sb = new StringBuffer();
+  var maxlen = 0;
+  for (var pass = 0; pass < 2; pass++) {

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

I'd prefer if we split this in 2 loops, so that each loop is just concerned with
one task. You might want to also first filter the lines you want to print out,
so the processing is only concerned with formatting at that point:

var relevantLines = lines.where((l) =>
  var match = firtMatch(line);
  return (match != null && match.indexOf('unittest') != 0))
});

int padding = relevantLines.fold(0, (n, line) => math.max(n,
line_member_thing.length));

for (line in relevantLines) { append to buffer }

[gram, 2013/04/29 21:24:18]

The filtering is not that conducive to this now that I handle multiple stack
formats but I have eliminated the loop.

+    for (var i = 0; i < lines.length; i++) {
+      if (lines[i] == '') continue;
+      var match = _nativeFrameRegExp.firstMatch(lines[i]);
+      if (match == null) {
+        throw new FormatException(
+            "Couldn't parse stack trace line '${lines[i]}' from $stack.");

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

I don't think we should throw if we can't parse a frame.

Unittest users want to know about the error in their code and would probably
prefer we show the original error rather than say that we couldn't parse the
stack trace.

[gram, 2013/04/29 21:24:18]

Done.

+      }
+      var member = match[1];
+      var location = match[2];
+      if (location.indexOf('unittest/lib/') >= 0) {

[Siggi Cherem (dart-lang), 2013/04/27 01:36:08]

when running things outside of the repo, the stacktraces don't show 'lib'. Here
is an example stack trace in web_ui:

  #0      main.<anonymous closure>.<anonymous closure>
(file:///usr/local/google/home/sigmund/dart/web-ui-mirror/test/analyzer_test.dart:41:7)
  #1      TestCase._runTest (package:unittest/src/test_case.dart:82:25)
  #2      TestCase._run._run (package:unittest/src/test_case.dart:124:23)
  #3      _guardAsync (package:unittest/unittest.dart:727:19)
  #4      _nextBatch (package:unittest/unittest.dart:767:24)
  #5      runTests.runTests.<anonymous closure>
(package:unittest/unittest.dart:710:15)
  #6      _defer.<anonymous closure> (package:unittest/unittest.dart:669:44)
  #7      _ThenFuture._sendValue (dart:async:397:24)
  #8      _FutureImpl._handleValue.<anonymous closure> (dart:async:270:28)
  #9      Timer.run.<anonymous closure> (dart:async:2251:21)
  #10     Timer.Timer.<anonymous closure> (dart:async-patch:15:15)

[gram, 2013/04/29 21:24:18]

I've addressed this.

+        if (member == '_nextBatch') break;
+      } else if (pass == 0) {
+        // Find max length of first column.
+        if (member.length > maxlen) maxlen = member.length;
+      } else {
+        sb.write(member);
+        // Pad second column to a fixed position.
+        for (var j = 0; j <= maxlen - member.length; j++) {
+          sb.write(' ');
+        }
+        sb.write(location);
+        sb.write('\n');
+      }
+    }
+  }
+  return sb.toString();
+}