Introduce RefasterRuleTestExtractor for documentation generation
#1317
Conversation
Stephan202
force-pushed
the
sschroevers/refaster-test-code-doc-generation
branch
from
a999cc6 to
c8c5107
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this comment.
Added some context. I also ported these changes to the website branch, where one can see how this all ties together.
| @@ -40,7 +40,7 @@ | |||
| @Immutable | |||
| @AutoService(Extractor.class) | |||
| @SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */) | |||
| public final class BugPatternTestExtractor implements Extractor<TestCases> { | |||
| public final class BugPatternTestExtractor implements Extractor<BugPatternTestCases> { | |||
There was a problem hiding this comment.
Renaming for disambiguation.
| private static String getFormattedSource(MethodTree method, VisitorState state) { | ||
| String source = SourceCode.treeToString(method, state); | ||
| int finalNewline = source.lastIndexOf(LINE_SEPARATOR); | ||
| if (finalNewline < 0) { |
There was a problem hiding this comment.
Pitest flags that this case isn't covered. There is actually an empty method test, but I suppose that source code does have a trailing newline (didn't check).
There was a problem hiding this comment.
Usually with checks like this -1 is returned if it is not found, it might be slightly more inuitive is we say == -1 instead of < 0, although it's the same, WDYT?
There was a problem hiding this comment.
Hmm, I generally prefer < 0, because any negative value isn't a valid index. We do this in a bunch of other places too:
$ git grep -i 'index.*< 0' | wc -l
8
There was a problem hiding this comment.
Rebased and reverted this change as discussed offline.
| } | ||
|
|
||
| int indentation = source.substring(finalNewline).lastIndexOf(' '); | ||
| String prefixToStrip = " ".repeat(indentation); |
There was a problem hiding this comment.
Pitest flags that the .repeat(indentation) logic isn't covered. This is because the test code is properly formatted; not an issue I suppose. (I did manually check that unformatted code doesn't break the extractor.)
| @JsonDeserialize(as = AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCase.class) | ||
| abstract static class RefasterTestCase { | ||
| static RefasterTestCase create(String name, String content) { | ||
| return new AutoValue_RefasterRuleCollectionTestExtractor_RefasterTestCase(name, content); |
There was a problem hiding this comment.
Pitest flags that these parameters can be swapped without failing a test. That's because we only validate that the type can be (de)serialized, without asserting the exact serialization format. That's a conscious choice. The final PR will improve coverage in this respect.
| @@ -26,6 +26,7 @@ ImmutableMultiset<ImmutableMultiset<Integer>> testEmptyImmutableMultiset() { | |||
| Stream.<Integer>empty().collect(toImmutableMultiset())); | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
Here and below: these annotations are now required, as the files are now compiled.
rickie
force-pushed
the
sschroevers/refaster-test-code-doc-generation
branch
from
c8c5107 to
2ee006d
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/refaster-test-code-doc-generation
branch
from
2ee006d to
3102a99
Compare
|
Rebased and added a commit to resolve a SonarCloud violation. Filed WIP PR #1320 related to this. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| private static String getFormattedSource(MethodTree method, VisitorState state) { | ||
| String source = SourceCode.treeToString(method, state); | ||
| int finalNewline = source.lastIndexOf(LINE_SEPARATOR); | ||
| if (finalNewline < 0) { |
There was a problem hiding this comment.
Usually with checks like this -1 is returned if it is not found, it might be slightly more inuitive is we say == -1 instead of < 0, although it's the same, WDYT?
| String className = tree.getSimpleName().toString(); | ||
|
|
||
| // XXX: Instead of throwing an error here, it'd be nicer to have a bug checker validate key | ||
| // aspects of `RefasterRuleCollectionRefasterTestCase` subtypes. |
There was a problem hiding this comment.
| // aspects of `RefasterRuleCollectionRefasterTestCase` subtypes. | |
| // aspects of `RefasterRuleCollectionTestCase` subtypes. |
Small typo?
There was a problem hiding this comment.
I renamed things a few times, and used "global search and replace". Likely a result of that 😅
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Tnx for the review @rickie! 🚀 |
mohamedsamehsalah
force-pushed
the
sschroevers/refaster-test-code-doc-generation
branch
from
b09f3da to
661f1a7
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| private static String getRuleCollectionName(ClassTree tree) { | ||
| String className = tree.getSimpleName().toString(); | ||
|
|
||
| // XXX: Instead of throwing an error here, it'd be nicer to have a bug checker validate key |
There was a problem hiding this comment.
Unless I am missing something, you can also argue that throwing an error provides faster feedback when developing / running tests locally.
There was a problem hiding this comment.
The bug checker would also run at compile time, but would provide a (much) better error message.
This new `Extractor` implementation collects Refaster example input and output code from rule collection tests. This change also introduces explicit compilation steps for the test code. As a side-effect this produces faster feedback in case of invalid input or output code.
Stephan202
force-pushed
the
sschroevers/refaster-test-code-doc-generation
branch
from
661f1a7 to
7a5ac15
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Stephan202
deleted the
sschroevers/refaster-test-code-doc-generation
branch
|
Synced the Largest remaining effort will be to write proper tests. |
Suggested commit message:
After this PR, approximately one more PR remains to port the remainder of the logic on the
websitebranch tomaster. After that we can configure the build such that any change onmasteris nearly immediately reflected on https://error-prone.picnic.tech.(The build will fail because of this issue; will try to address that separately.)