From 868e551ba90f1d30d89052089e42ce14e1096f7c Mon Sep 17 00:00:00 2001 From: Timo T Date: Sun, 5 Jun 2022 16:57:16 +0200 Subject: [PATCH] Fix test logging not using format specifiers for arbitrary text (#7073) --- core/src/com/unciv/ui/utils/Log.kt | 27 ++++++++++++------- tests/src/com/unciv/testing/BasicTests.kt | 26 +++++++++--------- .../com/unciv/testing/SerializationTests.kt | 2 +- .../src/com/unciv/testing/TranslationTests.kt | 12 ++++----- 4 files changed, 38 insertions(+), 29 deletions(-) diff --git a/core/src/com/unciv/ui/utils/Log.kt b/core/src/com/unciv/ui/utils/Log.kt index 7280d17282..d15f0736c2 100644 --- a/core/src/com/unciv/ui/utils/Log.kt +++ b/core/src/com/unciv/ui/utils/Log.kt @@ -34,7 +34,9 @@ object Log { } /** - * Logs the given message by [java.lang.String.format]ting them with an optional list of [params]. + * Logs the given message by [String.format][java.lang.String.format]ting them with an optional list of [params]. Since this uses + * [format specifiers][java.util.Formatter], make sure your message only contains `%` characters correctly used as [format specifiers][java.util.Formatter] + * and never do manual string concatenation, only use [params] to log arbitrary text that could contain `%` characters. * * Only actually does something when logging is enabled. * @@ -48,7 +50,9 @@ object Log { } /** - * Logs the given message by [java.lang.String.format]ting them with an optional list of [params]. + * Logs the given message by [String.format][java.lang.String.format]ting them with an optional list of [params]. Since this uses + * [format specifiers][java.util.Formatter], make sure your message only contains `%` characters correctly used as [format specifiers][java.util.Formatter] + * and never do manual string concatenation, only use [params] to log arbitrary text that could contain `%` characters. * * Only actually does something when logging is enabled. * @@ -61,7 +65,7 @@ object Log { } /** - * Logs the given [throwable] by appending it to [msg] + * Logs the given [throwable] by appending it to [msg]. * * Only actually does something when logging is enabled. * @@ -72,7 +76,7 @@ object Log { debug(getTag(), msg, throwable) } /** - * Logs the given [throwable] by appending it to [msg] + * Logs the given [throwable] by appending it to [msg]. * * Only actually does something when logging is enabled. */ @@ -82,7 +86,9 @@ object Log { } /** - * Logs the given error message by [java.lang.String.format]ting them with an optional list of [params]. + * Logs the given message by [String.format][java.lang.String.format]ting them with an optional list of [params]. Since this uses + * [format specifiers][java.util.Formatter], make sure your message only contains `%` characters correctly used as [format specifiers][java.util.Formatter] + * and never do manual string concatenation, only use [params] to log arbitrary text that could contain `%` characters. * * Always logs, even in release builds. * @@ -96,7 +102,9 @@ object Log { /** - * Logs the given error message by [java.lang.String.format]ting them with an optional list of [params]. + * Logs the given message by [String.format][java.lang.String.format]ting them with an optional list of [params]. Since this uses + * [format specifiers][java.util.Formatter], make sure your message only contains `%` characters correctly used as [format specifiers][java.util.Formatter] + * and never do manual string concatenation, only use [params] to log arbitrary text that could contain `%` characters. * * Always logs, even in release builds. * @@ -108,7 +116,7 @@ object Log { } /** - * Logs the given [throwable] by appending it to [msg] + * Logs the given [throwable] by appending it to [msg]. * * Always logs, even in release builds. * @@ -118,7 +126,7 @@ object Log { error(getTag(), msg, throwable) } /** - * Logs the given [throwable] by appending it to [msg] + * Logs the given [throwable] by appending it to [msg]. * * Always logs, even in release builds. */ @@ -173,7 +181,8 @@ fun debug(tag: Tag, msg: String, throwable: Throwable) { } private fun doLog(logger: (Tag, String, String) -> Unit, tag: Tag, msg: String, vararg params: Any?) { - logger(tag, Thread.currentThread().name, msg.format(*params)) + val formattedMessage = if (params.isEmpty()) msg else msg.format(*params) + logger(tag, Thread.currentThread().name, formattedMessage) } private fun isTagDisabled(tag: Tag): Boolean { diff --git a/tests/src/com/unciv/testing/BasicTests.kt b/tests/src/com/unciv/testing/BasicTests.kt index 062dca1931..f8ba2cbcba 100644 --- a/tests/src/com/unciv/testing/BasicTests.kt +++ b/tests/src/com/unciv/testing/BasicTests.kt @@ -68,7 +68,7 @@ class BasicTests { var allObsoletingUnitsHaveUpgrades = true for (unit in units) { if (unit.obsoleteTech != null && unit.upgradesTo == null && unit.name !="Scout" ) { - debug(unit.name + " obsoletes but has no upgrade") + debug("%s obsoletes but has no upgrade", unit.name) allObsoletingUnitsHaveUpgrades = false } } @@ -94,7 +94,7 @@ class BasicTests { val ruleset = RulesetCache[baseRuleset.fullName]!! val modCheck = ruleset.checkModLinks() if (modCheck.isNotOK()) - debug(modCheck.getErrorText(true)) + debug("%s", modCheck.getErrorText(true)) Assert.assertFalse(modCheck.isNotOK()) } } @@ -107,7 +107,7 @@ class BasicTests { for (paramType in entry.value) { if (paramType == UniqueParameterType.Unknown) { val badParam = uniqueType.text.getPlaceholderParameters()[entry.index] - debug("${uniqueType.name} param[${entry.index}] type \"$badParam\" is unknown") + debug("%s param[%s] type \"%s\" is unknown", uniqueType.name, entry.index, badParam) noUnknownParameters = false } } @@ -121,7 +121,7 @@ class BasicTests { var allOK = true for (uniqueType in UniqueType.values()) { if (uniqueType.targetTypes.isEmpty()) { - debug("${uniqueType.name} has no targets.") + debug("%s has no targets.", uniqueType.name) allOK = false } } @@ -135,7 +135,7 @@ class BasicTests { for (unit in units) { for (unique in unit.uniques) { if (!UniqueType.values().any { it.placeholderText == unique.getPlaceholderText() }) { - debug("${unit.name}: $unique") + debug("%s: %s", unit.name, unique) allOK = false } } @@ -150,7 +150,7 @@ class BasicTests { for (building in buildings) { for (unique in building.uniques) { if (!UniqueType.values().any { it.placeholderText == unique.getPlaceholderText() }) { - debug("${building.name}: $unique") + debug("%s: %s", building.name, unique) allOK = false } } @@ -165,7 +165,7 @@ class BasicTests { for (promotion in promotions) { for (unique in promotion.uniques) { if (!UniqueType.values().any { it.placeholderText == unique.getPlaceholderText() }) { - debug("${promotion.name}: $unique") + debug("%s: %s", promotion.name, unique) allOK = false } } @@ -183,7 +183,7 @@ class BasicTests { for (obj in objects) { for (unique in obj.uniques) { if (!UniqueType.values().any { it.placeholderText == unique.getPlaceholderText() }) { - debug("${obj.name}: $unique") + debug("%s: %s", obj.name, unique) allOK = false } } @@ -265,16 +265,16 @@ class BasicTests { if (replacementTextUnique.type == null) { - debug("${uniqueType.name}'s deprecation text \"$uniqueText\" does not match any existing type!") + debug("%s's deprecation text \"%s\" does not match any existing type!", uniqueType.name, uniqueText) allOK = false } if (replacementTextUnique.type == uniqueType) { - debug("${uniqueType.name}'s deprecation text references itself!") + debug("%s's deprecation text references itself!", uniqueType.name) allOK = false } for (conditional in replacementTextUnique.conditionals) { if (conditional.type == null) { - debug("${uniqueType.name}'s deprecation text contains conditional \"${conditional.text}\" which does not match any existing type!") + debug("%s's deprecation text contains conditional \"%s\" which does not match any existing type!", uniqueType.name, conditional.text) allOK = false } } @@ -284,7 +284,7 @@ class BasicTests { while (replacementUnique.getDeprecationAnnotation() != null) { if (iteration == 10) { allOK = false - debug("${uniqueType.name}'s deprecation text never references an undeprecated unique!") + debug("%s's deprecation text never references an undeprecated unique!", uniqueType.name) break } iteration++ @@ -302,7 +302,7 @@ class BasicTests { Thread.sleep(5000) // makes timings a little more repeatable val startTime = System.nanoTime() statMathRunner(iterations = 1_000_000) - debug("statMathStressTest took ${(System.nanoTime()-startTime)/1000}µs") + debug("statMathStressTest took %sµs", (System.nanoTime()-startTime) / 1000) } @Test diff --git a/tests/src/com/unciv/testing/SerializationTests.kt b/tests/src/com/unciv/testing/SerializationTests.kt index 3d7ae83cf6..b9b7ceb3cc 100644 --- a/tests/src/com/unciv/testing/SerializationTests.kt +++ b/tests/src/com/unciv/testing/SerializationTests.kt @@ -112,7 +112,7 @@ class SerializationTests { val pattern = """\{(\w+)\${'$'}delegate:\{class:kotlin.SynchronizedLazyImpl,""" val matches = Regex(pattern).findAll(json) matches.forEach { - debug("Lazy missing `@delegate:Transient` annotation: " + it.groups[1]!!.value) + debug("Lazy missing `@delegate:Transient` annotation: %s", it.groups[1]!!.value) } val result = matches.any() Assert.assertFalse("This test will only pass when no serializable lazy fields are found", result) diff --git a/tests/src/com/unciv/testing/TranslationTests.kt b/tests/src/com/unciv/testing/TranslationTests.kt index d6386aeaaf..0ef51734c6 100644 --- a/tests/src/com/unciv/testing/TranslationTests.kt +++ b/tests/src/com/unciv/testing/TranslationTests.kt @@ -96,7 +96,7 @@ class TranslationTests { for (placeholder in placeholders) { if (!output.contains(placeholder)) { allTranslationsHaveCorrectPlaceholders = false - debug("Placeholder `$placeholder` not found in `$language` for entry `$translationEntry`") + debug("Placeholder `%s` not found in `%s` for entry `%s`", placeholder, language, translationEntry) } } } @@ -116,7 +116,7 @@ class TranslationTests { val keyFromEntry = translationEntry.replace(squareBraceRegex, "[]") if (key != keyFromEntry) { allPlaceholderKeysMatchEntry = false - debug("Entry $translationEntry found under bad key $key") + debug("Entry %s found under bad key %s", translationEntry, key) break } } @@ -136,7 +136,7 @@ class TranslationTests { for (placeholder in placeholders) if (placeholders.count { it == placeholder } > 1) { noTwoPlaceholdersAreTheSame = false - debug("Entry $translationEntry has the parameter $placeholder more than once") + debug("Entry %s has the parameter %s more than once", translationEntry, placeholder) break } } @@ -152,7 +152,7 @@ class TranslationTests { var failed = false for (line in templateLines) { if (line.endsWith(" =")) { - debug("$line ends without a space at the end") + debug("%s ends without a space at the end", line) failed = true } } @@ -177,7 +177,7 @@ class TranslationTests { translationEntry.entry.tr() } catch (ex: Exception) { allWordsTranslatedCorrectly = false - debug("Crashed when translating ${translationEntry.entry} to $language") + debug("Crashed when translating %s to %s", translationEntry.entry, language) } } } @@ -198,7 +198,7 @@ class TranslationTests { || translation.count { it == '\"' } != 2 ) { allTranslationsCheckedOut = false - debug("Translation of the word boundary in $language was incorrectly formatted") + debug("Translation of the word boundary in %s was incorrectly formatted", language) } }