Serialize AnalysisErrors to JSON in the analysis server.

pkg/analysis_server/test/analysis_server_test.dart

 // Copyright (c) 2014, 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.
 
 library test.analysis_server;
 
 import 'dart:async';
 
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/source_io.dart';
@@ skipping 10 matching lines @@
   group('AnalysisServer', () {
     setUp(AnalysisServerTest.setUp);
     test('addContextToWorkQueue_twice',
         AnalysisServerTest.addContextToWorkQueue_twice);
     test('addContextToWorkQueue_whenNotRunning',
         AnalysisServerTest.addContextToWorkQueue_whenNotRunning);
     test('addContextToWorkQueue_whenRunning',
         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);
     test('unknownRequest', AnalysisServerTest.unknownRequest);
   });
 }
 
 class MockAnalysisContext_withPerformAnalysisTask extends MockAnalysisContext {
   List<AnalysisResult> results = [];
 
@@ skipping 41 matching lines @@
       expect(context.results, isEmpty);
       expect(channel.notificationsReceived, hasLength(2));
       expect(channel.notificationsReceived[0].event, equals('server.connected')
           );
       expect(channel.notificationsReceived[1].event, equals('context.errors'));
       expect(channel.notificationsReceived[1].params['source'], equals(
           '102file:///foo.dart')); // Issue 18739
       List<AnalysisError> errors =
           channel.notificationsReceived[1].params['errors'];
       expect(errors, hasLength(1));
-      expect(errors[0], equals(analysisError));
+      expect(errors[0], equals(AnalysisServer.errorToJson(analysisError)));
     });
   }
 
   static Future addContextToWorkQueue_twice() {
     // The context should only be asked to perform its analysis task once.
     MockAnalysisContext_withPerformAnalysisTask context =
         new MockAnalysisContext_withPerformAnalysisTask();
     server.addContextToWorkQueue(context);
     server.addContextToWorkQueue(context);
     context.results.add(new AnalysisResult(null, 0, null, 0));
@@ skipping 15 matching lines @@
   static Future echo() {
     server.handlers = [new EchoHandler()];
     var request = new Request('my22', 'echo');
     return channel.sendRequest(request)
         .then((Response response) {
           expect(response.id, equals('my22'));
           expect(response.error, isNull);
         });
   }
 
+  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
     // do no analysis.
     MockAnalysisContext context = new MockAnalysisContext();
     server.addContextToWorkQueue(context);
     server.running = false;
     // Pump the event queue to make sure the server doesn't try to do any
     // analysis.
     return pumpEventQueue();
@@ skipping 26 matching lines @@
   @override
   Response handleRequest(Request request) {
     if (request.method == 'echo') {
       var response = new Response(request.id);
       response.setResult('echo', true);
       return response;
     }
     return null;
   }
 }