Serialize AnalysisErrors to JSON in the analysis server.

pkg/analysis_server/test/analysis_server_test.dart

Index: pkg/analysis_server/test/analysis_server_test.dart
diff --git a/pkg/analysis_server/test/analysis_server_test.dart b/pkg/analysis_server/test/analysis_server_test.dart
index 64cb01042f4984c2697192c7af4851bfd0cf2a17..8b14042e04cc5bc25870e38c41d4dd6b256b2309 100644
--- a/pkg/analysis_server/test/analysis_server_test.dart
+++ b/pkg/analysis_server/test/analysis_server_test.dart
@@ -28,6 +28,12 @@ main() {
         AnalysisServerTest.addContextToWorkQueue_whenRunning);
     test('createContext', AnalysisServerTest.createContext);
     test('echo', AnalysisServerTest.echo);
+    test('errorToJson_formattingApplied',
+        AnalysisServerTest.errorToJson_formattingApplied);
+    test('errorToJson_noCorrection',
+        AnalysisServerTest.errorToJson_noCorrection);
+    test('errorToJson_withCorrection',
+        AnalysisServerTest.errorToJson_withCorrection);
     test('performTask_whenNotRunning',
         AnalysisServerTest.performTask_whenNotRunning);
     test('shutdown', AnalysisServerTest.shutdown);
@@ -89,7 +95,7 @@ class AnalysisServerTest {
       List<AnalysisError> errors =
           channel.notificationsReceived[1].params['errors'];
       expect(errors, hasLength(1));
-      expect(errors[0], equals(analysisError));
+      expect(errors[0], equals(AnalysisServer.errorToJson(analysisError)));
     });
   }
 
@@ -125,6 +131,49 @@ class AnalysisServerTest {
         });
   }
 
+  static void errorToJson_formattingApplied() {
+    Source source = new FileBasedSource.con1(new JavaFile('/foo.dart'));

[scheglov, 2014/04/23 19:20:56]

I think we need to learn how to use mocks in Dart.
If we need a Source which implements only "encoding", we need to configure a
mock to provide only it.
Creating real objects makes tests less obvious.

[Brian Wilkerson, 2014/04/24 15:44:05]

I disagree :-) I prefer to only use mocks for cases where a real object is too
expensive or cumbersome to use. But we can discuss this in person next week.

+    CompileTimeErrorCode errorCode = CompileTimeErrorCode.AMBIGUOUS_EXPORT;
+    AnalysisError analysisError =
+        new AnalysisError.con1(source, errorCode, ['foo', 'bar', 'baz']);
+    Map<String, Object> json = AnalysisServer.errorToJson(analysisError);
+
+    expect(json['message'],
+        equals("The element 'foo' is defined in the libraries 'bar' and 'baz'"));
+  }
+
+  static void errorToJson_noCorrection() {
+    Source source = new FileBasedSource.con1(new JavaFile('/foo.dart'));
+    CompileTimeErrorCode errorCode =
+        CompileTimeErrorCode.CONST_CONSTRUCTOR_WITH_NON_CONST_SUPER;
+    AnalysisError analysisError =
+        new AnalysisError.con2(source, 10, 5, errorCode, []);
+    Map<String, Object> json = AnalysisServer.errorToJson(analysisError);
+    expect(json, hasLength(5));
+
+    // TODO(paulberry): should be 'ffile:///foo.dart'.  See dartbug.com/18739
+    expect(json['source'], equals('102file:///foo.dart'));

[scheglov, 2014/04/23 19:20:56]

Yes, here is an example of an accidental complexity, and we test not quite what
we actually need.

[Paul Berry, 2014/04/23 20:36:26]

Agreed.  I also found out that this doesn't work on Windows builds (because
Windows includes a drive letter when canonicalizing the filename).  For the
moment I'm changing this to:

expect(json['source'], equals(source.encoding));

I'll add a task to my list to learn about mocks and revisit this test.

+
+    expect(json['errorCode'], equals(errorCode.ordinal));
+    expect(json['offset'], equals(analysisError.offset));
+    expect(json['length'], equals(analysisError.length));
+    expect(json['message'], equals(errorCode.message));
+  }
+
+  static void errorToJson_withCorrection() {
+    Source source = new FileBasedSource.con1(new JavaFile('/foo.dart'));
+
+    // TODO(paulberry): in principle we should test an error or hint that uses
+    // %s formatting in its correction string.  But no such errors or hints
+    // currently exist!

[Brian Wilkerson, 2014/04/24 15:44:05]

I'm not sure I know what you're referring to. Are you suggesting that we test
that the message text is created correctly?

[Paul Berry, 2014/04/24 16:19:26]

To clarify, I'm talking about the correction text here, not the message text.

I think it's the responsibility of a unit test on AnalysisError to test that the
correction text is created correctly (since it's the AnalysisError constructor
that creates the correction text by calling String.format(correctionTemplate,
arguments).

What I would like to verify in this test is that AnalysisServer.errorToJson()
gets the correction text in the proper way (using the getter
analysisError.correction).  If, instead, it used
analysisError.errorCode.correction, then it would be getting the correction
*template* rather than the properly constructed correction text.  Which means
that if the correction template contained a %s, that %s would leak up to the
editor, which wouldn't have enough information to know how to fill it in.

Unfortunately, I can't test that right now because there are no errors or hints
that use %s in their correction template.

So I guess my real question is: do we anticipate ever adding any errors or hints
that use %s in their correction template in the future?  If yes, then the TODO
item is a reminder to update this test once we've done so.  If no, then maybe we
should consider simplifying the AnalysisError class so that it doesn't call
String.format(correctionTemplate, arguments) anymore.

I suppose an alternative approach would be to rewrite this test using a mock
AnalysisError object.  But I tend to share your preference for using real
objects in unit tests when it's not too expensive to do so.

[Brian Wilkerson, 2014/04/24 16:47:45]

Yes, I expect that we will want to use %s in corrections.
There are no corrections that use it, at least in part, because there aren't
many corrections. Which, in turn, is because we didn't add corrections until
after most of the errors were already implemented and we didn't go back and add
the text.

So a third alternative would be to find an error that has no correction but for
which it would make sense to have a %s in the message, and add the correction
text to it.

+    HintCode errorCode = HintCode.MISSING_RETURN;
+
+    AnalysisError analysisError =
+        new AnalysisError.con2(source, 10, 5, errorCode, ['int']);
+    Map<String, Object> json = AnalysisServer.errorToJson(analysisError);
+    expect(json['correction'], equals(errorCode.correction));
+  }
+
   static Future performTask_whenNotRunning() {
     // If the server is shut down while there is analysis still pending,
     // performTask() should notice that the server is no longer running and