Unit tests for JUL to slf4j transformation

[woj-tek]

What's changed?

A set of unit-tests for JUL to slf4j rewrtites.

What's your motivation?

Followup to

• 155

Anyone you would like to review specifically?

Possibly @timtebeek :-)

Checklist

• [x] I've added unit tests to cover both positive and negative cases
• [ ] I've read and applied the recipe conventions and best practices
• [X] I've used the IntelliJ IDEA auto-formatter on affected files

(no negative cases; for now those are mostly unit-tests as a starting point thus best practices for recipe doesn't apply... yet)

[woj-tek]

I'm not sure if I haven't exaggerated with the amount of tests and the notion (for example that isLoggable should be removed altogether during processing.

[timtebeek]

Great to see! I have some formatting fixes to apply, but can't push to tigase:main it seems. Would you be able to change that?

fix.patch

```diff diff --git a/src/main/resources/META-INF/rewrite/slf4j.yml b/src/main/resources/META-INF/rewrite/slf4j.yml index 35b0b73..b3ff3fc 100644 --- a/src/main/resources/META-INF/rewrite/slf4j.yml +++ b/src/main/resources/META-INF/rewrite/slf4j.yml @@ -175,3 +175,16 @@ recipeList: oldFullyQualifiedTypeName: org.apache.commons.logging.Log newFullyQualifiedTypeName: org.slf4j.Logger - org.openrewrite.java.logging.ChangeLombokLogAnnotation + +--- +type: specs.openrewrite.org/v1beta/recipe +name: org.openrewrite.java.logging.slf4j.JulToSlf4j +displayName: Migrate JUL to SLF4J +description: Migrates usage of Java Util Logging (JUL) to using SLF4J directly. +tags: + - logging + - java-util-logging + - slf4j +recipeList: + - org.openrewrite.java.logging.log4j.JulToLog4j + - org.openrewrite.java.logging.slf4j.Log4j2ToSlf4j1 diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/JulToSlf4jTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/JulToSlf4jTest.java index 7dc3deb..ede7701 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/JulToSlf4jTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/JulToSlf4jTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 the original author or authors. + * Copyright 2024 the original author or authors. *

* Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -35,360 +35,405 @@ import static org.openrewrite.java.Assertions.java; // * WARNING -> WARN // * SEVERE -> ERROR -class JulToSlf4jTest - implements RewriteTest { - - @Override - public void defaults(RecipeSpec spec) { - spec.typeValidationOptions(TypeValidation.builder().build()) - .recipe(Environment.builder() -// .scanRuntimeClasspath("org.openrewrite.java.logging") - .scanYamlResources() - .build() - .activateRecipes()) - .parser(JavaParser.fromJavaVersion() - .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1")); - } - - @Test - void logLevelAllToTraceIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level; - import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.ALL)) { - logger.all("All log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.trace("All log entry"); - } - } - """)); - } - - @Test - void logLevelFineToDebugIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.FINE)) { - logger.fine("Fine log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.debug("Fine log entry"); - } - } - """)); - } - - @Test - void logLevelFinerToDebugIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.FINER)) { - logger.finer("Finer log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.debug("Finer log entry"); - } - } - """)); - } - - @Test - void logLevelFinestToTraceIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.FINEST)) { - logger.finest("Finest log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.trace("Finest log entry"); - } - } - """)); - } - - @Test - void logLevelInfoToInfoIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.INFO)) { - logger.info("Info log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.info("Info log entry"); - } - } - """)); - } - - @Test - void logLevelSevereToErrorIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.SEVERE)) { - logger.severe("Severe log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.error("Severe log entry"); - } - } - """)); - } - - @Test - void logLevelWarningToWarnIsLoggable() { - //language=java - rewriteRun(java(""" - import java.util.logging.Level;import java.util.logging.Logger; - - class Test { - static void method(Logger logger) { - if (logger.isLoggable(Level.WARNING)) { - logger.warning("Warning log entry"); - } - } - } - """, """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger) { - logger.warn("Warning log entry"); - } - } - """)); - } - - @Test - void loggerToLoggerFactory() { - rewriteRun( - // language=java - java(""" - import java.util.logging.Logger; - - class Test { - Logger logger1 = Logger.getLogger("Test"); - Logger logger2 = Logger.getLogger(Test.class.getName()); - Logger logger3 = Logger.getLogger(Test.class.getCanonicalName()); - } - """, """ - import org.slf4j.Logger; - import org.slf4j.LoggerFactory; - - class Test { - Logger logger1 = LoggerFactory.getLogger("Test"); - Logger logger2 = LoggerFactory.getLogger(Test.class); - Logger logger3 = LoggerFactory.getLogger(Test.class); - } - """)); - } - - @Test - void parametrizedLoggerCallsIsLoggable() { - rewriteRun( - // language=java - java(""" - import java.util.logging.Level; - import java.util.logging.Logger; - - class Test { - void method(Logger logger) { - logger.log(Level.FINEST, "FINEST Log entry, param1: {0}", param1); - logger.log(Level.FINEST, "FINEST Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.FINEST, "FINEST Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.FINER, "FINER Log entry, param1: {0}", param1); - logger.log(Level.FINER, "FINER Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.FINER, "FINER Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.FINE, "FINE Log entry, param1: {0}", param1); - logger.log(Level.FINE, "FINE Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.FINE, "FINE Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}", param1); - logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.INFO, "INFO Log entry, param1: {0}", param1); - logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.WARNING, "WARNING Log entry, param1: {0}", param1); - logger.log(Level.WARNING, "WARNING Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.WARNING, "WARNING Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - - logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}", param1); - logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); - logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); - } - } - """, """ - import org.apache.logging.log4j.Logger; - - class Test { - void method(Logger logger) { - logger.trace("FINEST Log entry, param1: {}", param1); - logger.trace("FINEST Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.trace("FINEST Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.trace("FINER Log entry, param1: {}", param1); - logger.trace("FINER Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.trace("FINER Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.debug("FINE Log entry, param1: {}", param1); - logger.debug("FINE Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.debug("FINE Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.info("CONFIG Log entry, param1: {}", param1); - logger.info("CONFIG Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.info("CONFIG Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.info("INFO Log entry, param1: {}", param1); - logger.info("INFO Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.info("INFO Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.warn("WARNING Log entry, param1: {}", param1); - logger.warn("WARNING Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.warn("WARNING Log entry, param1: {}, param2: {}, etc", param1, param2); - - logger.error("SEVERE Log entry, param1: {}", param1); - logger.error("SEVERE Log entry, param1: {}, param2: {}, etc", param1, param2); - logger.error("SEVERE Log entry, param1: {}, param2: {}, etc", param1, param2); - } - } - """)); - } - - @Test - void simpleLoggerCalls() { - rewriteRun( - // language=java - java(""" - import java.util.logging.Level; - import java.util.logging.Logger; - - class Test { - void method(Logger logger) { - logger.finest("finest"); - logger.finest(() -> "finest"); - logger.finer("finer"); - logger.finer(() -> "finer"); - logger.fine("fine"); - logger.fine(() -> "fine"); - logger.config("config"); - logger.config(() -> "config"); - logger.info("info"); - logger.info(() -> "info"); - logger.warning("warning"); - logger.warning(() -> "warning"); - logger.severe("severe"); - logger.severe(() -> "severe"); - } - } - """, """ - import org.apache.logging.log4j.Logger; - - class Test { - void method(Logger logger) { - logger.trace("finest"); - logger.trace(() -> "finest"); - logger.trace("finer"); - logger.trace(() -> "finer"); - logger.debug("fine"); - logger.debug(() -> "fine"); - logger.info("config"); - logger.info(() -> "config"); - logger.info("info"); - logger.info(() -> "info"); - logger.warn("warning"); - logger.warn(() -> "warning"); - logger.error("severe"); - logger.error(() -> "severe"); - } - } - """)); - } - - @Test - void staticFinalLoggerIsStaticFinal() { - //language=java - rewriteRun(java(""" - import java.util.logging.Logger; - - class Test { - private static final Logger logger1 = Logger.getLogger("Test"); - private static final Logger logger2 = Logger.getLogger(Test.class.getName()); - private static final Logger logger3 = Logger.getLogger(Test.class.getCanonicalName()); - } - """, """ - import org.slf4j.Logger; - import org.slf4j.LoggerFactory; - - class Test { - private static final Logger logger1 = LoggerFactory.getLogger("Test"); - private static final Logger logger2 = LoggerFactory.getLogger(Test.class); - private static final Logger logger3 = LoggerFactory.getLogger(Test.class); - } - """)); - } +class JulToSlf4jTest implements RewriteTest { + @Override + public void defaults(RecipeSpec spec) { + spec.typeValidationOptions(TypeValidation.builder().build()) + .recipeFromResources("org.openrewrite.java.logging.slf4j.JulToSlf4j") + .parser(JavaParser.fromJavaVersion() + .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1")); + } + + @Test + void logLevelAllToTraceIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level; + import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.ALL)) { + logger.log(Level.ALL, "All log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.trace("All log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelFineToDebugIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.FINE)) { + logger.fine("Fine log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.debug("Fine log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelFinerToDebugIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.FINER)) { + logger.finer("Finer log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.debug("Finer log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelFinestToTraceIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.FINEST)) { + logger.finest("Finest log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.trace("Finest log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelInfoToInfoIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.INFO)) { + logger.info("Info log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.info("Info log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelSevereToErrorIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.SEVERE)) { + logger.severe("Severe log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.error("Severe log entry"); + } + } + """ + ) + ); + } + + @Test + void logLevelWarningToWarnIsLoggable() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Level;import java.util.logging.Logger; + + class Test { + static void method(Logger logger) { + if (logger.isLoggable(Level.WARNING)) { + logger.warning("Warning log entry"); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger) { + logger.warn("Warning log entry"); + } + } + """ + ) + ); + } + + @Test + void loggerToLoggerFactory() { + rewriteRun( + // language=java + java( + """ + import java.util.logging.Logger; + + class Test { + Logger logger1 = Logger.getLogger("Test"); + Logger logger2 = Logger.getLogger(Test.class.getName()); + Logger logger3 = Logger.getLogger(Test.class.getCanonicalName()); + } + """, + """ + import org.slf4j.Logger; + import org.slf4j.LoggerFactory; + + class Test { + Logger logger1 = LoggerFactory.getLogger("Test"); + Logger logger2 = LoggerFactory.getLogger(Test.class); + Logger logger3 = LoggerFactory.getLogger(Test.class); + } + """ + ) + ); + } + + @Test + void parametrizedLoggerCallsIsLoggable() { + rewriteRun( + // language=java + java( + """ + import java.util.logging.Level; + import java.util.logging.Logger; + + class Test { + void method(Logger logger, String param1, String param2) { + logger.log(Level.FINEST, "FINEST Log entry, param1: {0}", param1); + logger.log(Level.FINEST, "FINEST Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.FINEST, "FINEST Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.FINER, "FINER Log entry, param1: {0}", param1); + logger.log(Level.FINER, "FINER Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.FINER, "FINER Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.FINE, "FINE Log entry, param1: {0}", param1); + logger.log(Level.FINE, "FINE Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.FINE, "FINE Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}", param1); + logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.CONFIG, "CONFIG Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.INFO, "INFO Log entry, param1: {0}", param1); + logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.WARNING, "WARNING Log entry, param1: {0}", param1); + logger.log(Level.WARNING, "WARNING Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.WARNING, "WARNING Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + + logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}", param1); + logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}, param2: {1}, etc", new Object[]{param1, param2}); + logger.log(Level.SEVERE, "SEVERE Log entry, param1: {0}, param2: {1}, etc", new String[]{param1, param2}); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + void method(Logger logger, String param1, String param2) { + logger.trace("FINEST Log entry, param1: {}", param1); + logger.trace("FINEST Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.trace("FINEST Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.trace("FINER Log entry, param1: {}", param1); + logger.trace("FINER Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.trace("FINER Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.debug("FINE Log entry, param1: {}", param1); + logger.debug("FINE Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.debug("FINE Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.info("CONFIG Log entry, param1: {}", param1); + logger.info("CONFIG Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.info("CONFIG Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.info("INFO Log entry, param1: {}", param1); + logger.info("INFO Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.info("INFO Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.warn("WARNING Log entry, param1: {}", param1); + logger.warn("WARNING Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.warn("WARNING Log entry, param1: {}, param2: {}, etc", param1, param2); + + logger.error("SEVERE Log entry, param1: {}", param1); + logger.error("SEVERE Log entry, param1: {}, param2: {}, etc", param1, param2); + logger.error("SEVERE Log entry, param1: {}, param2: {}, etc", param1, param2); + } + } + """ + ) + ); + } + + @Test + void simpleLoggerCalls() { + rewriteRun( + // language=java + java(""" + import java.util.logging.Level; + import java.util.logging.Logger; + + class Test { + void method(Logger logger) { + logger.finest("finest"); + logger.finest(() -> "finest"); + logger.finer("finer"); + logger.finer(() -> "finer"); + logger.fine("fine"); + logger.fine(() -> "fine"); + logger.config("config"); + logger.config(() -> "config"); + logger.info("info"); + logger.info(() -> "info"); + logger.warning("warning"); + logger.warning(() -> "warning"); + logger.severe("severe"); + logger.severe(() -> "severe"); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + void method(Logger logger) { + logger.trace("finest"); + logger.trace(() -> "finest"); + logger.trace("finer"); + logger.trace(() -> "finer"); + logger.debug("fine"); + logger.debug(() -> "fine"); + logger.info("config"); + logger.info(() -> "config"); + logger.info("info"); + logger.info(() -> "info"); + logger.warn("warning"); + logger.warn(() -> "warning"); + logger.error("severe"); + logger.error(() -> "severe"); + } + } + """ + ) + ); + } + + @Test + void staticFinalLoggerIsStaticFinal() { + //language=java + rewriteRun( + java( + """ + import java.util.logging.Logger; + + class Test { + private static final Logger logger1 = Logger.getLogger("Test"); + private static final Logger logger2 = Logger.getLogger(Test.class.getName()); + private static final Logger logger3 = Logger.getLogger(Test.class.getCanonicalName()); + } + """, + """ + import org.slf4j.Logger; + import org.slf4j.LoggerFactory; + + class Test { + private static final Logger logger1 = LoggerFactory.getLogger("Test"); + private static final Logger logger2 = LoggerFactory.getLogger(Test.class); + private static final Logger logger3 = LoggerFactory.getLogger(Test.class); + } + """ + ) + ); + } } ```

[woj-tek]

I tried applying the patch but got "error: corrupt patch at line 794"

Nevertheless, I added you to the repo (had to move it though…)

[timtebeek]

Great start @woj-tek ; I've pushed a few quick changes to get the first recipes to go through.

Feel free to extend what I did with further commits. I do like quick iterations, so perhaps we can merge part of this work early, and mark some of the more complex cases as @Disabled tests for now.

[timtebeek]

Feel free to collapse those isLoggable tests into one that tests all, similar to what we did for the logging messages; I think the implementation for that one is going to be very similar. I do think we should convert from let's say isLoggable(Level.INFO) to isInfoEnabled(), instead of just removing the conditionals all the time.

[woj-tek]

Great to see! I have some formatting fixes to apply,

Do you have IDEA formatter that does that? Mine seems to make the formatting utterly awful.

Feel free to extend what I did with further commits. I do like quick iterations, so perhaps we can merge part of this work early, and mark some of the more complex cases as @Disabled tests for now.

OK, I disabled parametrized tests for now.

Feel free to collapse those isLoggable tests into one that tests all, similar to what we did for the logging messages; I think the implementation for that one is going to be very similar. I do think we should convert from let's say isLoggable(Level.INFO) to isInfoEnabled(), instead of just removing the conditionals all the time.

OK, collapsed that into one and updated org.openrewrite.java.logging.jul.LoggerIsLoggable but I have a couple of questions/issues: 1) shouldn't org.openrewrite.java.logging.jul.LoggerIsLoggable be moved to slf4j package as it does conversion from JUL to slf4j (it seems that's the convention) 2) I had to explicitly use fully qualified org.slf4j.Logger to avoid conflict - I hope it's ok? 3) I had problem with isLoggable for Level.ALL - it doesn't have Logger.all(String) thus it would require conversion from Logger.log(Level.ALL, String) to Logger.trace(String) which I don't know how to handle so I leaft it out for now

if (logger.isLoggable(Level.ALL)) {
    logger.log(Level.ALL, "ALL log entry");
}
…
if (logger.isTraceEnabled()) {
    logger.trace("ALL log entry");
}

How should we handle dealing with removal of is<Level>Enabled? I guesss it could/would be slf4j specific so dedicated unit-test and recipe?

[timtebeek]

Great to see! I have some formatting fixes to apply,

Do you have IDEA formatter that does that? Mine seems to make the formatting utterly awful.

As a team we've standardized on the IntelliJ IDEA auto format; that seemed easiest to use as long as folks are all using IntelliJ. For other IDEAs that's not as great; which one are you using?

Feel free to extend what I did with further commits. I do like quick iterations, so perhaps we can merge part of this work early, and mark some of the more complex cases as @Disabled tests for now.

OK, I disabled parametrized tests for now.

Perfect, let's pick that one up separately, probably with a dedicated visitor recipe to unpack the parameters from the argument array.

Feel free to collapse those isLoggable tests into one that tests all, similar to what we did for the logging messages; I think the implementation for that one is going to be very similar. I do think we should convert from let's say isLoggable(Level.INFO) to isInfoEnabled(), instead of just removing the conditionals all the time.

OK, collapsed that into one and updated org.openrewrite.java.logging.jul.LoggerIsLoggable but I have a couple of questions/issues:

1. shouldn't org.openrewrite.java.logging.jul.LoggerIsLoggable be moved to slf4j package as it does conversion from JUL to slf4j (it seems that's the convention)

Yes you're right! I've renamed the classes to use a Jul prefix and moved them to the slf4j package.

2. I had to explicitly use fully qualified org.slf4j.Logger to avoid conflict - I hope it's ok?

Yes that's fine; the generated recipes automatically shorten those qualified usages where possible, so should be find

3. I had problem with isLoggable for Level.ALL - it doesn't have Logger.all(String) thus it would require conversion from Logger.log(Level.ALL, String) to Logger.trace(String) which I don't know how to handle so I leaft it out for now

No worries; I've added a dedicated recipe specifically for this conversion in https://github.com/openrewrite/rewrite-logging-frameworks/pull/157/commits/5f2b29f0b17b123c6bfc9874b621682a30dbf360

if (logger.isLoggable(Level.ALL)) {
    logger.log(Level.ALL, "ALL log entry");
}
…
if (logger.isTraceEnabled()) {
    logger.trace("ALL log entry");
}

How should we handle dealing with removal of is<Level>Enabled? I guess it could/would be slf4j specific so dedicated unit-test and recipe?

What would you want to additionally handle beyond what's already asserted through tests? What's the reason you'd want to remove those conditionals? As I've been told there's still some performance benefits to checking before logging. If you'd still like to remove them then we can have the @AfterTemplate return true, and then call out to SimplifyConstantIfBranchExecution to clean up the conditional.

[woj-tek]

As a team we've standardized on the IntelliJ IDEA auto format; that seemed easiest to use as long as folks are all using IntelliJ. For other IDEAs that's not as great; which one are you using?

I'm using IDEA but we had custom formatter. Fortunatlely it's possible to configure it per-project now.

What would you want to additionally handle beyond what's already asserted through tests? What's the reason you'd want to remove those conditionals? As I've been told there's still some performance benefits to checking before logging. If you'd still like to remove them then we can have the @AfterTemplate return true, and then call out to SimplifyConstantIfBranchExecution to clean up the conditional.

Yes, I wanted to remove the conditionals but I think it could be a separate recipe to be run if someone wants to "cleaup" the code.

From what I gather the check before logging for performance benefits was mostly applicable to JUL (especially in the past). Nowadays, it should be less of an issue in most of the cases like logger call with plain parameters - if someone is doing some processing to the parameters then checking before the call could bring some performance boost as those calculations would be avoided in such case.

There is also a "novelty" in slf4j: Fluent Logging API:

For disabled log levels, the returned LoggingEventBuilder instance does nothing, thus preserving the nanosecond level performance of the traditional logging interface.

which I think could nullify that issue altogether but I think this could be handled on case-by-case-basis. (I was pondering whether we shouldn't migrate to fluent API but that would result in kinda complicated recipe which mostly woudln't benefit most of the users as simple logger calls with parameters seems to be better here)