Expect.equals and Expect.identical escape more strings

pkg/expect/lib/expect.dart

Index: pkg/expect/lib/expect.dart
diff --git a/pkg/expect/lib/expect.dart b/pkg/expect/lib/expect.dart
index 4ee2832d383bb10203d22ad7edcd9c98648aa0b5..db3c42a6814dedf9ccbf6e68ba46e8637fcbdcbb 100644
--- a/pkg/expect/lib/expect.dart
+++ b/pkg/expect/lib/expect.dart
@@ -26,7 +26,7 @@ class Expect {
    * [start] and increasing [end], up to at most length characters.
    * If the start or end of the slice are not matching the start or end of
    * the string, ellipses are added before or after the slice.
-   * Control characters may be encoded as "\xhh" codes.
+   * Characters other than printable ASCII are escaped.
    */
   static String _truncateString(String string, int start, int end, int length) {
     if (end - start > length) {
@@ -40,21 +40,39 @@ class Expect {
       if (start < 0) start = 0;
       if (end > string.length) end = string.length;
     }
-    if (start == 0 && end == string.length) return string;
     StringBuffer buf = new StringBuffer();
     if (start > 0) buf.write("...");
+    _escapeSubstring(buf, string, 0, string.length);
+    if (end < string.length) buf.write("...");
+    return buf.toString();
+  }
+
+  /// Return the string with characters that are not printable ASCII characters
+  /// escaped as either "\xXX" codes or "\uXXXX" codes.
+  static String _escapeString(String string) {
+    StringBuffer buf = new StringBuffer();
+    _escapeSubstring(buf, string, 0, string.length);
+    return buf.toString();
+  }
+
+  static _escapeSubstring(StringBuffer buf, String string, int start, int end) {

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

If we are trying to be "complete" here, then we should probably also escape
backslashes, otherwise the string 
  "\\u{123}\u{123}"
will be escaped into 
  \u{123}\u{123}
which isn't really correct.

Or maybe we should just use JSON.encode from dart:convert, it already handles
all these things (but sadly only using \u, not \x).

[sra1, 2017/04/06 21:19:18]

Done.

+    const hexDigits = "0123456789ABCDEF";
     for (int i = start; i < end; i++) {
       int code = string.codeUnitAt(i);
-      if (code < 0x20) {
+      if (0x20 <= code && code < 0x7F) {
+        buf.writeCharCode(string.codeUnitAt(i));

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

string.codeUnitAt(i) -> code

[sra1, 2017/04/06 21:19:18]

Done.

+      } else if (code < 0x100) {
         buf.write(r"\x");

[sra1, 2017/03/17 05:34:56]

I would also be happy combining \x and \u into \u{XXX}
as in

buf.write(r"\u{");
buf.write(code.toRadixString(16));
buf.write("}");

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

I'd prefer not doing that. I find \x more readable, probably beacuse it's
immediately clear what range it is in. Reserving \u for non-Latin-1 characters
immediately tells you that \u{XXX} isn't something you would recognize.

[sra1, 2017/04/06 21:19:18]

Acknowledged.

-        buf.write("0123456789abcdef"[code ~/ 16]);
-        buf.write("0123456789abcdef"[code % 16]);
+        buf.write(hexDigits[code >> 4]);
+        buf.write(hexDigits[code & 15]);
       } else {
-        buf.writeCharCode(string.codeUnitAt(i));
+        buf.write(r"\u");
+        buf.write(hexDigits[(code >> 12) & 15]);

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

You technically don't need "& 15" for the top bits (but then, this is clearly
not optimized for speed, so who cares? Well, apparently I do - mostly because
it's inconsistent with the two-digit case :).

[sra1, 2017/04/06 21:19:18]

Acknowledged.

+        buf.write(hexDigits[(code >> 8) & 15]);
+        buf.write(hexDigits[(code >> 4) & 15]);
+        buf.write(hexDigits[code & 15]);
       }
     }
-    if (end < string.length) buf.write("...");
-    return buf.toString();
   }
 
   /**
@@ -98,6 +116,8 @@ class Expect {
       if (stringDifference != null) {
         _fail("Expect.equals($stringDifference$msg) fails.");
       }
+      _fail("Expect.equals(expected: <${_escapeString(expected)}>"
+            ", actual: <${_escapeString(actual)}>$msg) fails.");
     }
     _fail("Expect.equals(expected: <$expected>, actual: <$actual>$msg) fails.");
   }
@@ -145,6 +165,11 @@ class Expect {
   static void identical(var expected, var actual, [String reason = null]) {
     if (_identical(expected, actual)) return;
     String msg = _getMessage(reason);
+    if (expected is String && actual is String) {

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

You might want a special case here for when the strings are *equal* but not
*identical*, otherwise the error will look odd.
Can't happen in JS, but it can on the VM.

[sra1, 2017/04/06 21:19:18]

Acknowledged.

+      _fail("Expect.identical(expected: <${_escapeString(expected)}>"
+            ", actual: <${_escapeString(actual)}>$msg) "

[Lasse Reichstein Nielsen, 2017/03/17 06:38:08]

In the long run, we might want a "prettify" function that can generalize to
other types as well (and be applied recursively to collections).

[sra1, 2017/04/06 21:19:18]

Acknowledged.

+            "fails.");
+    }
     _fail("Expect.identical(expected: <$expected>, actual: <$actual>$msg) "
         "fails.");
   }