cannot test properties with comment due to assertions issue

[KaiqianYang]

I wrote a recipe to add comment in properties, then wrote a test.

@Test
    void testFindPassword() {
        rewriteRun(
            spec -> spec.recipe(new AddCommentToProperty("*password","TODO: Don't save passwords or login information in files",true)),
            properties(
                """
                    application.Password = 1111
                    application.name = name
                      """,
                """
                    application.Password = 1111
                    #TODO: Don't save passwords or login information in files
                    application.name = name
                    """
            )
        );
    }

This test is getting error:

[To assert that a Recipe makes no change, supply only "before" source.] 
Expecting actual:
  "application.Password = 1111
#TODO: Don't save passwords or login information in files
application.name = name"
not to be equal to:
  "application.Password = 1111
#TODO: Don't save passwords or login information in files
application.name = name"

Apparently, the properties added with comment is not the same with the former properties. The issue seems due to assertion ignoring comments in properties file.

[knutwannheden]

@KaiqianYang Thanks for reaching out. It looks like your example got mangled here in the GitHub issue. I assume that in your source code it looks something like this:

    @Test
    void testFindPassword() {
        rewriteRun(
          spec -> spec.recipe(new AddCommentToProperty("*password", "TODO: Don't save passwords or login information in files", true)),
          properties(
            """
              application.Password = 1111
              application.name = name
              """,
            """
              application.Password = 1111
              #TODO: Don't save passwords or login information in files
              application.name = name
              """));
    }

OpenRewrite doesn't provide any AddCommentToProperty recipe, but that could be a welcome addition.

Given the error message I am however also surprised by the To assert that a Recipe makes no change, supply only "before" source. error. This shouldn't be the case. Can you tell us what version of OpenRewrite you are using. I cannot reproduce this behavior.

[KaiqianYang]

@KaiqianYang Thanks for reaching out. It looks like your example got mangled here in the GitHub issue. I assume that in your source code it looks something like this:

    @Test
    void testFindPassword() {
        rewriteRun(
          spec -> spec.recipe(new AddCommentToProperty("*password", "TODO: Don't save passwords or login information in files", true)),
          properties(
            """
              application.Password = 1111
              application.name = name
              """,
            """
              application.Password = 1111
              #TODO: Don't save passwords or login information in files
              application.name = name
              """));
    }

OpenRewrite doesn't provide any AddCommentToProperty recipe, but that could be a welcome addition.

Given the error message I am however also surprised by the To assert that a Recipe makes no change, supply only "before" source. error. This shouldn't be the case. Can you tell us what version of OpenRewrite you are using. I cannot reproduce this behavior.

Yes, AddCommentToProperty recipe is not provided but implemented here: https://github.com/Azure/azure-spring-rewrite/blob/recipe-find-eureka-config-client-info/src/main/java/com/azure/spring/migration/openrewrite/properties/AddCommentToProperty.java. The test is here: https://github.com/Azure/azure-spring-rewrite/blob/recipe-find-eureka-config-client-info/src/test/java/com/azure/spring/migration/openrewrite/properties/AddCommentToPropertyTest.java

We are using openrewrite 7.38

[knutwannheden]

@KaiqianYang I can confirm this is a bug. The problem is that the List returned by Properties.File#getContent() is modifiable. As a workaround you can in your recipe copy this into a new ArrayList: List<Content> contents = new ArrayList<>(file.getContent());. We will look into fixing the issue.

Thanks for reporting!

[KaiqianYang]

List contents = new ArrayList<>(file.getContent());

Thanks for confirming!

I hava copied that into a new ArrayList: https://github.com/Azure/azure-spring-rewrite/blob/recipe-find-eureka-config-client-info/src/main/java/com/azure/spring/migration/openrewrite/properties/AddCommentToProperty.java#L68

@Override
    public TreeVisitor<?, ExecutionContext> getVisitor() {
        return new PropertiesVisitor<ExecutionContext>() {
            @Override
            public Properties visitFile(Properties.File file, ExecutionContext ctx) {
                List<Content> contents = new ArrayList<>(file.getContent());
               ......

And the previous error gone, but instead there is an error: [Expected recipe to complete in 1 cycle, but took 2 cycles.]

In fact, I ran into this error with rewrite test several times but never really understood how to fix it. Could you help? I would really appreciate your advice.

[knutwannheden]

@KaiqianYang OpenRewrite's RewriteTest will by default run your recipe two times, just to make sure it doesn't make any unnecessary changes. I think in your case it would be a good idea to check if there already is a comment of the form you want to add and if it already exists, do nothing. Hope that makes sense.

You can also modify the test to expect multiple cycles, but I would try to avoid making "unexpected" changes. See https://docs.openrewrite.org/authoring-recipes/recipe-conventions-and-best-practices#do-no-harm for more details.

[KaiqianYang]

@KaiqianYang OpenRewrite's RewriteTest will by default run your recipe two times, just to make sure it doesn't make any unnecessary changes. I think in your case it would be a good idea to check if there already is a comment of the form you want to add and if it already exists, do nothing. Hope that makes sense.

You can also modify the test to expect multiple cycles, but I would try to avoid making "unexpected" changes. See https://docs.openrewrite.org/authoring-recipes/recipe-conventions-and-best-practices#do-no-harm for more details.

@knutwannheden Thanks for your explanation. I found the problem was that the pointers to properties file contents are changed due to new arrayList put in file regardless of whether content change made.

List<Content> contents = new ArrayList<>(file.getContent());
for (int i = 0; i < contents.size(); i++) {
      ......
}
return file.withContent(contents);

Solved my problem by only returning file.withContent(contents) when content change made.

Thanks for your help.