test: 100% Kotlin test coverage of the Android layer; fix regressions from #388

.github/copilot-instructions.md

@@ -14,8 +14,11 @@ sockets, syslog, email).
   range without guarding them.
 - **Java level:** Match the source/target compatibility already set in the Gradle
   build. Do not introduce language features above that level.
-- **Tests:** JUnit tests live under `logback-android/src/test/java`. Run with
-  `./gradlew test`.
+- **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.
 - **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,6 +92,19 @@ 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
+
+      - 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/
+
       - name: Upload test reports
         if: failure()
         uses: actions/upload-artifact@v7

logback-android/build.gradle

@@ -1,10 +1,12 @@
 apply plugin: 'com.android.library'
-// Compiles src/main/kotlin. Applied as the standalone plugin (AGP's
-// built-in Kotlin support is opted out in gradle.properties) because this
-// project needs per-flavor Kotlin bytecode targets; see the afterEvaluate
-// block below.
+// Compiles src/main/kotlin and src/test/kotlin. Applied as the standalone
+// plugin (AGP's built-in Kotlin support is opted out in gradle.properties)
+// because this project needs per-flavor Kotlin bytecode targets; see the
+// afterEvaluate block below.
 apply plugin: 'org.jetbrains.kotlin.android'
 apply plugin: 'org.gradle.test-retry'
+// Unit-test coverage; see the coverage tasks at the end of this file.
+apply plugin: 'jacoco'
 
 kotlin {
     // Library mode: every public declaration in the Kotlin sources must
@@ -60,17 +62,29 @@ 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.
+            enableUnitTestCoverage true
         }
     }
     lint {
         checkAllWarnings = true
         lintConfig = rootProject.file('gradle/lint.xml')
     }
+    testCoverage {
+        jacocoVersion = '0.8.15'
+    }
     testOptions {
         unitTests {
             includeAndroidResources = true
 
             all {
+                jacoco {
+                    // Robolectric's sandbox classloader defines the classes
+                    // under test without a code-source location; record them.
+                    includeNoLocationClasses = true
+                    excludes = ['jdk.internal.*']
+                }
                 testLogging {
                     events 'failed'
                     showStackTraces = true
@@ -123,6 +137,52 @@ 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 ->
+    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 })
+    task.sourceDirectories.from(files('src/main/kotlin', 'src/main/java'))
+}
+def androidLayerVerifications = ['Jdk11Debug', 'Jdk8Debug'].collect { variant ->
+    tasks.register("androidLayerCoverageReport${variant}", JacocoReport) {
+        group = 'verification'
+        description = "Reports the ${variant} unit tests' coverage of the Android-specific layer."
+        androidLayerCoverage(it, variant)
+        reports {
+            html.required = true
+            xml.required = true
+        }
+    }
+    tasks.register("androidLayerCoverageVerification${variant}", JacocoCoverageVerification) {
+        group = 'verification'
+        description = "Fails unless the ${variant} unit tests cover every line and branch of the Android-specific layer."
+        androidLayerCoverage(it, variant)
+        violationRules {
+            rule {
+                element = 'CLASS'
+                limit { counter = 'LINE'; value = 'COVEREDRATIO'; minimum = 1.0 }
+                limit { counter = 'BRANCH'; value = 'COVEREDRATIO'; minimum = 1.0 }
+            }
+        }
+    }
+}
+tasks.register('verifyAndroidLayerCoverage') {
+    group = 'verification'
+    description = 'Fails unless the unit tests cover every line and branch of the Android-specific layer.'
+    dependsOn androidLayerVerifications
+}
+
 dependencies {
 
     testImplementation('junit:junit:4.13.2') {
@@ -134,6 +194,10 @@ dependencies {
     // Mockito 5 uses the inline mock maker by default, which is required to
     // mock final/JDK classes on JDK 17+ (older versions fail with NPEs).
     testImplementation 'org.mockito:mockito-core:5.24.0'
+    // Kotlin unit tests: kotlin.test assertions (versioned by the Kotlin
+    // plugin) and Mockito's Kotlin API
+    testImplementation 'org.jetbrains.kotlin:kotlin-test-junit'
+    testImplementation 'org.mockito.kotlin:mockito-kotlin:6.4.0'
     testImplementation 'joda-time:joda-time:2.14.4'
 
     testImplementation 'com.icegreen:greenmail:2.1.14'

logback-android/src/main/kotlin/ch/qos/logback/classic/android/LogcatAppender.kt

@@ -46,7 +46,7 @@ public open class LogcatAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * [LayoutWrappingEncoder] with a layout is accepted (issue #376).
      */
     @set:DefaultClass(PatternLayoutEncoder::class)
-    public var encoder: LayoutWrappingEncoder<ILoggingEvent>? = null
+    public open var encoder: LayoutWrappingEncoder<ILoggingEvent>? = null
 
     /**
      * The layout-wrapping encoder for this appender's *logcat* tag
@@ -66,15 +66,15 @@ public open class LogcatAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * (in this case: `f.f.foo.foo.bar.Test`).
      */
     @set:DefaultClass(PatternLayoutEncoder::class)
-    public var tagEncoder: LayoutWrappingEncoder<ILoggingEvent>? = null
+    public open var tagEncoder: LayoutWrappingEncoder<ILoggingEvent>? = null
 
     /**
      * Whether to ask Android before logging a message with a specific
      * tag and priority (i.e., calls `android.util.Log.isLoggable`).
      *
      * See [Log.isLoggable](https://developer.android.com/reference/android/util/Log#isLoggable(java.lang.String,%20int))
      */
-    public var checkLoggable: Boolean = false
+    public open var checkLoggable: Boolean = false
 
     /**
      * Checks that required parameters are set, and if everything is in order,
@@ -168,14 +168,16 @@ public open class LogcatAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * Gets the logcat tag string of a logging event
      *
      * @param event logging event to evaluate
-     * @return the tag string, truncated if max length exceeded
+     * @return the tag string, truncated if max length exceeded; or `null` if
+     * the event has no logger name (and there is no tag encoder)
      */
-    protected open fun getTag(event: ILoggingEvent): String {
+    protected open fun getTag(event: ILoggingEvent): String? {
         // format tag based on encoder layout; truncate if max length
         // exceeded (only necessary for isLoggable(), which throws
-        // IllegalArgumentException)
-        val tag = this.tagEncoder?.layout?.doLayout(event) ?: event.loggerName
-        return if (checkLoggable && tag.length > MAX_TAG_LENGTH) {
+        // IllegalArgumentException). The tag is null for an event without a
+        // logger name, which logcat accepts.
+        val tag: String? = this.tagEncoder?.layout?.doLayout(event) ?: event.loggerName
+        return if (checkLoggable && tag != null && tag.length > MAX_TAG_LENGTH) {
             "${tag.substring(0, MAX_TAG_LENGTH - 1)}*"
         } else {
             tag

logback-android/src/main/kotlin/ch/qos/logback/classic/android/SQLiteAppender.kt

@@ -40,7 +40,7 @@ import ch.qos.logback.core.util.Duration
  */
 public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
 
-    private var db: SQLiteDatabase? = null
+    private lateinit var db: SQLiteDatabase
     private lateinit var insertPropertiesSQL: String
     private lateinit var insertExceptionSQL: String
     private lateinit var insertSQL: String
@@ -52,17 +52,17 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * The database name resolver, used to customize the names of the
      * table names and columns in the database.
      */
-    public var dbNameResolver: DBNameResolver? = null
+    public open var dbNameResolver: DBNameResolver? = null
 
     /**
      * The absolute path to the SQLite database
      */
-    public var filename: String? = null
+    public open var filename: String? = null
 
     /**
      * The maximum history in time duration (e.g., "1 day") of records to keep
      */
-    public var maxHistory: String
+    public open var maxHistory: String
         get() = maxHistoryDuration?.toString() ?: ""
         set(value) {
             maxHistoryDuration = Duration.valueOf(value)
@@ -71,15 +71,15 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
     /**
      * The maximum history in milliseconds
      */
-    public val maxHistoryMs: Long
+    public open val maxHistoryMs: Long
         get() = maxHistoryDuration?.milliseconds ?: 0
 
     /**
      * The [SQLiteLogCleaner] invoked when [maxHistory] is exceeded at
      * startup and in between logging events. Reading this property creates
      * the default log cleaner if none was set.
      */
-    public var logCleaner: SQLiteLogCleaner? = null
+    public open var logCleaner: SQLiteLogCleaner? = null
         get() {
             if (field == null) {
                 field = SQLiteLogCleaner { db, expiry ->
@@ -101,7 +101,7 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * @param filename absolute path to database file
      * @return the file object if a valid file found; otherwise, null
      */
-    public fun getDatabaseFile(filename: String?): File? {
+    public open fun getDatabaseFile(filename: String?): File? {
         var dbFile: File? = null
         if (!filename.isNullOrBlank()) {
             dbFile = File(filename)
@@ -122,7 +122,7 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
         }
 
         val db = try {
-            dbFile.parentFile?.mkdirs()
+            dbFile.absoluteFile.parentFile.mkdirs()
             addInfo("db path: ${dbFile.absolutePath}")
             SQLiteDatabase.openOrCreateDatabase(dbFile.path, null)
         } catch (e: SQLiteException) {
@@ -155,15 +155,16 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
     }
 
     override fun stop() {
-        db?.close()
+        if (this::db.isInitialized) {
+            db.close()
+        }
         this.lastCleanupTime = 0
     }
 
     public override fun append(eventObject: ILoggingEvent) {
         if (!isStarted) {
             return
         }
-        val db = this.db ?: return
 
         try {
             clearExpiredLogs(db)
@@ -190,10 +191,10 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * Removes expired logs from the database
      */
     private fun clearExpiredLogs(db: SQLiteDatabase) {
-        val maxHistory = this.maxHistoryDuration
+        val maxHistory = this.maxHistoryDuration ?: return
         if (lastCheckExpired(maxHistory, this.lastCleanupTime)) {
             this.lastCleanupTime = clock.currentTimeMillis()
-            logCleaner?.performLogCleanup(db, maxHistory!!)
+            logCleaner?.performLogCleanup(db, maxHistory)
         }
     }
 
@@ -204,8 +205,8 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
      * @param lastCleanupTime timestamp (ms) of last cleanup
      * @return true if last check has expired
      */
-    private fun lastCheckExpired(expiry: Duration?, lastCleanupTime: Long): Boolean {
-        if (expiry == null || expiry.milliseconds <= 0) {
+    private fun lastCheckExpired(expiry: Duration, lastCleanupTime: Long): Boolean {
+        if (expiry.milliseconds <= 0) {
             return false
         }
         val timeDiff = clock.currentTimeMillis() - lastCleanupTime
@@ -319,7 +320,6 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
         if (mergedMap.isEmpty()) {
             return
         }
-        val db = this.db ?: return
         db.compileStatement(insertPropertiesSQL).use { stmt ->
             for ((key, value) in mergedMap) {
                 stmt.bindLong(1, eventId)
@@ -361,7 +361,6 @@ public open class SQLiteAppender : UnsynchronizedAppenderBase<ILoggingEvent>() {
     }
 
     private fun insertThrowable(throwableProxy: IThrowableProxy, eventId: Long) {
-        val db = this.db ?: return
         db.compileStatement(insertExceptionSQL).use { stmt ->
             var tp: IThrowableProxy? = throwableProxy
             var baseIndex: Short = 0