openrewrite / rewrite-logging-frameworks

OpenRewrite recipes for assisting with Java logging migration tasks.
Apache License 2.0
26 stars 21 forks source link

Add test for appended exception #136

Closed koppor closed 10 months ago

koppor commented 10 months ago

This refs https://github.com/openrewrite/rewrite/issues/4054, but contributes only a small portion of the issue in question.

What's changed?

Test added for checking rewrite of

logger.error("Cannot instantiate dependency: " + clazz, ex);

to

logger.error("Cannot instantiate dependency: {}", clazz, ex);
koppor commented 10 months ago

Fixed test so that the test should be green 😅

timtebeek commented 10 months ago

Even with this test it passes, even though that very closely matches: https://github.com/JabRef/jabref/blob/b7990ea3e80eae4ab7cc8cf8d508fea5d2e0ebe8/src/main/java/org/jabref/gui/FallbackExceptionHandler.java#L12

    @Test
    @Issue("https://github.com/openrewrite/rewrite/issues/4054")
    void exceptionAppended() {
        rewriteRun(
          spec -> spec.recipeFromResources("org.openrewrite.java.logging.slf4j.ParameterizedLogging"),
          //language=java
          java(
            """
              import org.slf4j.Logger;
              import org.slf4j.LoggerFactory;

              class Foo {
                  private static final Logger logger = LoggerFactory.getLogger(Foo.class);

                  static void method(Class<?> clazz, IllegalAccessException ex) {
                      logger.error("Cannot instantiate dependency: " + clazz, ex);
                  }
              }
              """,
            """
              import org.slf4j.Logger;
              import org.slf4j.LoggerFactory;

              class Foo {
                  private static final Logger logger = LoggerFactory.getLogger(Foo.class);

                  static void method(Class<?> clazz, IllegalAccessException ex) {
                      logger.error("Cannot instantiate dependency: {}", clazz, ex);
                  }
              }
              """
          )
        );
    }
koppor commented 10 months ago

Even with this test it passes, even though that very closely matches: JabRef/jabref@b7990ea/src/main/java/org/jabref/gui/FallbackExceptionHandler.java#L12

Yeah, this is the strange thing.

Running the recipe in the moderne plattform results in 0 updates. Think, the reason is somewhere else.

https://github.com/openrewrite/rewrite/issues/4054 needs to be investigated further.

timtebeek commented 10 months ago

Looks like we already had a very similar test for this; rather than duplicate I've moved the @Issue marker; hope you agree, since the new test did not help us to reproduce the issue (yet).

timtebeek commented 10 months ago

Turns out the issue is caused by missing type information, as explored through the platform: https://app.moderne.io/recipes/org.openrewrite.java.search.FindMissingTypes That means it's likely not a recipe issue. Thanks for helping trouble shoot this one!

koppor commented 10 months ago

Looks like we already had a very similar test for this; rather than duplicate I've moved the @Issue marker; hope you agree, since the new test did not help us to reproduce the issue (yet).

It's OK for me. I personally like to have more tests than less tests. It could be that someone modifies the logic in the future so that the two tests cover different things.