Introduce validator for Refaster template collections and tests #43

rickie · 2022-03-11T13:20:30Z

❗ ~~This branch is now on top of #113~~ ❗

New method: RefasterTemplateCollectionValidator#validate.

This can verify that all Refaster templates from a given template
collection are covered by at least one test and that the match rewrites
code in the correct test method.

Introduce the following modules:

refaster-runner
refaster-test-support

We can use this validation in the future to be more certain about new PRs, as this can guarantee, to some extent, that Refaster templates are properly tested.

Notes

The goal of the validator is to help a developer write Refaster templates and the tests for the template. We want every template to have a test and verify that Refaster matches in the place that we expect it to match.

rickie

Some context 😄.

rickie · 2022-03-11T13:22:59Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java

+   *
+   * @return A mapping from Refaster template names to {@link CodeTransformer}s.
+   */
+  public static ImmutableListMultimap<String, CodeTransformer> loadAllCodeTransformers() {


These methods are all coming from the RefasterCheck (link). These methods are almost exactly the same (see one change I'll point out in getClassPathResources()).

rickie · 2022-03-11T13:25:38Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/CodeTransformers.java

+
+  private static ImmutableSet<ResourceInfo> getClassPathResources() {
+    try {
+      return ClassPath.from(ClassLoader.getSystemClassLoader()).getResources();


Here we changed from RefasterCheck.class.getClassLoader() to the backup (link).

In the "old' situation we needed to get the resources (the *TemplatesTest{Input,Output}.java files) from the same module as RefasterCheck was in. Now that this is in a new module, we need the SystemClassLoader.

See #121: this change actually completely breaks the check (the tests just don't show that).

rickie · 2022-03-11T13:26:11Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterCheck.java

@@ -178,57 +169,4 @@ private static CodeTransformer createCompositeCodeTransformer(ErrorProneFlags fl
        .map(Map.Entry::getValue)
        .collect(toImmutableList());
  }
-
-  private static ImmutableListMultimap<String, CodeTransformer> loadAllCodeTransformers() {


So these methods moved to CodeTransformers.java

rickie · 2022-03-11T13:29:31Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+   *
+   * @param clazz The Refaster template collection under test.
+   */
+  public static void validateTemplateCollection(Class<?> clazz) {


For this method we are limited to what the API of BugCheckerRefactoringTestHelper offers us. We want to open a PR upstream to make this method public instead of private. In that case, we can improve this method because we can just pass the input and output file without retrieving the content first (which, for now, is an OK solution).

rickie · 2022-03-11T13:29:54Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+      name = "RefasterTestBugChecker",
+      summary = "Validate a Refaster template collection and its tests",
+      severity = ERROR)
+  public static final class RefasterTestBugChecker extends BugChecker


This is the check where the magic happens.

Badbond

Super great to see this, can only imagine that you're pretty proud of this. In the middle of reviewing, only where "the magic happens" to go. Got a few questions and suggestions already.

Memes be here

Btw, can we introduce a cognitive complexity metric for PRs as well? 😅 🧠
Normally not for being meme-y in PRs, but this PR deserves this one given the levels of meta-programming:

Badbond · 2022-03-18T11:52:03Z

refaster-test-support/src/main/java/tech/picnic/errorprone/refaster/test/package-info.java

@@ -0,0 +1,4 @@
+/** Utilities that support testing Refaster templates and validate the tests. */


IIUC, the latter is based on the former. I'd make this more explicit by stronger coupling. Also, the sentence reads a bit weirdly because the former has a gerund (if I know my grammar) while the latter has not. Suggesting:

Suggested change

/** Utilities that support testing Refaster templates and validate the tests. */

/** Utilities that support testing Refaster templates and allows for validating these tests. */

Badbond · 2022-03-18T13:22:13Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/RefasterCheck.java


-  @VisibleForTesting
  static final Supplier<ImmutableListMultimap<String, CodeTransformer>> ALL_CODE_TRANSFORMERS =


This can be private now (if you don't keep the tests). Or you can move it to CodeTransformers and keep it package private; this might be cleaner from a test-perspective (for the loadAllCodeTransformers test).

Good catch.

Or you can move it to CodeTransformers and keep it package private;

It is also used in the refaster-test-support so making it package-private is currently not an option.
Also considered moving the Supplier as is into the CodeTransformers class. Not sure if it would be improvement currently. Not a good idea or opinion about what would be the best approach for now.

Also considered moving the Supplier as is into the CodeTransformers class. Not sure if it would be improvement currently. Not a good idea or opinion about what would be the best approach for now.

IMHO we should do that, since now we load the classpath resources twice.

Badbond · 2022-03-18T13:50:26Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/package-info.java

+/** Classes used to find Refaster templates on the classpath and to apply them. */
+@com.google.errorprone.annotations.CheckReturnValue
+@javax.annotation.ParametersAreNonnullByDefault
+package tech.picnic.errorprone.refaster.runner;


Currently, I don't see much value in the module refaster-runner as it is only used by refaster-test-support. Do you envision functionality where runners are not only used in test context but also in a different context? If not, I'd suggest consolidating the two modules because of YAGNI.

Main reason why I tripped over this, is because this module doesn't have any tests anymore. Which I get, because the RefasterCheck and CodeTransformers logic is covered by RefasterCollectionTestUtil. But that class resides in a different module now. This link might become obscure once both modules keep growing causing it harder to debug test failures as a result of refaster-runner changes in the future. We could also keep some minimal level of testing in refaster-runner (see other comment).

We specifically choose to split refaster-runner and refaster-test-support.

refaster-runner and refaster-test-support are doing two completely different things. The refaster-runner runs during compilation and handles everything with regards to compiling the resources and the classpath. The test-support contains things that we can use during the test phase (so only in the context of compiling & running tests) and help with validation of templates and tests. So refaster-test-support can be used with <scope>test</scope>, which is different from refaster-runner.

We want people to be able to use & depend the refaster-runner without having to depend on / use things from the refaster-test-support.

We created a setup (module-wise) that we want to work towards and this is part of that.

This module doesn't have any tests anymore.

We decided to defer this for now. The tests in RefasterCheckTest are mostly based on the test resources from error-prone-contrib and is not what we actually want to test in refaster-runner. The replacement method highlighted in the other comment is part of what we test with the validateTemplateCollection (now we test more than previously).
So I agree that we should have more tests in refaster-runner but as I said, we want to defer that for now 😄.

Badbond · 2022-03-18T14:01:24Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterCheckTest.java

-   */
-  @MethodSource("templateGroupsUnderTest")
-  @ParameterizedTest
-  void replacement(String group) {


I feel we can keep some of these tests (like this one), possibly isolate them a bit more, and place them under refaster-runner. This would help in identifying erroneous changes to the classes in this package. We could create a few dummy templates to verify the RefasterCheck and CodeTransformers logic.

See other comment :)

Badbond · 2022-03-18T14:05:39Z

refaster-test-support/pom.xml

+        <dependency>
+            <groupId>${project.groupId}</groupId>
+            <artifactId>refaster-runner</artifactId>
+            <version>${project.version}</version>


I think generally we use root's <dependencyManagement> for managing internal modules' version (so together with next suggestion):

Suggested change

<version>${project.version}</version>

pom.xml

Badbond

A few more points of feedback. Only RefasterCollectionTestUtil to go, but did not review it yet as I expect a few changes based on the feedback. For other reviewers:

As discussed during the demo, @rickie will update RefasterCollectionTestUtil to report line numbers and columns instead of the current positions (which is the character count).

Badbond · 2022-03-21T10:36:28Z

...est/resources/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplatesTestInput.java

@@ -0,0 +1,9 @@
+package tech.picnic.errorprone.refaster.test;
+
+final class MatchInWrongMethodTemplatesTest implements RefasterTemplateTestCase {


Just a bit of a thought here. It might be nice, especially when we open-source, as a convention to add class-level Javadoc to these test cases. This way, once the test collection grows, we can easily with human-readable text read what these test input-output files are for. Now it is slightly hard to oversee with 3 files per test-case.
Don't have a strong opinion on it, nor for its location, so feel free to do with this feedback what you want 😅

Good one, will propose something. Let me know what you think :).

Can also be used to make the link between input and output clearer (at least for these resources).

Badbond · 2022-03-21T11:02:06Z

...sources/tech/picnic/errorprone/refaster/test/MissingTestAndWrongTestTemplatesTestOutput.java

+/* Did not encounter test in /tech.picnic.errorprone.refaster.test.MissingTestAndWrongTestTemplatesTestInput.java for the following template(s):
+- TemplateWithoutTest


/tech.picnic.errorprone.refaster.test.MissingTestAndWrongTestTemplatesTestInput.java feels a bit superfluous as this output file is connected to the input file. The test needs to be present in both (unless we extend Refaster to rename methods as well? 🤔). I'd consider removing the filename. I'd also consider indicating that we are missing a test method:

Suggested change

/* Did not encounter test in /tech.picnic.errorprone.refaster.test.MissingTestAndWrongTestTemplatesTestInput.java for the following template(s):

- TemplateWithoutTest

/* Did not encounter a test method for the following template(s):

- TemplateWithoutTest

or even indicating which method we expect per template:

Suggested change

/* Did not encounter test in /tech.picnic.errorprone.refaster.test.MissingTestAndWrongTestTemplatesTestInput.java for the following template(s):

- TemplateWithoutTest

/* Did not encounter test methods for the following template(s):

- `testTemplateWithoutTest` for template `TemplateWithoutTest`.

See also next comment related to this.

as this output file is connected to the input file.

Yeah true, what I do like about mentioning the file, is that this points the user exactly to where they need to go and makes this connection clear. If a user is not 100% familiar with this setup it could help. OTOH I can see why removing it would also be clear enough 🤔.

I like the suggestions of making some messages more specific. The concern I have for some of them, is that strictly speaking they are not always "correct". Based on the implementation we can not always draw the conclusion like suggested here. Let me give an example:

/* Did not encounter test methods for the following template(s):

testTemplateWithoutTest for template TemplateWithoutTest.

We don't verify this currently (again, strictly speaking). First, we use the classpath to collect all names of the Refaster templates of a collection. Next, we get a list of all the matches and compare this list to the list from the previous step. We can easily see that a template does not occur in the list of matches and we print out this message.

We could do something like (to be improved):
Did not encounter test code that matched the following template(s): [....] Please add a method named 'test' + the name of the template.

Whatever we decide, it is clear that we can improve the way in which we guide the developer based on the (error) messages 😄.

The reason for specifically mentioning MissingTestAndWrongTestTemplatesTestInput is that it helps the developer do the first step, which is adding something in the input file. If someone already added a method in the output file but forgot the input file, there will be a different error message (as this is already handled by the EP test framework). It will look something like:
(Btw, this also holds for the case where something is something in the input but not in the output.)

expected: boolean testTemplateWithoutTest() { return "foo".length() == 0; } but was: <not that method>

So once the developer added something to the input file the rest of the framework will "take care of the rest" and guide the developer further. WDYT?

Nonetheless, the full package name is not that useful so we can omit that for sure.

On second thought, you are right. We should keep the name of the class in there. Looking at the test case, it isn't that interesting. But as this is console output, might be less visible that this is the problematic file indeed. Not keeping the package in there SGTM.

We don't verify this currently (again, strictly speaking). First, we use the classpath to collect all names of the Refaster templates of a collection. Next, we get a list of all the matches and compare this list to the list from the previous step. We can easily see that a template does not occur in the list of matches and we print out this message.

Regarding that, the implementation is indeed something I propose to change (see other comment). What I would do is separate this into two checks, for which the first is:

"Use the classpath to collect all names of the Refaster templates of a collection"

Get all the methods defined in the input test file.

Require all templates to have a method testTemplateX. If not, print this Did not encounter test method for the following template(s) message.

Effectively, this is what currently is already enforced. So I thought it would be nicer to make this a more explicit initial simple check before we start looking at 'what templates have been matched in which test method?'.

Then for the second check, we can verify the following to produce the The following matches occurred in method ... message:

Does my TemplateX apply changes to the testTemplateX method? If not -> message.

Do templates other than TemplateX match on testTemplateX method? If so -> message.

I think this improves the developer experience and is quite a nice setup as the messages are more encapsulated/tied to what is verified.

So I thought it would be nicer to make this a more explicit initial simple check before we start looking at 'what templates have been matched in which test method?'.

To give context to this thought of mine, see this test testcase for this template I wrote on my branch which has the suggestion applied for this review. Here it would've been nice to also have a message stating that we Did not encounter test method for the following template(s): StringEquals (even though it matched unexpectedly in a test method targeted towards testing a different template). Of course, after fixing this unexpected match, another run of the tests would fail with this 'Did not encounter ...' message. So all in all, happy to keep current implementation too 🙂

Badbond · 2022-03-21T11:16:14Z

...st/resources/tech/picnic/errorprone/refaster/test/MatchInWrongMethodTemplatesTestOutput.java

+/* The following matches occurred in method `testWrongName` (position: [131,233]):
+- Template `StringIsEmpty` matched on position: [161..177)
+- Template `StringIsEmpty` matched on position: [183..199)
+- Template `StringIsEmpty` matched on position: [212..228)
+*/


Having a bit of trouble giving feedback on this to come up with a suggestion. My initial thought was that this is a bit generic non-actionable feedback. It might be nice to point out what is wrong or steer the devs in a certain direction instead.

In my mind this type of feedback currently can happen in three cases (given the fact that we match templates on method name in tests):

A template unexpectedly matches on a test method meant for another template. Think for example about a new introduced template that matches a subset of the cases for an already existing template. Then the new narrower-scoped template could change an existing test targeted towards the broader-scoped template. The developer should resolve the overlap in templates, maybe.

A template matches a test method for auxiliary use (e.g. helper methods). This should just be fixed in the XInput file.

The method name is wrong, as tested here. It should be renamed.

The third case is not distinguishable from the second apart from our own interpretation of the test code. Therefore, I'd prefer also a Did not encounter test method for the following template(s) message (see the other comment). I think we can achieve this by simply looking at (the lack of) methods defined instead of templatesWithoutMatch.

For the other two cases, I think this message is indeed nice to have a message indicating that we unexpectedly found a match in the code for a specific template. Not sure about any indication of why we did not expect it. Maybe by just adding the word unexpectedly, but might be nice to give further reason. WDYT?

Oh nice, I really like the word unexpectedly.

The developer should resolve the overlap in templates, maybe.

Yes for sure, otherwise the self-check will not succeed 😉.

Good observations, thought quite some time about it now.

If we would change the message to the next line, I think we would cover all cases (and be clear with regards of guiding the developer):
Template 'StringIsEmpty' matched on line 15, expected to match in a method named 'testStringIsEmpty'.

(Combined reply on the new comment)

I think this improves the developer experience and is quite a nice setup as the messages are more encapsulated/tied to what is verified.

This is true, will think about it and discuss it.

Currently, the main reason for adding these checks is to help ourselves by making sure that the Refaster templates are tested and tested in the right place.
Additionally, we want to make sure that the tests of the existing body of templates are correct.
Finally, this will help us with doing reviews of new PRs in the future easier (also for contributors outside of Picnic).

So for now, I want to argue that with improving the error messages the current implementation would be sufficient for our needs.
Based on the new error messages (to be pushed) all cases are sufficiently covered IMHO.
If we later feel that it is necessary to change or improve the checks, we can always do that.

For example, in the case that we get many contributions or developers find the development confusing, we can always choose to add this distinction in the error message.

rickie · 2022-03-21T14:11:57Z

Based on the content of the PR and the feedback (also from the demo) I feel that it could help to point out the following (also to explain to other reviewers) 😄.

We primarily rely on the test framework provided by Error Prone. In RefasterCollectionTestUtil.java we use the BugCheckerRefactoringTestHelper which is located in the error_prone_test_helpers module. The validation we do here is on top of the framework to point out some extra things. This means that there is already a complete test framework available that guides the developer.

The validation that we add is where the validation of the framework "stops" but where we want to be sure that (for example) specific cases are handled / tested. This means that what we validate could, in some cases, be quite specific.

mussatto

Very cool change!

I have very little knowledge about how this works and spent most of the time trying to understand the code, so I am finding very difficult to give useful feedback :)

I don't know if it would be applicable to this type of class, specially given the stuff from javac - but is it possible to split the classes to unit test them?

If we are planning on open sourcing this, should we also add a README file describing how to use this new feature?

very nice tests btw!

mussatto · 2022-03-21T21:21:48Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+      name = "RefasterTestBugChecker",
+      summary = "Validate a Refaster template collection and its tests",
+      severity = ERROR)
+  public static final class RefasterTestBugChecker extends BugChecker


is there any specific reason to not split this into a separate file?

We use this setup in refaster-support currently. However, there we have small checks that are in the same class. This checker is now pretty big and deserves it's own class, so will move it!

mussatto · 2022-03-22T09:50:34Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+      return fqcn.substring(fqcn.lastIndexOf('.') + 1);
+    }
+
+    private class ValidateMatchesInMethodsScanner extends TreeScanner<Void, VisitorState> {


same for this class :)

This one I wouldn't move to it's own class. In Error Prone (so in the upstream repo) this is how Visitors and Scanners are often used. They are in most cases specific to the BugChecker and almost always live in the class of the check. I propose we do the same here 😄.

mussatto · 2022-03-22T10:47:35Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+
+      JCCompilationUnit compilationUnit = (JCCompilationUnit) tree;
+      ImmutableRangeMap<Integer, String> matchesRangeMap =
+          buildRangeMapForMatches(matches, compilationUnit.endPositions);


I saw that the compilationUnit.endPositions is initialised with null, does this happen in real scenarios?
I don't know how this works IRL :)

Ah good catch! This is something that I had to work with during my thesis.

Looking at the usage of this field, we see (among others) the following classes that can put something in this field:

JavaCompiler.java

JavacTrees.java

JavacParser.java

This is something that the compiler and parser will handle for us. So we don't need to worry about it being null 😄.

Badbond

Went through it all. Really cool work! Got a few more comments.
~~As said in last comments, have local changes that I can push for them if you want.~~ Pushed them to a seperate branch: see diff.

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

Badbond · 2022-03-23T11:18:54Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+   *
+   * @param clazz The Refaster template collection under test.
+   */
+  public static void validateTemplateCollection(Class<?> clazz) {


Just a suggestion, comes down to style preference maybe. Generally, I prefer to extract this form of variable initialization to its own method, if sensible. Here, I am doubtful but leaning towards extracting it to deduplicate receiving content for two files as well. What it could look like if we do so:

public static void validateTemplateCollection(Class<?> clazz) { String className = clazz.getSimpleName(); BugCheckerRefactoringTestHelper.newInstance( RefasterTestBugChecker.class, RefasterCollectionTestUtil.class) .setArgs(ImmutableList.of("-XepOpt:RefasterTestChecker:TemplateCollection=" + className)) .addInputLines( clazz.getName() + "TestInput.java", getContentOfResource(clazz, className + "TestInput.java")) .addOutputLines( clazz.getName() + "TestOutput.java", getContentOfResource(clazz, className + "TestOutput.java")) .doTest(TEXT_MATCH); } private static String getContentOfResource(Class<?> clazz, String resource) { JavaFileObject object = FileObjects.forResource(clazz, resource); try { return object.getCharContent(false).toString(); } catch (IOException e) { throw new IllegalStateException("Can't retrieve content for file " + resource, e); } }

Note, changed the exception message as we can now just point to the problematic file instead.

For context, ideally, we want to propose this simple change to google/error-prone. It would make this method a lot nicer because we can just pass the JavaFileObjects directly. We can significantly improve this method if they accept that change upstream.

Having said that, I like the suggestion 😄 although the instantiation of the BugChecker is a bit big in that case 🤔 . Not sure how to improve, nice suggestion 🚀 !

Badbond · 2022-03-23T12:51:45Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+    private ImmutableSet<String> getTemplateNamesWithoutMatch(List<Description> matches) {
+      ImmutableSet<String> templateNamesOfMatches =
+          matches.stream()
+              .map(description -> description.checkName)
+              .map(RefasterTestBugChecker::getNameFromFQCN)
+              .collect(toImmutableSet());
+
+      return Sets.difference(templateNamesFromClassPath, templateNamesOfMatches).immutableCopy();
+    }


We can reuse the matchesRangeMap as this already contains the FQCN of the matched templates. You could also decide to inline this, but that does not make it much more readable. Suggestion:

Suggested change

private ImmutableSet<String> getTemplateNamesWithoutMatch(List<Description> matches) {

ImmutableSet<String> templateNamesOfMatches =

matches.stream()

.map(description -> description.checkName)

.map(RefasterTestBugChecker::getNameFromFQCN)

.collect(toImmutableSet());

return Sets.difference(templateNamesFromClassPath, templateNamesOfMatches).immutableCopy();

}

private ImmutableSet<String> getTemplateNamesWithoutMatch(

ImmutableRangeMap<Integer, String> matchesRangeMap) {

return Sets.difference(

templateNamesFromClassPath,

ImmutableSet.copyOf(matchesRangeMap.asMapOfRanges().values()))

.immutableCopy();

}

Nice idea, I tweaked it a bit. WDYT of passing a ImmutableCollection. It feels a bit too specific to pass the full ImmutableRangeMap, what if we make it a ImmutableCollection? We can say "hey method here is a collection, what are templates without match"?

(Pushing suggestion once I went over all feedback)

Badbond · 2022-03-23T13:05:54Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+    private void appendCommentToCompilationUnit(String message, String list, VisitorState state) {
+      String comment = String.format("\n/* %s:%s*/", message, list);
+      CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();
+      state.reportMatch(
+          describeMatch(compilationUnit, SuggestedFix.postfixWith(compilationUnit, comment)));
+    }


I think it would look cleaner to do the following. This also causes more presentation logic to be isolated in here.

Make parameter list a Stream<String> instead of a formatted String.

Call this stream conflicts or something, list is a bit too non-descriptive to my taste.

Collect in here and inline the collector as this is the only usage.

Don't have prefix and suffix in collector as we should always have at least one element and we can put it in the main String.format instead.

Resulting in:

Suggested change

private void appendCommentToCompilationUnit(String message, String list, VisitorState state) {

String comment = String.format("\n/* %s:%s*/", message, list);

CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();

state.reportMatch(

describeMatch(compilationUnit, SuggestedFix.postfixWith(compilationUnit, comment)));

}

private void appendCommentToCompilationUnit(

String message, Stream<String> conflicts, VisitorState state) {

String comment =

String.format("\n/* %s:\n- %s\n*/", message, conflicts.collect(joining("\n- ")));

CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();

state.reportMatch(

describeMatch(compilationUnit, SuggestedFix.postfixWith(compilationUnit, comment)));

}

Feel free to adopt a subset of these suggestions 😄

The reason for having the collector like this is that (IMO) it makes the first parameter of the String.format a bit easier to read and understand. Probably a preference thingy, we could see what other reviewers think 👀.

Agree with all the other feedback! Will push something.

Badbond · 2022-03-23T13:41:11Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+        boolean correctTemplatesMatchedInMethod =
+            matchesInCurrentMethod.asMapOfRanges().values().stream().allMatch(methodName::equals);
+        if (!correctTemplatesMatchedInMethod) {
+          appendCommentToCompilationUnit(


We could pass in tree here to place these suggestions postfix the violating method instead of postfix the end of the compilation unit (for the other usage we can just pass compilationUnit). This might make it easier for huge Input/Output files to see where you need to change things.

So that would result in appendCommentToCompilationUnit:

state.reportMatch( describeMatch( state.getPath().getCompilationUnit(), SuggestedFix.postfixWith(unit, comment)));

Discussed this briefly during the stand-up. Very good idea! Based on how the output is generated (and taking into account long test methods), we could also prefix the method with the comment. As a result, we also see the method name instead of a few test cases which can be harder to track. Based on the provided diff with small tweaks we get this as a warning if we run the test (which looks awesome 🚀 !) :

+ /* The following matches unexpectedly occurred in method `testWrongName`: + - Template `StringIsEmpty` matches on line 6, while it should match in a method named `testStringIsEmpty`. + - Template `StringIsEmpty` matches on line 7, while it should match in a method named `testStringIsEmpty`. + - Template `StringIsEmpty` matches on line 8, while it should match in a method named `testStringIsEmpty`. + */ boolean testWrongName() { "foo".isEmpty(); "bar".isEmpty();

In the output files we add these comments to validate the RefasterCollectionTestUtil "extension". It is important to note that the shown line numbers won't match the correct line number in the output files that are used to test the extension (the resources that live in refaster-test-support under test/resources).
However, we are not supposed to write these comments as a developer because we need to fix this warning and not add them in the output file to let the build pass. For that reason the line numbers look incorrect, but what we are now validating is not someone should do. I'm not sure if I'm explaining it clearly, let me know if it is still unclear :D.

Nice! Agreed. Prefix looks even better 🚀.

Haha, yes clear. To clarify for other reviewers maybe I'll try to approach it from a different point of view. Important to note is that this output will be shown in console output when running tests. Therefore the developer will be searching for the file and the line numbers will match. For the purpose of testing RefasterCollectionTestUtil, we verify that it does match this output, hence why in this case the line numbers appear off by the added comments.

@rickie it might perhaps be nice to document this with one or two sentences in RefasterCollectionTestUtilTest.java.

Good one, pushed a suggestion. Suggestions welcome of course 😉.

Badbond · 2022-03-23T13:59:43Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterCollectionTestUtil.java

+              matchesRangeMap.asMapOfRanges().entrySet().stream()
+                  .map(


We should filter here those that are correct (~~I have local changes for all suggestions including a testcase for this, let me know if you want me to push it or send it to you~~ Pushed them to a seperate branch: see diff):

Suggested change

matchesRangeMap.asMapOfRanges().entrySet().stream()

.map(

matchesRangeMap.asMapOfRanges().entrySet().stream()

.filter(e -> !e.getValue().equals(methodName))

.map(

Good catch! Used the diff to add an extra test case 🥳.

rickie · 2022-03-28T14:30:08Z

Thanks for all the suggestions! Addressed all the feedback by now. Will work on a README @mussatto !

Decided to push the changes already to make the code ready for the next round of feedback 😉.

rickie · 2022-04-04T13:13:58Z

@mussatto @Badbond in the last PR I incorporated all feedback. Also added the package structure.

This means the code / readme is ready for a new round of feedback 😉.

Badbond

Nice documentation 👍 That will help in onboarding.

Badbond · 2022-04-04T14:17:43Z

refaster-test-support/README.md

+template collection. This test utility applies the Refaster templates in the collection to a provided input file, and
+expects the result to exactly match the contents of a provided output file.


This test utility applies the Refaster templates in the collection to a provided input file, and
expects the result to exactly match the contents of a provided output file.

Is a bit too similar to the following the paragraph before:

The Refaster templates from the collection are applied on the input file and should exactly match the content of
the provided output file.

I'm not sure if this repetition is bad. The first part of the paragraph summarizes all the following content. It seemed nice to give the reader a summary of what it will read. Perhaps it is not that useful.

Badbond · 2022-04-04T14:19:33Z

refaster-test-support/README.md

+  template collection must match the naming convention `<TemplateCollectionClassName>Templates.java`.
+- An input file matching the naming convention `<TemplateCollectionClassName>TemplatesTestInput.java` is added for every
+  template collection.
+- An output file matching the naming convention `<TemplateCollectionClassName>TemplatesTestOutput.java`


Technically, <TemplateCollectionClassName>Templates.java is the template collection's class name, right? 🤔

Suggested change

template collection must match the naming convention `<TemplateCollectionClassName>Templates.java`.

- An input file matching the naming convention `<TemplateCollectionClassName>TemplatesTestInput.java` is added for every

template collection.

- An output file matching the naming convention `<TemplateCollectionClassName>TemplatesTestOutput.java`

template collection must match the naming convention `<TemplateCollectionName>Templates.java`.

- An input file matching the naming convention `<TemplateCollectionName>TemplatesTestInput.java` is added for every

template collection.

- An output file matching the naming convention `<TemplateCollectionName>TemplatesTestOutput.java`

Badbond · 2022-04-04T14:20:38Z

refaster-test-support/README.md

+  template collection.
+- An output file matching the naming convention `<TemplateCollectionClassName>TemplatesTestOutput.java`
+  file is added for every template collection.
+- For each Refaster template in the collection, the input and output file contain a method. The name of the method is


Suggested change

- For each Refaster template in the collection, the input and output file contain a method. The name of the method is

- For each Refaster template in the collection, the input and output file must contain a method. The name of the method is

Badbond · 2022-04-04T14:22:05Z

refaster-test-support/README.md

+- The method contains at least one expression that matches the `@BeforeTemplate`. As a result, the output file contains
+  the same method with an updated expression, matching the content of the `@AfterTemplate`.


Maybe also mention that no other templates must match? Could not find the right words for a suggestion 🤔

Good one, will push a proposal.

Badbond · 2022-04-04T14:23:05Z

refaster-test-support/README.md

+
+As a result from these tests, unexpected output will be shown in the console.
+
+An example of a folder structure for such a setup:


Preference thing, a bit nicer to read IMO.

Suggested change

An example of a folder structure for such a setup:

An example of a folder structure for such a setup is as follows:

Badbond · 2022-04-04T14:29:06Z

refaster-test-support/README.md

+        └── ExampleTemplatesTestInput.java -- Contains a class named `ExampleTemplatesTest`
+        └── ExampleTemplatesTestOutput.java -- Contains a class named `ExampleTemplatesTest`


Two remarks:

For consistency, end the comments with a dot (we currently do that for the RefasterCollectionTest line).

Might be nice to give an example of the methods too.

Did not pay attention to formatting for line column width, so maybe this suggestion isn't as 'nice' to apply.

Suggested change

└── ExampleTemplatesTestInput.java -- Contains a class named `ExampleTemplatesTest`

└── ExampleTemplatesTestOutput.java -- Contains a class named `ExampleTemplatesTest`

└── ExampleTemplatesTestInput.java -- Contains a class named `ExampleTemplatesTest` and two methods named `testExample1Template` and `testExample2Template`.

└── ExampleTemplatesTestOutput.java -- Contains a class named `ExampleTemplatesTest` and two methods named `testExample1Template` and `testExample2Template`.

Good one, makes it really clear for the reader. W.r.t. linewrapping, I'm not sure how to proceed here, in general I mean. I think the max of 80 is used in other repositories. I couldn't find a nice guideline for this. That's why I used the current formatting I have in IntelliJ... Ideas on this are welcome 😉.

(Made sure to wrap at 79 as that is our convention for README.mds).

Badbond

Pushed two minor tweaks. LGTM 🚀

mussatto

looks awesome to me :)

mussatto · 2022-04-04T19:23:48Z

refaster-test-support/README.md

+
+An example of a folder structure for such a setup is as follows:
+
+```


noice example :)

mussatto

looks awesome to me :)

rickie · 2022-04-16T11:46:36Z

Rebased.

Stephan202

Added a small commit. I noticed that the Refaster templates are now no longer tested (notice the shorter build time). And the self-check build also isn't slower, which makes me suspect that the Refaster templates aren't executed at all. That requires some investigation.

Stephan202 · 2022-05-22T15:16:26Z

pom.xml

        <module>refaster-resource-compiler</module>
+        <module>refaster-runner</module>


For consistency, should we s/refaster-resource-compiler/refaster-compiler/? 🤔

Good one, created #110!

Stephan202 · 2022-08-04T16:28:19Z

Added a commit with a Javadoc tweak. Only open point from my side is a review of the README.

rickie · 2022-08-04T21:38:31Z

Only open point from my side is a review of the README.

Haha, looking forward to the results 👀.

Stephan202 · 2022-08-07T16:57:48Z

There were complex conflicts, so I solved them using a merge commit. Also added another tiny cleanup commit. README review is still TBD.

Badbond

Hehe I see @Stephan202 also is taking a look at the README.md now 😄 Flushing comments I had. Will commit those on paragraph @Stephan202 did not yet touch and reread the changed paragraphs while taking earlier comments in mind.

Badbond · 2022-08-09T12:43:19Z

refaster-test-support/README.md

+
+## What does this module do?
+
+These utilities allow validating the rewrites (or their absence) performed by


I think the following is more grammatically correct/reads nicer:

Suggested change

These utilities allow validating the rewrites (or their absence) performed by

These utilities allow for validating the rewrites (or absence thereof) as performed by

Badbond · 2022-08-09T12:44:53Z

refaster-test-support/README.md

+it matches and transforms code as intended. If a Refaster template is not
+covered by a test, if it influences unrelated test code, or if the associated
+test doesn't follow certain established standards, then this irregularity will
+be reported, and the associated template collection test will fail. This way


Committed

Suggested change

be reported, and the associated template collection test will fail. This way

be reported, and the associated template collection test will fail. This way,

Badbond · 2022-08-09T12:47:27Z

refaster-test-support/README.md

+A class that contains one or more Refaster templates is called a Refaster
+template collection. In a nutshell, to test a Refaster template collection
+class using `RefasterTemplateCollection`, one should create suitably named
+input and output files. The collection's Refaster templates are applied to the


Maybe a bit too explicit, but nowhere in this paragraph we have mentioned what is in these input or output files.

Suggested change

input and output files. The collection's Refaster templates are applied to the

input and output source code files. The collection's Refaster templates are applied to the

Will leave this one to @Stephan202 .

Badbond · 2022-08-09T12:48:47Z

refaster-test-support/README.md

+templates in the collection to a provided input file, and expects the result
+to exactly match the contents of a provided output file.
+
+To adopt this setup, the following requirements have to be met:


Committed

Suggested change

To adopt this setup, the following requirements have to be met:

To adopt this setup, the following requirements must be met:

Badbond · 2022-08-09T12:50:24Z

refaster-test-support/README.md

+- Create a class with a (parameterized) test method that invokes
+  `RefasterTemplateCollection#validate` and passes the collection(s) to
+  validate. The Refaster template collection must match the naming convention
+  `<TemplateCollectionName>Templates.java`.


Committed and reflowed

A few suggestions on this line. This suggestion makes the narrative more in line with 'requirements must be met' and the other items below w.r.t. 'is added'.

Suggested change

- Create a class with a (parameterized) test method that invokes

`RefasterTemplateCollection#validate` and passes the collection(s) to

validate. The Refaster template collection must match the naming convention

`<TemplateCollectionName>Templates.java`.

- A class with a (parameterized) test method that invokes

`RefasterTemplateCollection#validate` is created which provides the collection(s) to

validate. The Refaster template collection must match the naming convention

`<TemplateCollectionName>Templates.java`.

Badbond · 2022-08-09T12:55:31Z

refaster-test-support/README.md

+  template collection.
+- For every Refaster template in the collection, the input and output file must
+  contain a method. The name of the method is equal to the name of the Refaster
+  template prefixed with `test` (e.g. `test<RefasterTemplateClassName>`).


Committed

Suggested change

template prefixed with `test` (e.g. `test<RefasterTemplateClassName>`).

template prefixed with `test` (e.g. `test<RefasterTemplateInnerClassName>`).

Maybe to avoid confusion with the template collection class?

TBH, to me, this one is not a clear improvement.

Feel free to revert or change to something else if you don't agree. I can understand where you're coming from.

Will do :). I like the suggestions in general 😄 👍🏻.

Reverted, but was mainly giving my opinion to see what other reviewers think. That's why I commented in the first place.

Badbond · 2022-08-09T12:56:23Z

refaster-test-support/README.md

+  contain a method. The name of the method is equal to the name of the Refaster
+  template prefixed with `test` (e.g. `test<RefasterTemplateClassName>`).
+- The method contains at least one expression that matches the
+  `@BeforeTemplate` of one specific Refaster template. As a result, the output


Committed and reflowed

Suggested change

`@BeforeTemplate` of one specific Refaster template. As a result, the output

`@BeforeTemplate` of the corresponding Refaster template. As a result, the output

The test enforces the exact refaster template to match right? Not just any refaster template in the collection?

Badbond

Currently looking at RefasterTemplateCollection.java, flushing comments already.

Badbond · 2022-08-09T13:49:30Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/package-info.java

@@ -0,0 +1,4 @@
+/** Classes used to locate and apply compiled Refaster templates on. */


Not sure if we want to have an intentional 1-to-1 mapping between the pom.xml's description and this package-info.java (we did so for refaster-test-support), but this one is currently not the same. I must say, I prefer the pom.xml's description over this and would be in favor of replacing it.

Badbond · 2022-08-09T13:56:14Z

...sources/tech/picnic/errorprone/refaster/test/MissingTestAndWrongTestTemplatesTestOutput.java

+  /*
+   *  ERROR: The following matches unexpectedly occurred in method `testWrongName`:
+   *  - Template `StringIsEmpty` matches on line 6, while it should match in a method named `testStringIsEmpty`.
+   *  - Template `StringIsEmpty` matches on line 7, while it should match in a method named `testStringIsEmpty`.
+   *  - Template `StringIsEmpty` matches on line 8, while it should match in a method named `testStringIsEmpty`.
+   */
+  boolean testWrongName() {
+    "foo".isEmpty();
+    "bar".isEmpty();
+    return "baz".isEmpty();
+  }
+}


This is already tested with MatchInWrongMethodTemplatesTestOutput.java, isn't it? 🤔

Strictly speaking yes, but it is to have something here as well. Also to see if it behaves correctly if the two situations are combined. So is the error still correct if there is a test missing and this method has some wrong entries.

So I'd suggest to leave it as is.

Ah makes sense. Might be nice to reflect that in the main template collection's JavaDoc, but I agree, asserting the combination is nice 👍

Badbond · 2022-08-09T14:00:19Z

refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/ValidTemplates.java

+import javax.annotation.Nullable;
+
+/** Refaster template collection to validate that having no violations works as expected. */
+final class ValidTemplates {


Would it be nice to add a template containing a Refaster.anyOf in its before? I know we won't ascertain all branches to be fulfilled, but at least knowing the collection is capable of reading one, might be nice.

Leaving this decision to @rickie.

(I'm fine deferring it because the checker currently doesn't do anything with Refaster.anyOf, so indeed strictly speaking it doesn't add much.)

I'd also like to postpone this 😄.

(Good point though 😉)

Badbond · 2022-08-09T14:09:10Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java

+/**
+ * A {@link BugChecker} that applies a Refaster template collection to an associated test input file
+ * by delegating to the {@link Refaster} checker, and subsequently validates that each template
+ * modifies exactly one distinct method, as indicated by each method's name.


The name of the template enforces the name of the test method, so this would be more correct, right?
Also, missing some documentation that the input method should be prefixed with test.

Having a hard time to come up with a suggestion (especially as it is all one line now and my brain is a bit depleted), but here we go:

Suggested change

* modifies exactly one distinct method, as indicated by each method's name.

* modifies exactly one distinct method, as identified by the corresponding template's name and prefixed with `test`.

I think we should start a new paragraph ;). Will propose something.

Badbond

Amazing PR and definitely a fun one to review. Also since my last review, the code became even nicer and easily readable. Thanks a lot for all the hard effort @rickie and @Stephan202 🙏 Definitely one to be proud of. Unfortunately don't have the time to post a suggested commit message, but I trust you'll get a good one out of it. Enjoy the first release, and make sure to celebrate it with the entire team!

Stephan202

Added a commit; README review is still TBD.

Stephan202 · 2022-08-09T17:06:18Z

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/package-info.java

@@ -0,0 +1,4 @@
+/** Classes used to locate and apply compiled Refaster templates on. */


Stephan202 · 2022-08-09T17:07:39Z

refaster-test-support/src/test/java/tech/picnic/errorprone/refaster/test/ValidTemplates.java

+import javax.annotation.Nullable;
+
+/** Refaster template collection to validate that having no violations works as expected. */
+final class ValidTemplates {


Leaving this decision to @rickie.

(I'm fine deferring it because the checker currently doesn't do anything with Refaster.anyOf, so indeed strictly speaking it doesn't add much.)

Stephan202 · 2022-08-09T17:16:13Z

...t-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterTemplateCollection.java

+/**
+ * A {@link BugChecker} that applies a Refaster template collection to an associated test input file
+ * by delegating to the {@link Refaster} checker, and subsequently validates that each template
+ * modifies exactly one distinct method, as indicated by each method's name.


I think we should start a new paragraph ;). Will propose something.

rickie · 2022-08-09T21:13:57Z

Bit late for a response, but the latest changes LGTM!

Stephan202 · 2022-08-10T07:26:56Z

Added a commit; PTAL.

rickie · 2022-08-10T07:33:20Z

Nice improvements. Looks really good 👍🏻.

Stephan202 · 2022-08-10T08:25:40Z

Added one more commit. @rickie do we already have a suggested commit message?

rickie · 2022-08-10T08:26:03Z

I'll write one now! @Stephan202

rickie · 2022-08-10T08:36:06Z

Suggested commit message:

Introduce utilities to validate Refaster template collections (#43)

The `Refaster` check is now located in the new `refaster-runner` Maven module.

The new `refaster-test-support` module exposes
`RefasterTemplateCollection#validate`, which for a given template collection
validates:
- That there exist corresponding `*Test{Input,Output}.java` files.
- That each template is excercised by precisely one method in these files.

Stephan202 · 2022-08-10T10:23:49Z

^ Tweaked it. Have a meeting now; will do another pass later.

rickie commented Mar 11, 2022

View reviewed changes

rickie requested review from Stephan202, werli, mussatto and Badbond March 11, 2022 13:35

Badbond requested changes Mar 18, 2022

View reviewed changes

Badbond requested changes Mar 21, 2022

View reviewed changes

mussatto reviewed Mar 22, 2022

View reviewed changes

Badbond requested changes Mar 23, 2022

View reviewed changes

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from bb49a9e to 1c7b580 Compare March 28, 2022 14:29

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from b607643 to f08b3b0 Compare April 4, 2022 13:12

rickie requested review from mussatto and Badbond April 4, 2022 13:14

Badbond reviewed Apr 4, 2022

View reviewed changes

rickie requested a review from Badbond April 4, 2022 15:24

Badbond approved these changes Apr 4, 2022

View reviewed changes

mussatto approved these changes Apr 6, 2022

View reviewed changes

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from b89118c to 77b3b70 Compare April 16, 2022 11:46

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from 77b3b70 to f8119a4 Compare May 5, 2022 16:30

This was referenced May 16, 2022

Introduce AssertJ exception assertions Refaster templates #92

Merged

Improve Refaster matching algorithm #104

Closed

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from 52c1d9e to bc48dc2 Compare May 21, 2022 11:26

Stephan202 reviewed May 22, 2022

View reviewed changes

rickie removed the request for review from werli May 23, 2022 08:08

rickie mentioned this pull request May 23, 2022

Improve Refaster tests inconsistencies #109

Merged

rickie force-pushed the rossendrijver/junit_extension_refaster_tests branch from 5b49655 to 53ffe2b Compare August 4, 2022 12:18

Javadoc tweak

b0a8658

Stephan202 added 2 commits August 7, 2022 18:55

Merge branch 'master' into rossendrijver/junit_extension_refaster_tests

1dcd659

Semi-related fix

93785fb

rickie added a commit that referenced this pull request Aug 8, 2022

Introduce validator for Refaster template collections and tests (#43)

e97296c

Suggestions (partial README review)

b8e3fd8

Badbond reviewed Aug 9, 2022

View reviewed changes

Apply subset of README.md suggestions

dca5a6b

Badbond reviewed Aug 9, 2022

View reviewed changes

PSM-1299 Partially apply feedback from Pieter

f60da11

Badbond approved these changes Aug 9, 2022

View reviewed changes

Pieter's suggestions

4bcd8fb

Stephan202 reviewed Aug 9, 2022

View reviewed changes

Suggestions

b514ad9

Apply Pieter's suggestion

9b7beb3

Merge branch 'master' into rossendrijver/junit_extension_refaster_tests

fa0f49a

Stephan202 approved these changes Aug 10, 2022

View reviewed changes

Stephan202 merged commit b06945b into master Aug 10, 2022

Stephan202 deleted the rossendrijver/junit_extension_refaster_tests branch August 10, 2022 12:40

rickie mentioned this pull request Sep 2, 2022

Introduce @Template{,Collection} to improve the validation of *TemplatesTest{Input,Output} files. #25

Closed

		@@ -0,0 +1,4 @@
		/** Utilities that support testing Refaster templates and validate the tests. */


		@VisibleForTesting
		static final Supplier<ImmutableListMultimap<String, CodeTransformer>> ALL_CODE_TRANSFORMERS =

		@@ -0,0 +1,9 @@
		package tech.picnic.errorprone.refaster.test;

		final class MatchInWrongMethodTemplatesTest implements RefasterTemplateTestCase {

		/* Did not encounter test in /tech.picnic.errorprone.refaster.test.MissingTestAndWrongTestTemplatesTestInput.java for the following template(s):
		- TemplateWithoutTest

		template collection. This test utility applies the Refaster templates in the collection to a provided input file, and
		expects the result to exactly match the contents of a provided output file.

	- For each Refaster template in the collection, the input and output file contain a method. The name of the method is
	- For each Refaster template in the collection, the input and output file must contain a method. The name of the method is

		- The method contains at least one expression that matches the `@BeforeTemplate`. As a result, the output file contains
		the same method with an updated expression, matching the content of the `@AfterTemplate`.


		As a result from these tests, unexpected output will be shown in the console.

		An example of a folder structure for such a setup:

		└── ExampleTemplatesTestInput.java -- Contains a class named `ExampleTemplatesTest`
		└── ExampleTemplatesTestOutput.java -- Contains a class named `ExampleTemplatesTest`

		<module>refaster-resource-compiler</module>
		<module>refaster-runner</module>


		## What does this module do?

		These utilities allow validating the rewrites (or their absence) performed by

	These utilities allow validating the rewrites (or their absence) performed by
	These utilities allow for validating the rewrites (or absence thereof) as performed by

	be reported, and the associated template collection test will fail. This way
	be reported, and the associated template collection test will fail. This way,

	input and output files. The collection's Refaster templates are applied to the
	input and output source code files. The collection's Refaster templates are applied to the

	To adopt this setup, the following requirements have to be met:
	To adopt this setup, the following requirements must be met:

	template prefixed with `test` (e.g. `test<RefasterTemplateClassName>`).
	template prefixed with `test` (e.g. `test<RefasterTemplateInnerClassName>`).

	`@BeforeTemplate` of one specific Refaster template. As a result, the output
	`@BeforeTemplate` of the corresponding Refaster template. As a result, the output

		@@ -0,0 +1,4 @@
		/** Classes used to locate and apply compiled Refaster templates on. */

	* modifies exactly one distinct method, as indicated by each method's name.
	* modifies exactly one distinct method, as identified by the corresponding template's name and prefixed with `test`.

Introduce validator for Refaster template collections and tests #43

Introduce validator for Refaster template collections and tests #43

Conversation

rickie commented Mar 11, 2022 • edited Loading

Notes

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

rickie Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Mar 21, 2022

mussatto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mussatto Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Mar 28, 2022

rickie commented Apr 4, 2022

Badbond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Mar 11, 2022 •

edited

Loading

Badbond Mar 21, 2022 •

edited

Loading

Badbond Mar 21, 2022 •

edited

Loading

Badbond Mar 21, 2022 •

edited

Loading

rickie Mar 21, 2022 •

edited

Loading

mussatto Mar 22, 2022 •

edited

Loading

Badbond left a comment •

edited

Loading

Badbond Mar 23, 2022 •

edited

Loading

Badbond Mar 23, 2022 •

edited

Loading

Badbond Apr 4, 2022 •

edited

Loading

Badbond left a comment •

edited

Loading