Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for appended exception #136

Closed
wants to merge 7 commits into from
Closed

Add test for appended exception #136

wants to merge 7 commits into from

Conversation

koppor
Copy link

@koppor koppor commented Jan 5, 2024

This refs openrewrite/rewrite#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);

@timtebeek timtebeek marked this pull request as draft January 5, 2024 11:27
@koppor
Copy link
Author

koppor commented Jan 5, 2024

Fixed test so that the test should be green 😅

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a test to reproduce the issue you're having. Oddly enough the test passes, so now we need to figure out what's different in your scenario. 🤔

@timtebeek
Copy link
Contributor

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
Copy link
Author

koppor commented Jan 5, 2024

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.

openrewrite/rewrite#4054 needs to be investigated further.

@koppor koppor marked this pull request as ready for review January 5, 2024 12:55
@timtebeek
Copy link
Contributor

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
Copy link
Contributor

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!

@timtebeek timtebeek closed this Jan 5, 2024
@koppor koppor deleted the add-parameterized-logging-test branch January 6, 2024 11:20
@koppor
Copy link
Author

koppor commented Jan 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants