Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
247893004:
Serialize AnalysisErrors to JSON in the analysis server. (Closed)
|
Created:
6 years, 8 months ago by Paul Berry Modified:
6 years, 8 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionSerialize AnalysisErrors to JSON in the analysis server.
R=scheglov@google.com
Committed: https://code.google.com/p/dart/source/detail?r=35329
Patch Set 1 #
Total comments: 14
Messages
Total messages: 7 (0 generated)
Note: all but one of the TODO's in this CL are things that I can't address without running the translator. I'll coordinate with Konstantin in the next few days to learn how to run it so I can take care of those items. As for the remaining TODO (in errorToJson_withCorrection()), I'm assuming that we're planning to make more use of corrections in the future, so we can beef up this test once that's happened. If I'm wrong about that let me know :)
lgtm https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/analysis_server.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:192: notification.setParameter(ERRORS_PARAM, new List.from(notice.errors.map( You could use .map(f).toList(). https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:202: // with dynamic is unnecessary. You could probably cast it to Enum. (errorCode as Enum).ordinal https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:206: 'errorCode': errorCode.ordinal, I don't think that using "ordinal" is valid at all. It changes every time when we regenerate Engine. I'd say it is valid only inside of a running Engine, but shouldn't be ever exposed outside. We need some stable presentation of an error. I think it should class name + error/enum name (not message). Still requires a change in ErrorCode though. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... File pkg/analysis_server/test/analysis_server_test.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:135: Source source = new FileBasedSource.con1(new JavaFile('/foo.dart')); 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. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:155: expect(json['source'], equals('102file:///foo.dart')); Yes, here is an example of an accidental complexity, and we test not quite what we actually need.
Message was sent while issue was closed.
Committed patchset #1 manually as r35329 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/analysis_server.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:192: notification.setParameter(ERRORS_PARAM, new List.from(notice.errors.map( On 2014/04/23 19:20:56, scheglov wrote: > You could use .map(f).toList(). Done. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:202: // with dynamic is unnecessary. On 2014/04/23 19:20:56, scheglov wrote: > You could probably cast it to Enum. > (errorCode as Enum).ordinal Done. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:206: 'errorCode': errorCode.ordinal, On 2014/04/23 19:20:56, scheglov wrote: > I don't think that using "ordinal" is valid at all. > It changes every time when we regenerate Engine. > I'd say it is valid only inside of a running Engine, but shouldn't be ever > exposed outside. > > We need some stable presentation of an error. > I think it should class name + error/enum name (not message). > Still requires a change in ErrorCode though. Good point. I've updated the TODO comment accordingly, and we can revisit in a future CL. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... File pkg/analysis_server/test/analysis_server_test.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:155: expect(json['source'], equals('102file:///foo.dart')); On 2014/04/23 19:20:56, scheglov wrote: > Yes, here is an example of an accidental complexity, and we test not quite what > we actually need. 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.
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/analysis_server.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/analysis_server.dart:206: 'errorCode': errorCode.ordinal, > We need some stable presentation of an error. There has been some discussion in the past about creating a list of error numbers that can be shared across all of the tools. I think that would be the better choice. Perhaps we should use this need as a forcing function. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... File pkg/analysis_server/test/analysis_server_test.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:135: Source source = new FileBasedSource.con1(new JavaFile('/foo.dart')); > Creating real objects makes tests less obvious. 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. https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:168: // currently exist! I'm not sure I know what you're referring to. Are you suggesting that we test that the message text is created correctly?
Message was sent while issue was closed.
https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... File pkg/analysis_server/test/analysis_server_test.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:168: // currently exist! On 2014/04/24 15:44:05, Brian Wilkerson wrote: > I'm not sure I know what you're referring to. Are you suggesting that we test > that the message text is created correctly? 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.
Message was sent while issue was closed.
https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... File pkg/analysis_server/test/analysis_server_test.dart (right): https://codereview.chromium.org/247893004/diff/1/pkg/analysis_server/test/ana... pkg/analysis_server/test/analysis_server_test.dart:168: // currently exist! > ... do we anticipate ever adding any errors or hints > that use %s in their correction template in the future? Yes, I expect that we will want to use %s in corrections. > Unfortunately, I can't test that right now because there are no > errors or hints that use %s in their correction template. > ... > I suppose an alternative approach would be to rewrite this test > using a mock AnalysisError object. 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
