test: 100% line and branch coverage of the whole library

.claude/skills/phase-start/SKILL.md

@@ -85,10 +85,14 @@ flavors separately.
 Write the failing test first where the phase fixes a bug. Before pushing, run what CI runs:
 
 ```
-./gradlew lint testJdk11DebugUnitTest testJdk8DebugUnitTest
+./gradlew lint testJdk11DebugUnitTest testJdk8DebugUnitTest verifyCoverage
 ./scripts/check-architecture-docs.sh
 ```
 
+`verifyCoverage` fails unless the unit tests cover every line and branch of every class, so new
+or changed code comes with the tests that cover it. When it fails, `./gradlew
+coverageReportJdk11Debug` and `scripts/coverage-gaps.py` on the XML report list what is missing.
+
 Open the PR as a draft (see `AGENTS.md`), with a Conventional Commits title and a short
 "deferred work considered" note naming what you folded in and what you deliberately left. Two
 lines. It is the record that the check happened, and it is what lets the next phase trust that

.github/copilot-instructions.md

@@ -17,8 +17,10 @@ sockets, syslog, email).
 - **Tests:** JUnit tests live under `logback-android/src/test/java`, except the
   tests of the Android-specific layer (`ch.qos.logback.{classic,core}.android`),
   which are written in Kotlin under `logback-android/src/test/kotlin`. Run with
-  `./gradlew test`. That layer is gated at 100% line and branch coverage
-  (`./gradlew verifyAndroidLayerCoverage`); keep it there when changing it.
+  `./gradlew test`. The whole library, Java and Kotlin, is gated at 100% line
+  and branch coverage (`./gradlew verifyCoverage`; `scripts/coverage-gaps.py`
+  lists what a report misses); a change must come with the tests that keep it
+  there.
 - **Upstream parity:** Much of the code mirrors upstream logback. Prefer changes
   that stay close to upstream behavior and naming so the port remains easy to
   sync.

.github/workflows/build.yml

@@ -92,18 +92,32 @@ jobs:
       - name: Unit tests (jdk8 variant)
         run: ./gradlew testJdk8DebugUnitTest
 
-      # The Android-specific layer (ch.qos.logback.{classic,core}.android) must
-      # stay at 100% line and branch coverage (rule in logback-android/build.gradle).
-      # Reuses the coverage the unit test runs above recorded.
-      - name: Coverage gate (Android-specific layer)
-        run: ./gradlew verifyAndroidLayerCoverage androidLayerCoverageReportJdk11Debug
+      # Every class in the library, Java and Kotlin, must stay at 100% line and
+      # branch coverage (rule in logback-android/build.gradle). Reuses the
+      # coverage the unit test runs above recorded. The reports are written
+      # first so that the next step can list what a failing gate is missing.
+      - name: Coverage reports
+        run: ./gradlew coverageReportJdk11Debug coverageReportJdk8Debug
+
+      - name: Coverage gaps
+        run: |
+          for variant in Jdk11Debug Jdk8Debug; do
+            report="logback-android/build/reports/jacoco/coverageReport$variant/coverageReport$variant.xml"
+            echo "::group::Uncovered lines and branches ($variant)"
+            python3 scripts/coverage-gaps.py "$report"
+            echo "::endgroup::"
+            { echo "### Uncovered lines and branches ($variant)"; echo '```'; python3 scripts/coverage-gaps.py "$report"; echo '```'; } >> "$GITHUB_STEP_SUMMARY"
+          done
+
+      - name: Coverage gate
+        run: ./gradlew verifyCoverage
 
       - name: Upload coverage report
         if: ${{ !cancelled() && matrix.java == 21 }}
         uses: actions/upload-artifact@v7
         with:
           name: coverage-report
-          path: logback-android/build/reports/jacoco/androidLayerCoverageReportJdk11Debug/
+          path: logback-android/build/reports/jacoco/coverageReportJdk11Debug/
 
       - name: Upload test reports
         if: failure()

logback-android/build.gradle

@@ -63,7 +63,7 @@ android {
         debug {
             debuggable true
             // Records JaCoCo coverage of the debug unit tests, which the
-            // Android-layer coverage gate at the end of this file checks.
+            // coverage gate at the end of this file checks.
             enableUnitTestCoverage true
         }
     }
@@ -137,37 +137,37 @@ afterEvaluate {
     }
 }
 
-// Coverage gate for the Android-specific layer (ch.qos.logback.{classic,core}.android):
-// its unit tests must cover 100% of its lines and branches, so that they pin
-// the behavior of every path. For each debug variant,
-// androidLayerCoverageReport<Variant> writes an HTML/XML report and
-// androidLayerCoverageVerification<Variant> enforces the rule;
-// verifyAndroidLayerCoverage runs both variants' checks.
-def androidLayerClasses = ['ch/qos/logback/classic/android/**', 'ch/qos/logback/core/android/**']
-def androidLayerCoverage = { JacocoReportBase task, String variant ->
+// Coverage gate for the whole library, Java and Kotlin: the unit tests must
+// cover 100% of the lines and branches of every class, so that they pin the
+// behavior of every path. For each debug variant, coverageReport<Variant>
+// writes an HTML/XML report (scripts/coverage-gaps.py lists what it misses)
+// and coverageVerification<Variant> enforces the rule; verifyCoverage runs
+// both variants' checks.
+def libraryClasses = ['ch/qos/logback/**', 'org/slf4j/impl/**']
+def libraryCoverage = { JacocoReportBase task, String variant ->
     def testTask = tasks.named("test${variant}UnitTest", Test)
     task.dependsOn(testTask)
     task.executionData.from(testTask.map { it.extensions.getByType(JacocoTaskExtension).destinationFile })
     task.classDirectories.from(files(
             tasks.named("compile${variant}Kotlin").flatMap { it.destinationDirectory },
             tasks.named("compile${variant}JavaWithJavac").flatMap { it.destinationDirectory },
-    ).asFileTree.matching { include androidLayerClasses })
+    ).asFileTree.matching { include libraryClasses })
     task.sourceDirectories.from(files('src/main/kotlin', 'src/main/java'))
 }
-def androidLayerVerifications = ['Jdk11Debug', 'Jdk8Debug'].collect { variant ->
-    tasks.register("androidLayerCoverageReport${variant}", JacocoReport) {
+def coverageVerifications = ['Jdk11Debug', 'Jdk8Debug'].collect { variant ->
+    tasks.register("coverageReport${variant}", JacocoReport) {
         group = 'verification'
-        description = "Reports the ${variant} unit tests' coverage of the Android-specific layer."
-        androidLayerCoverage(it, variant)
+        description = "Reports the ${variant} unit tests' coverage of the library."
+        libraryCoverage(it, variant)
         reports {
             html.required = true
             xml.required = true
         }
     }
-    tasks.register("androidLayerCoverageVerification${variant}", JacocoCoverageVerification) {
+    tasks.register("coverageVerification${variant}", JacocoCoverageVerification) {
         group = 'verification'
-        description = "Fails unless the ${variant} unit tests cover every line and branch of the Android-specific layer."
-        androidLayerCoverage(it, variant)
+        description = "Fails unless the ${variant} unit tests cover every line and branch of the library."
+        libraryCoverage(it, variant)
         violationRules {
             rule {
                 element = 'CLASS'
@@ -177,10 +177,10 @@ def androidLayerVerifications = ['Jdk11Debug', 'Jdk8Debug'].collect { variant ->
         }
     }
 }
-tasks.register('verifyAndroidLayerCoverage') {
+tasks.register('verifyCoverage') {
     group = 'verification'
-    description = 'Fails unless the unit tests cover every line and branch of the Android-specific layer.'
-    dependsOn androidLayerVerifications
+    description = 'Fails unless the unit tests cover every line and branch of the library.'
+    dependsOn coverageVerifications
 }
 
 dependencies {

logback-android/src/main/java/ch/qos/logback/classic/joran/ReconfigureOnChangeTask.java

@@ -150,7 +150,7 @@ private void fallbackConfiguration(LoggerContext lc, List<SaxEvent> eventList) {
         ConfigurationWatchList oldCWL = ConfigurationWatchListUtil.getConfigurationWatchList(lc);
         ConfigurationWatchList newCWL = oldCWL.buildClone();
 
-        if (failsafeEvents == null || failsafeEvents.isEmpty()) {
+        if (failsafeEvents.isEmpty()) {
             addWarn("No previous configuration to fall back on.");
         } else {
             addWarn(FALLING_BACK_TO_SAFE_CONFIGURATION);

logback-android/src/main/java/ch/qos/logback/classic/net/SimpleSocketServer.java

@@ -78,7 +78,11 @@ protected static void doMain(Class<? extends SimpleSocketServer> serverClass,
     LoggerContext lc = (LoggerContext) LoggerFactory.getILoggerFactory();
     configureLC(lc, configFile);
 
-    SimpleSocketServer sss = new SimpleSocketServer(lc, port);
+    // Unlike upstream logback 1.2, which ignores serverClass and always
+    // starts a plain SimpleSocketServer (so SimpleSSLSocketServer.main
+    // started a server without SSL), create the requested class.
+    SimpleSocketServer sss = serverClass.getConstructor(LoggerContext.class, int.class)
+        .newInstance(lc, port);
     sss.start();
   }
 

logback-android/src/main/java/ch/qos/logback/classic/spi/ThrowableProxy.java

@@ -17,8 +17,6 @@
 
 import ch.qos.logback.core.CoreConstants;
 
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.util.Collections;
 import java.util.IdentityHashMap;
 import java.util.Set;
@@ -38,18 +36,6 @@ public class ThrowableProxy implements IThrowableProxy {
   private transient PackagingDataCalculator packagingDataCalculator;
   private boolean calculatedPackageData = false;
 
-  private static final Method GET_SUPPRESSED_METHOD;
-
-  static {
-    Method method = null;
-    try {
-      method = Throwable.class.getMethod("getSuppressed");
-    } catch (NoSuchMethodException e) {
-      // ignore, will get thrown in Java < 7
-    }
-    GET_SUPPRESSED_METHOD = method;
-  }
-
   private static final ThrowableProxy[] NO_SUPPRESSED = new ThrowableProxy[0];
   private static final StackTraceElementProxy[] NO_STACK_TRACE = new StackTraceElementProxy[0];
 
@@ -78,26 +64,16 @@ private ThrowableProxy(Throwable throwable, Set<Throwable> visited) {
                 stackTraceElementProxyArray);
       }
 
-      if (GET_SUPPRESSED_METHOD != null) {
-        // this will only execute on Java >= 7
-        try {
-          Object obj = GET_SUPPRESSED_METHOD.invoke(throwable);
-          if (obj instanceof Throwable[]) {
-            Throwable[] throwableSuppressed = (Throwable[]) obj;
-            if (throwableSuppressed.length > 0) {
-              suppressed = new ThrowableProxy[throwableSuppressed.length];
-              for (int i = 0; i < throwableSuppressed.length; i++) {
-                this.suppressed[i] = new ThrowableProxy(throwableSuppressed[i], visited);
-                this.suppressed[i].commonFrames = ThrowableProxyUtil
-                        .findNumberOfCommonFrames(throwableSuppressed[i].getStackTrace(),
-                                stackTraceElementProxyArray);
-              }
-            }
-          }
-        } catch (IllegalAccessException e) {
-          // ignore
-        } catch (InvocationTargetException e) {
-          // ignore
+      Throwable[] throwableSuppressed = throwable.getSuppressed();
+      // while JDK's implementation of getSuppressed() will always return a non-null array,
+      // this might not be the case in mocked throwables or in other implementations
+      if (throwableSuppressed != null && throwableSuppressed.length > 0) {
+        suppressed = new ThrowableProxy[throwableSuppressed.length];
+        for (int i = 0; i < throwableSuppressed.length; i++) {
+          this.suppressed[i] = new ThrowableProxy(throwableSuppressed[i], visited);
+          this.suppressed[i].commonFrames = ThrowableProxyUtil
+                  .findNumberOfCommonFrames(throwableSuppressed[i].getStackTrace(),
+                          stackTraceElementProxyArray);
         }
       }
     }

logback-android/src/main/java/ch/qos/logback/classic/util/ContextInitializer.java

@@ -54,16 +54,14 @@ public ContextInitializer(LoggerContext loggerContext) {
    * Finds a configuration file by system property
    * @return the file; or {@code null} if not found
    */
-  private URL findConfigFileFromSystemProperties(boolean updateStatus) {
+  private URL findConfigFileFromSystemProperties() {
     String logbackConfigFile = OptionHelper.getSystemProperty(CONFIG_FILE_PROPERTY);
     if (logbackConfigFile != null) {
       URL result = null;
       try {
         File file = new File(logbackConfigFile);
         if (file.exists() && file.isFile()) {
-          if (updateStatus) {
-            statusOnResourceSearch(logbackConfigFile, logbackConfigFile);
-          }
+          statusOnResourceSearch(logbackConfigFile, logbackConfigFile);
           result = file.toURI().toURL();
         } else {
           result = new URL(logbackConfigFile);
@@ -77,9 +75,7 @@ private URL findConfigFileFromSystemProperties(boolean updateStatus) {
           return result;
         }
       } finally {
-        if (updateStatus) {
-          statusOnResourceSearch(logbackConfigFile, result != null ? result.toString() : null);
-        }
+        statusOnResourceSearch(logbackConfigFile, result != null ? result.toString() : null);
       }
     }
     return null;
@@ -89,23 +85,21 @@ private URL findConfigFileFromSystemProperties(boolean updateStatus) {
    * Finds a configuration file in the application's assets directory
    * @return the URL of the file; or {@code null} if not found
    */
-  private URL findConfigFileURLFromAssets(boolean updateStatus) {
-    return getResource(AUTOCONFIG_FILE, this.classLoader, updateStatus);
+  private URL findConfigFileURLFromAssets() {
+    return getResource(AUTOCONFIG_FILE, this.classLoader);
   }
 
   /**
    * Uses the given classloader to search for a resource
    * @return the URL of the resource; or {@code null} if not found
    */
-  private URL getResource(String filename, ClassLoader myClassLoader, boolean updateStatus) {
+  private URL getResource(String filename, ClassLoader myClassLoader) {
     URL url = myClassLoader.getResource(filename);
-    if (updateStatus) {
-      String resourcePath = null;
-      if (url != null) {
-        resourcePath = filename;
-      }
-      statusOnResourceSearch(filename, resourcePath);
+    String resourcePath = null;
+    if (url != null) {
+      resourcePath = filename;
     }
+    statusOnResourceSearch(filename, resourcePath);
     return url;
   }
 
@@ -121,24 +115,21 @@ private URL getResource(String filename, ClassLoader myClassLoader, boolean upda
   public void autoConfig() throws JoranException {
     StatusListenerConfigHelper.installIfAsked(loggerContext);
 
-    boolean verbose = true;
     boolean configured = false;
 
     JoranConfigurator configurator = new JoranConfigurator();
     configurator.setContext(loggerContext);
 
     // search system property
-    if (!configured) {
-      URL url = findConfigFileFromSystemProperties(verbose);
-      if (url != null) {
-        configurator.doConfigure(url);
-        configured = true;
-      }
+    URL url = findConfigFileFromSystemProperties();
+    if (url != null) {
+      configurator.doConfigure(url);
+      configured = true;
     }
 
     // search assets
     if (!configured) {
-      URL assetsConfigXml = findConfigFileURLFromAssets(verbose);
+      URL assetsConfigXml = findConfigFileURLFromAssets();
       if (assetsConfigXml != null) {
         configurator.doConfigure(assetsConfigXml);
         configured = true;

logback-android/src/main/java/ch/qos/logback/core/helpers/CyclicBuffer.java

@@ -156,7 +156,10 @@ public void resize(int newSize) {
       throw new IllegalArgumentException("Negative array size [" + newSize
           + "] not allowed.");
     }
-    if (newSize == numElems)
+    // Upstream compares with numElems here and in the loop below, which ignores a
+    // resize to the current length and wraps a partially filled buffer at the wrong
+    // index; the capacity (maxSize) is what both checks are about.
+    if (newSize == maxSize)
       return; // nothing to do
 
     //
@@ -167,7 +170,7 @@ public void resize(int newSize) {
     for (int i = 0; i < loopLen; i++) {
       temp[i] = ea[first];
       ea[first] = null;
-      if (++first == numElems)
+      if (++first == maxSize)
         first = 0;
     }
     ea = temp;

logback-android/src/main/java/ch/qos/logback/core/joran/action/AbstractIncludeAction.java

@@ -20,7 +20,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.MalformedURLException;
-import java.net.URI;
 import java.net.URL;
 import java.net.UnknownHostException;
 
@@ -69,7 +68,7 @@ public void begin(InterpretationContext ec, String name, Attributes attributes)
       if (url != null) {
         processInclude(ec, url);
       }
-    } catch (JoranException e) {
+    } catch (JoranException | MalformedURLException e) {
       optionalWarning("Error while parsing " + attributeInUse, e);
     }
 
@@ -140,21 +139,15 @@ private URL resourceAsURL(String resourceAttribute) {
       return url;
   }
 
-  private URL filePathAsURL(String path) {
+  private URL filePathAsURL(String path) throws MalformedURLException {
     File file = new File(path);
     if (!file.exists() || !file.isFile()) {
       optionalWarning("File does not exist [" + path + "]", new FileNotFoundException(path));
       return null;
     }
 
-    URI uri = file.toURI();
-    try {
-      return uri.toURL();
-    } catch (MalformedURLException e) {
-      // impossible to get here
-      e.printStackTrace();
-      return null;
-    }
+    // cannot throw: a file's URI always has the "file" scheme, whose URL handler exists
+    return file.toURI().toURL();
   }
 
   protected String getAttributeInUse() {
@@ -165,7 +158,7 @@ protected boolean isOptional() {
     return this.optional;
   }
 
-  private URL getInputURL(InterpretationContext ec, Attributes attributes) {
+  private URL getInputURL(InterpretationContext ec, Attributes attributes) throws MalformedURLException {
     String fileAttribute = attributes.getValue(FILE_ATTR);
     String urlAttribute = attributes.getValue(URL_ATTR);
     String resourceAttribute = attributes.getValue(RESOURCE_ATTR);