Issue 35560043: Make JUnit3TestNotRun fixes more local (resolves issue #204) (Closed)
|
Created:
10 years, 9 months ago by radoslaw.burny Modified:
10 years, 9 months ago Visibility:
Public. |
DescriptionMake JUnit3TestNotRun fixes more local (resolves issue #204)
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
MessagesTotal messages: 11
Sign in to reply to this message.
https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... File core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java (right): https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java:112: // and body. Any comments or special formatting in this space are removed. It would be nice if it was possible to preserve comments. In the case where the source of the node is available, what about something like the following? int start = decl.getStartPosition() + methodSource.toString().indexOf(name); int end = start + name.length(); fix = new SuggestedFix().replace(start, end, fixedName);
Sign in to reply to this message.
https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... File core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java (right): https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java:112: // and body. Any comments or special formatting in this space are removed. Note that it only changes part between return type and opening brace of method body. I haven't seen anyone place comments there; any "normally" placed comment (be it before method, or inside body) is preserved. Also, many other matchers (e.g. UnneededConditionalOperator) have similar behavior. I'd be careful not to branch on source availability, because only one path will be unit-tested then. Maybe I should remove my comment instead, since its probably misleading. What do you think? On 2013/12/01 05:04:59, cushon wrote: > It would be nice if it was possible to preserve comments. In the case where the > source of the node is available, what about something like the following? > > int start = decl.getStartPosition() + methodSource.toString().indexOf(name); > int end = start + name.length(); > fix = new SuggestedFix().replace(start, end, fixedName);
Sign in to reply to this message.
I agree that it's a weird place for comments and probably not worth worrying about. Using source.indexof(...) to locate the name comes with it's own set of problems, so I'm fine with the current approach. https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... File core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java (right): https://codereview.appspot.com/35560043/diff/1/core/src/main/java/com/google/... core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java:111: // We don't have start position for a method symbol, so we replace everything between result type nit: wrap at 100 chars.
Sign in to reply to this message.
On 2013/12/01 16:43:39, cushon wrote: Right, it can fail if method name appears in comment... Since Eddie is OOO, who do I set patches to? > nit: wrap at 100 chars. Done. Good catch, I forgot I set Vim's colorcolumn to 101, not 100.
Sign in to reply to this message.
https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... File core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java (right): https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java:81: // non-standard formatting - just to check that replacements are correctly located is there actually an assertion that replacements are correctly located? or is this a hint that we should manually verify it?
Sign in to reply to this message.
https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... File core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java (right): https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java:81: // non-standard formatting - just to check that replacements are correctly located On 2013/12/02 21:05:46, alexeagle wrote: > is there actually an assertion that replacements are correctly located? or is > this a hint that we should manually verify it? Perhaps the test should assert that the suggestion includes "void testMoreSpaces()". The line break in the follow test case makes it harder to verify the positioning. Maybe the replacement should be tweaked? - decl.restype.getStartPosition() + 5, decl.body.getStartPosition(), fixedName + "() "); + decl.restype.getStartPosition() + 4, decl.body.getStartPosition(), " " + fixedName + "() ");
Sign in to reply to this message.
https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... File core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java (right): https://codereview.appspot.com/35560043/diff/20001/core/src/test/resources/co... core/src/test/resources/com/google/errorprone/bugpatterns/JUnit3TestNotRunPositiveCases.java:81: // non-standard formatting - just to check that replacements are correctly located On 2013/12/02 21:22:25, cushon wrote: > On 2013/12/02 21:05:46, alexeagle wrote: > > is there actually an assertion that replacements are correctly located? or is > > this a hint that we should manually verify it? > > Perhaps the test should assert that the suggestion includes "void > Since the replacement is textual, not AST-based, I wanted a simple check to make sure the result is well-formed. I admit it wasn't a very good attempt; I've improved it according to Liam's suggestions. > The line break in the follow test case makes it harder to verify the > positioning. Maybe the replacement should be tweaked? > > - decl.restype.getStartPosition() + 5, decl.body.getStartPosition(), fixedName + > "() "); > + decl.restype.getStartPosition() + 4, decl.body.getStartPosition(), " " + > fixedName + "() "); Right, then we can even check for "void testMoreSpaces() {".
Sign in to reply to this message.
Liam, I didn't notice you have already applied patch #2. In case we want the improvement to the unit tests that was discussed above, I have uploaded it in patch #3 (as a diff against the version you applied).
Sign in to reply to this message.
On 2013/12/03 23:14:32, radoslaw.burny wrote: > Liam, I didn't notice you have already applied patch #2. In case we want the > improvement to the unit tests that was discussed above, I have uploaded it in > patch #3 (as a diff against the version you applied). Thanks, I will apply your latest patch.
Sign in to reply to this message.
LGTM On Tue, Dec 3, 2013 at 4:03 PM, <cushon@google.com> wrote: > On 2013/12/03 23:14:32, radoslaw.burny wrote: >> >> Liam, I didn't notice you have already applied patch #2. In case we > > want the >> >> improvement to the unit tests that was discussed above, I have > > uploaded it in >> >> patch #3 (as a diff against the version you applied). > > > Thanks, I will apply your latest patch. > > https://codereview.appspot.com/35560043/
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
