From 614550749ae864421d104ac78d194b3807055ce5 Mon Sep 17 00:00:00 2001 From: Gabriel Feo Date: Thu, 5 Sep 2024 13:21:11 -0400 Subject: [PATCH 1/2] Fix Config.logLevel precedence and javadoc (cherry picked from commit 2717999ee67dfd82f229d3d3f8366c743a0afc2b) --- .../com/gabrielfeo/develocity/api/Config.kt | 27 +++++++++++++------ .../develocity/api/internal/LoggerFactory.kt | 9 +++---- .../api/internal/SystemProperties.kt | 6 +++-- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt b/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt index 57cac4b4..89530e13 100644 --- a/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt +++ b/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt @@ -14,12 +14,15 @@ import kotlin.time.Duration.Companion.days data class Config( /** - * Changes the default log level for library classes, such as the HTTP client. By default, - * log level is the value of - * [org.slf4j.simpleLogger.defaultLogLevel](https://www.slf4j.org/api/org/slf4j/simple/SimpleLogger.html) - * system property or `"off"`. + * Changes the default log level for library classes, such as the HTTP client. Default value, by order of + * precedence: + * + * - `DEVELOCITY_API_LOG_LEVEL` environment variable + * - `org.slf4j.simpleLogger.defaultLogLevel` system property + * - `"off"` * * Possible values: + * * - "off" (default) * - "error" * - "warn" @@ -30,6 +33,7 @@ data class Config( */ val logLevel: String = env["DEVELOCITY_API_LOG_LEVEL"] + ?: systemProperties.logLevel ?: "off", /** @@ -38,7 +42,7 @@ data class Config( */ val apiUrl: String = env["DEVELOCITY_API_URL"] - ?: error("DEVELOCITY_API_URL is required"), + ?: error(ERROR_NULL_API_URL), /** * Provides the access token for a Develocity API instance. By default, uses environment @@ -46,7 +50,7 @@ data class Config( */ val apiToken: () -> String = { env["DEVELOCITY_API_TOKEN"] - ?: error("DEVELOCITY_API_TOKEN is required") + ?: error(ERROR_NULL_API_TOKEN) }, /** @@ -143,11 +147,14 @@ data class Config( /** * HTTP cache location. By default, uses environment variable `DEVELOCITY_API_CACHE_DIR` - * or the system temporary folder (`java.io.tmpdir` / develocity-api-kotlin-cache). + * or creates a `~/.develocity-api-kotlin-cache` directory. */ val cacheDir: File = env["DEVELOCITY_API_CACHE_DIR"]?.let(::File) - ?: File(systemProperties["user.home"], ".develocity-api-kotlin-cache"), + ?: let { + val userHome = checkNotNull(systemProperties.userHome) { ERROR_NULL_USER_HOME } + File(userHome, ".develocity-api-kotlin-cache"), + } /** * Max size of the HTTP cache. By default, uses environment variable @@ -204,3 +211,7 @@ data class Config( ?: 1.days.inWholeSeconds, ) } + +private const val ERROR_NULL_API_URL = "DEVELOCITY_API_URL is required" +private const val ERROR_NULL_API_TOKEN = "DEVELOCITY_API_TOKEN is required" +private const val ERROR_NULL_USER_HOME = "'user.home' system property must not be null" diff --git a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt index 47a07269..98f2c63d 100644 --- a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt +++ b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt @@ -18,11 +18,10 @@ class RealLoggerFactory( } private fun setLogLevel() { - if (System.getProperty(SIMPLE_LOGGER_LOG_LEVEL) != null) { - return - } System.setProperty(SIMPLE_LOGGER_LOG_LEVEL, config.logLevel) } -} -internal const val SIMPLE_LOGGER_LOG_LEVEL = "org.slf4j.simpleLogger.defaultLogLevel" + companion object { + const val LOG_LEVEL_SYSTEM_PROPERTY = "org.slf4j.simpleLogger.defaultLogLevel" + } +} diff --git a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/SystemProperties.kt b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/SystemProperties.kt index 4305f1db..8a4ab9e8 100644 --- a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/SystemProperties.kt +++ b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/SystemProperties.kt @@ -3,9 +3,11 @@ package com.gabrielfeo.develocity.api.internal internal var systemProperties: SystemProperties = RealSystemProperties internal interface SystemProperties { - operator fun get(name: String): String? + val userHome: String? + val logLevel: String? } internal object RealSystemProperties : SystemProperties { - override fun get(name: String): String? = System.getProperty(name) + override val userHome: String? = System.getProperty("user.home") + override val logLevel: String? = System.getProperty(RealLoggerFactory.LOG_LEVEL_SYSTEM_PROPERTY) } From d2220d4ed4dcad617f04a824e5ff60228db729a3 Mon Sep 17 00:00:00 2001 From: Gabriel Feo Date: Thu, 5 Sep 2024 14:11:41 -0400 Subject: [PATCH 2/2] Add more tests --- .../com/gabrielfeo/develocity/api/Config.kt | 6 ++--- .../develocity/api/internal/LoggerFactory.kt | 2 +- .../gabrielfeo/develocity/api/ConfigTest.kt | 21 +++++++++++++++++ .../develocity/api/internal/FakeEnv.kt | 2 -- .../develocity/api/internal/FakeKeychain.kt | 2 -- .../api/internal/FakeSystemProperties.kt | 2 -- .../api/internal/LoggerFactoryTest.kt | 23 ++++--------------- .../api/internal/FakeSystemProperties.kt | 6 +++++ 8 files changed, 36 insertions(+), 28 deletions(-) delete mode 100644 library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeEnv.kt delete mode 100644 library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeKeychain.kt delete mode 100644 library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt create mode 100644 library/src/testFixtures/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt diff --git a/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt b/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt index 89530e13..7a59a314 100644 --- a/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt +++ b/library/src/main/kotlin/com/gabrielfeo/develocity/api/Config.kt @@ -151,10 +151,10 @@ data class Config( */ val cacheDir: File = env["DEVELOCITY_API_CACHE_DIR"]?.let(::File) - ?: let { + ?: run { val userHome = checkNotNull(systemProperties.userHome) { ERROR_NULL_USER_HOME } - File(userHome, ".develocity-api-kotlin-cache"), - } + File(userHome, ".develocity-api-kotlin-cache") + }, /** * Max size of the HTTP cache. By default, uses environment variable diff --git a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt index 98f2c63d..af704510 100644 --- a/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt +++ b/library/src/main/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactory.kt @@ -18,7 +18,7 @@ class RealLoggerFactory( } private fun setLogLevel() { - System.setProperty(SIMPLE_LOGGER_LOG_LEVEL, config.logLevel) + System.setProperty(LOG_LEVEL_SYSTEM_PROPERTY, config.logLevel) } companion object { diff --git a/library/src/test/kotlin/com/gabrielfeo/develocity/api/ConfigTest.kt b/library/src/test/kotlin/com/gabrielfeo/develocity/api/ConfigTest.kt index 99fb8977..e79e91af 100644 --- a/library/src/test/kotlin/com/gabrielfeo/develocity/api/ConfigTest.kt +++ b/library/src/test/kotlin/com/gabrielfeo/develocity/api/ConfigTest.kt @@ -9,6 +9,7 @@ class ConfigTest { @BeforeTest fun before() { env = FakeEnv("DEVELOCITY_API_URL" to "https://example.com/api/") + systemProperties = FakeSystemProperties() } @Test @@ -51,4 +52,24 @@ class ConfigTest { (env as FakeEnv)["DEVELOCITY_API_READ_TIMEOUT_MILLIS"] = "100000" assertEquals(100_000L, Config().readTimeoutMillis) } + + @Test + fun `Given logLevel in env, logLevel is env value`() { + (env as FakeEnv)["DEVELOCITY_API_LOG_LEVEL"] = "trace" + assertEquals("trace", Config().logLevel) + } + + @Test + fun `Given logLevel in System props and not in env, logLevel is prop value`() { + (env as FakeEnv)["DEVELOCITY_API_LOG_LEVEL"] = null + (systemProperties as FakeSystemProperties).logLevel = "info" + assertEquals("info", Config().logLevel) + } + + @Test + fun `Given no logLevel set, logLevel is off`() { + (env as FakeEnv)["DEVELOCITY_API_LOG_LEVEL"] = null + (systemProperties as FakeSystemProperties).logLevel = null + assertEquals("off", Config().logLevel) + } } diff --git a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeEnv.kt b/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeEnv.kt deleted file mode 100644 index 6ee2553f..00000000 --- a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeEnv.kt +++ /dev/null @@ -1,2 +0,0 @@ -package com.gabrielfeo.develocity.api.internal - diff --git a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeKeychain.kt b/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeKeychain.kt deleted file mode 100644 index 6ee2553f..00000000 --- a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeKeychain.kt +++ /dev/null @@ -1,2 +0,0 @@ -package com.gabrielfeo.develocity.api.internal - diff --git a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt b/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt deleted file mode 100644 index 6ee2553f..00000000 --- a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt +++ /dev/null @@ -1,2 +0,0 @@ -package com.gabrielfeo.develocity.api.internal - diff --git a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactoryTest.kt b/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactoryTest.kt index 5b81cf94..6614dc48 100644 --- a/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactoryTest.kt +++ b/library/src/test/kotlin/com/gabrielfeo/develocity/api/internal/LoggerFactoryTest.kt @@ -8,32 +8,19 @@ import kotlin.test.assertEquals class LoggerFactoryTest { + private val logLevelProperty = RealLoggerFactory.LOG_LEVEL_SYSTEM_PROPERTY + @BeforeTest @AfterTest fun cleanup() { - System.clearProperty(SIMPLE_LOGGER_LOG_LEVEL) + System.clearProperty(logLevelProperty) env = FakeEnv("DEVELOCITY_API_URL" to "https://example.com/") } @Test - fun `Logging off by default`() { - val loggerFactory = RealLoggerFactory(Config()) - loggerFactory.newLogger(LoggerFactoryTest::class) - assertEquals("off", System.getProperty(SIMPLE_LOGGER_LOG_LEVEL)) - } - - @Test - fun `Pre-existing defaultLogLevel is honored`() { - val loggerFactory = RealLoggerFactory(Config(logLevel = "bar")) - System.setProperty(SIMPLE_LOGGER_LOG_LEVEL, "foo") - loggerFactory.newLogger(LoggerFactoryTest::class) - assertEquals("foo", System.getProperty(SIMPLE_LOGGER_LOG_LEVEL)) - } - - @Test - fun `Logging can be set from config`() { + fun `Level always copied from`() { val loggerFactory = RealLoggerFactory(Config(logLevel = "foo")) loggerFactory.newLogger(LoggerFactoryTest::class) - assertEquals("foo", System.getProperty(SIMPLE_LOGGER_LOG_LEVEL)) + assertEquals("foo", System.getProperty(logLevelProperty)) } } diff --git a/library/src/testFixtures/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt b/library/src/testFixtures/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt new file mode 100644 index 00000000..54f2d7b1 --- /dev/null +++ b/library/src/testFixtures/kotlin/com/gabrielfeo/develocity/api/internal/FakeSystemProperties.kt @@ -0,0 +1,6 @@ +package com.gabrielfeo.develocity.api.internal + +data class FakeSystemProperties( + override var userHome: String? = System.getProperty("java.io.tmpdir"), + override var logLevel: String? = null, +) : SystemProperties