From 726a1942db071db542a3c992cb945bebc3ffcc83 Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Tue, 5 Oct 2021 08:08:41 +0200 Subject: [PATCH] Sharpen unit tests for UniqueTypes (and mod checker too) (#5398) * Sharpen unit tests for UniqueTypes (and mod checker too) * Sharpen unit tests for UniqueTypes - fix newly found problems --- .../jsons/Civ V - Vanilla/UnitPromotions.json | 6 +- core/src/com/unciv/models/ruleset/Ruleset.kt | 67 ++++++++++++++++--- .../ruleset/unique/UniqueParameterType.kt | 57 +++++++++++++--- .../unciv/models/ruleset/unique/UniqueType.kt | 20 +++--- tests/src/com/unciv/testing/BasicTests.kt | 37 +++++++++- 5 files changed, 152 insertions(+), 35 deletions(-) diff --git a/android/assets/jsons/Civ V - Vanilla/UnitPromotions.json b/android/assets/jsons/Civ V - Vanilla/UnitPromotions.json index 698bce9123..2df2e5d1e7 100644 --- a/android/assets/jsons/Civ V - Vanilla/UnitPromotions.json +++ b/android/assets/jsons/Civ V - Vanilla/UnitPromotions.json @@ -206,7 +206,7 @@ { "name": "Boarding Party III", "prerequisites": ["Boarding Party II"], - "uniques": ["+[15]% Strength "], + "uniques": ["[+15]% Strength "], "unitTypes": ["Melee Water"] }, @@ -375,7 +375,7 @@ { "name": "Air Targeting I", - "prerequisites": ["Interception I","Dogfighting I", "Siege I","Bombardment I"], + "prerequisites": ["Interception I","Siege I","Bombardment I"], // "Dogfighting I" "uniques": ["[+33]% Strength "], "unitTypes": ["Fighter","Bomber"] }, @@ -388,7 +388,7 @@ { "name": "Sortie", - "prerequisites": ["Interception II", "Dogfighting II"], + "prerequisites": ["Interception II"], // "Dogfighting II" "uniques": ["[1] extra interceptions may be made per turn"], "unitTypes": ["Fighter"] }, diff --git a/core/src/com/unciv/models/ruleset/Ruleset.kt b/core/src/com/unciv/models/ruleset/Ruleset.kt index 9cfe60e591..b4c53ee814 100644 --- a/core/src/com/unciv/models/ruleset/Ruleset.kt +++ b/core/src/com/unciv/models/ruleset/Ruleset.kt @@ -80,7 +80,7 @@ class Ruleset { val units = LinkedHashMap() val unitPromotions = LinkedHashMap() val unitTypes = LinkedHashMap() - + val mods = LinkedHashSet() var modOptions = ModOptions() @@ -302,6 +302,11 @@ class Ruleset { " ${complianceError.acceptableParameterTypes.joinToString(" or ") { it.parameterName }} !" } + if (severityToReport != UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + // If we don't filter these messages will be listed twice as this function is called twice on most objects + // The tests are RulesetInvariant in nature, but RulesetSpecific is called for _all_ objects, invariant is not. + continue + val deprecationAnnotation = unique.type.declaringClass.getField(unique.type.name) .getAnnotation(Deprecated::class.java) if (deprecationAnnotation != null) { @@ -355,10 +360,10 @@ class Ruleset { /** @return `true` means at least errors impacting gameplay exist, new game screen should warn or block */ fun isWarnUser() = getFinalSeverity() >= RulesetErrorSeverity.Warning - fun getErrorText() = - filter { it.errorSeverityToReport != RulesetErrorSeverity.WarningOptionsOnly } + fun getErrorText(unfiltered: Boolean = false) = + filter { unfiltered || it.errorSeverityToReport != RulesetErrorSeverity.WarningOptionsOnly } .sortedByDescending { it.errorSeverityToReport } - .joinToString { it.errorSeverityToReport.name + ": " + it.text } + .joinToString("\n") { it.errorSeverityToReport.name + ": " + it.text } } fun checkModLinks(): RulesetErrorList { @@ -403,15 +408,25 @@ class Ruleset { checkUniques(nation, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant) } - for (promotion in unitPromotions.values) + for (promotion in unitPromotions.values) { if (promotion.effect != "") - lines.add("`Promotion.effect` used in ${promotion.name} is deprecated, please use `uniques` instead", - RulesetErrorSeverity.WarningOptionsOnly) + lines.add( + "`Promotion.effect` used in ${promotion.name} is deprecated, please use `uniques` instead", + RulesetErrorSeverity.WarningOptionsOnly + ) - for (resource in tileResources.values) + checkUniques(promotion, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant) + } + + for (resource in tileResources.values) { if (resource.unique != null) - lines.add("`Resource.unique` used in ${resource.name} is deprecated, please use `uniques` instead", - RulesetErrorSeverity.WarningOptionsOnly) + lines.add( + "`Resource.unique` used in ${resource.name} is deprecated, please use `uniques` instead", + RulesetErrorSeverity.WarningOptionsOnly + ) + + checkUniques(resource, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant) + } // Quit here when no base ruleset is loaded - references cannot be checked if (!modOptions.isBaseRuleset) return lines @@ -544,6 +559,38 @@ class Ruleset { checkUniques(era, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) } + for (belief in beliefs.values) { + checkUniques(belief, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + for (nation in nations.values) { + checkUniques(nation, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + for (policy in policies.values) { + if (policy.requires != null) + for (prereq in policy.requires!!) + if (!policies.containsKey(prereq)) + lines += "${policy.name} requires policy $prereq which does not exist!" + checkUniques(policy, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + for (reward in ruinRewards.values) { + for (difficulty in reward.excludedDifficulties) + if (!difficulties.containsKey(difficulty)) + lines += "${reward.name} references difficulty ${difficulty}, which does not exist!" + checkUniques(reward, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + for (promotion in unitPromotions.values) { + for (prereq in promotion.prerequisites) + if (!unitPromotions.containsKey(prereq)) + lines += "${promotion.name} requires promotion $prereq which does not exist!" + for (unitType in promotion.unitTypes) + if (!unitTypes.containsKey(unitType) && (unitTypes.isNotEmpty() || !baseRuleset.unitTypes.containsKey(unitType))) + lines += "${promotion.name} references unit type ${unitType}, which does not exist!" + checkUniques(promotion, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + for (unitType in unitTypes.values) { + checkUniques(unitType, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) + } + return lines } } diff --git a/core/src/com/unciv/models/ruleset/unique/UniqueParameterType.kt b/core/src/com/unciv/models/ruleset/unique/UniqueParameterType.kt index 9eef0b6107..5fc8cc9022 100644 --- a/core/src/com/unciv/models/ruleset/unique/UniqueParameterType.kt +++ b/core/src/com/unciv/models/ruleset/unique/UniqueParameterType.kt @@ -1,5 +1,6 @@ package com.unciv.models.ruleset.unique +import com.unciv.models.ruleset.BeliefType import com.unciv.models.ruleset.Ruleset import com.unciv.models.ruleset.tile.ResourceType import com.unciv.models.ruleset.tile.TerrainType @@ -20,7 +21,15 @@ enum class UniqueParameterType(val parameterName:String) { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset) = if (parameterText == "All") null else UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant }, - MapUnitFilter("mapUnitFilter"){ + CombatantFilter("combatantFilter") { + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): + UniqueType.UniqueComplianceErrorSeverity? { + if (parameterText == "City") return null + return MapUnitFilter.getErrorSeverity(parameterText, ruleset) + } + + }, + MapUnitFilter("mapUnitFilter") { private val knownValues = setOf("Wounded", "Barbarians", "City-State", "Embarked", "Non-City") override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? { @@ -32,7 +41,7 @@ enum class UniqueParameterType(val parameterName:String) { return BaseUnitFilter.getErrorSeverity(parameterText, ruleset) } }, - BaseUnitFilter("baseUnitFilter"){ + BaseUnitFilter("baseUnitFilter") { // As you can see there is a difference between these and what's in unitTypeStrings (for translation) - // the goal is to unify, but for now this is the "real" list private val knownValues = setOf("All", "Melee", "Ranged", "Civilian", "Military", "Land", "Water", "Air", @@ -49,7 +58,7 @@ enum class UniqueParameterType(val parameterName:String) { return UniqueType.UniqueComplianceErrorSeverity.WarningOnly } }, - Stats("stats"){ + Stats("stats") { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? { if (!com.unciv.models.stats.Stats.isStats(parameterText)) @@ -57,14 +66,14 @@ enum class UniqueParameterType(val parameterName:String) { return null } }, - StatName("stat"){ + StatName("stat") { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? { if (Stat.values().any { it.name == parameterText }) return null return UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant } }, - CityFilter("cityFilter"){ + CityFilter("cityFilter") { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? { if (parameterText !in cityFilterStrings) @@ -114,6 +123,14 @@ enum class UniqueParameterType(val parameterName:String) { return UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific } }, + /** Used by NaturalWonderGenerator, only tests base terrain */ + BaseTerrain("baseTerrain") { + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): + UniqueType.UniqueComplianceErrorSeverity? { + if (ruleset.terrains[parameterText]?.type?.isBaseTerrain == true) return null + return UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific + } + }, Promotion("promotion") { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? = when (parameterText) { @@ -122,10 +139,32 @@ enum class UniqueParameterType(val parameterName:String) { } }, Era("era") { + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): + UniqueType.UniqueComplianceErrorSeverity? = when (parameterText) { + in ruleset.eras -> null + else -> UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific + } + }, + Resource("resource") { + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): + UniqueType.UniqueComplianceErrorSeverity? = when (parameterText) { + in ruleset.tileResources -> null + else -> UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific + } + }, + BeliefTypeName("beliefType") { + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): + UniqueType.UniqueComplianceErrorSeverity? = when (parameterText) { + in BeliefType.values().map { it.name } -> null + else -> UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant + } + }, + FoundingOrEnhancing("foundingOrEnhancing") { + private val knownValues = setOf("founding", "enhancing") override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? = when (parameterText) { - in ruleset.eras -> null - else -> UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific + in knownValues -> null + else -> UniqueType.UniqueComplianceErrorSeverity.RulesetInvariant } }, /** Behaves like [Unknown], but states explicitly the parameter is OK and its contents are ignored */ @@ -135,9 +174,7 @@ enum class UniqueParameterType(val parameterName:String) { }, Unknown("param") { override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): - UniqueType.UniqueComplianceErrorSeverity? { - return null - } + UniqueType.UniqueComplianceErrorSeverity? = null }; abstract fun getErrorSeverity(parameterText:String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity? diff --git a/core/src/com/unciv/models/ruleset/unique/UniqueType.kt b/core/src/com/unciv/models/ruleset/unique/UniqueType.kt index b16c743bf5..d1e40d9537 100644 --- a/core/src/com/unciv/models/ruleset/unique/UniqueType.kt +++ b/core/src/com/unciv/models/ruleset/unique/UniqueType.kt @@ -59,15 +59,15 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { //////////////////////////////////////// GLOBAL UNIQUES //////////////////////////////////////// - + /////// Stat providing uniques - Stats("[stats]", UniqueTarget.Global), + Stats("[stats]", UniqueTarget.Global, UniqueTarget.FollowerBelief), StatsPerCity("[stats] [cityFilter]", UniqueTarget.Global), @Deprecated("As of 3.16.16", ReplaceWith("[stats] "), DeprecationLevel.WARNING) StatBonusForNumberOfSpecialists("[stats] if this city has at least [amount] specialists", UniqueTarget.Global), - StatPercentBonus("[amount]% [Stat]", UniqueTarget.Global), + StatPercentBonus("[amount]% [stat]", UniqueTarget.Global), BonusStatsFromCityStates("[amount]% [stat] from City-States", UniqueTarget.Global), RemoveAnnexUnhappiness("Remove extra unhappiness from annexed cities", UniqueTarget.Building), @@ -103,7 +103,7 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { ProvidesResources("Provides [amount] [resource]", UniqueTarget.Improvement, UniqueTarget.Building), - GrowthPercentBonus("[amount]% growth [cityFilter]", UniqueTarget.Global), + GrowthPercentBonus("[amount]% growth [cityFilter]", UniqueTarget.Global, UniqueTarget.FollowerBelief), @Deprecated("As of 3.16.14", ReplaceWith("[amount]% growth [cityFilter]"), DeprecationLevel.WARNING) GrowthPercentBonusPositive("+[amount]% growth [cityFilter]", UniqueTarget.Global), @Deprecated("As of 3.16.14", ReplaceWith("[amount]% growth [cityFilter] "), DeprecationLevel.WARNING) @@ -115,10 +115,10 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { FreeExtraBeliefs("May choose [amount] additional [beliefType] beliefs when [foundingOrEnhancing] a religion", UniqueTarget.Global), FreeExtraAnyBeliefs("May choose [amount] additional of any type when [foundingOrEnhancing] a religion", UniqueTarget.Global), - + ///////////////////////////////////////// UNIT UNIQUES ///////////////////////////////////////// - + Strength("[amount]% Strength", UniqueTarget.Unit, UniqueTarget.Global), StrengthNearCapital("[amount]% Strength decreasing with distance from the capital", UniqueTarget.Unit), @@ -150,7 +150,7 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { StrengthUnitsTiles("[amount]% Strength for [mapUnitFilter] units in [tileFilter]", UniqueTarget.Global), @Deprecated("As of 3.17.5", ReplaceWith("[+15]% Strength ")) StrengthVsCities("+15% Combat Strength for all units when attacking Cities", UniqueTarget.Global), - + Movement("[amount] Movement", UniqueTarget.Unit, UniqueTarget.Global), Sight("[amount] Sight", UniqueTarget.Unit, UniqueTarget.Global), @@ -188,10 +188,10 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { CanEnterIceTiles("Can enter ice tiles", UniqueTarget.Unit), CannotEnterOcean("Cannot enter ocean tiles", UniqueTarget.Unit), CannotEnterOceanUntilAstronomy("Cannot enter ocean tiles until Astronomy", UniqueTarget.Unit), - + //////////////////////////////////////// TERRAIN UNIQUES //////////////////////////////////////// - + NaturalWonderNeighborCount("Must be adjacent to [amount] [simpleTerrain] tiles", UniqueTarget.Terrain), NaturalWonderNeighborsRange("Must be adjacent to [amount] to [amount] [simpleTerrain] tiles", UniqueTarget.Terrain), NaturalWonderSmallerLandmass("Must not be on [amount] largest landmasses", UniqueTarget.Terrain), @@ -272,7 +272,7 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) { // todo: remove forced sign TimedAttackStrength("+[amount]% attack strength to all [mapUnitFilter] Units for [amount] turns", UniqueTarget.Global), // used in Policy FreeStatBuildings("Provides the cheapest [stat] building in your first [amount] cities for free", UniqueTarget.Global), // used in Policy - FreeSpecificBuildings("Provides a [building] in your first [amount] cities for free", UniqueTarget.Global), // used in Policy + FreeSpecificBuildings("Provides a [buildingName] in your first [amount] cities for free", UniqueTarget.Global), // used in Policy ; /** For uniques that have "special" parameters that can accept multiple types, we can override them manually diff --git a/tests/src/com/unciv/testing/BasicTests.kt b/tests/src/com/unciv/testing/BasicTests.kt index 3d0f06188b..111fd3a027 100644 --- a/tests/src/com/unciv/testing/BasicTests.kt +++ b/tests/src/com/unciv/testing/BasicTests.kt @@ -7,9 +7,12 @@ import com.unciv.UncivGameParameters import com.unciv.models.metadata.GameSettings import com.unciv.models.ruleset.Ruleset import com.unciv.models.ruleset.RulesetCache +import com.unciv.models.ruleset.unique.UniqueParameterType +import com.unciv.models.ruleset.unique.UniqueType import com.unciv.models.ruleset.unit.BaseUnit import com.unciv.models.stats.Stat import com.unciv.models.stats.Stats +import com.unciv.models.translations.getPlaceholderParameters import org.junit.Assert import org.junit.Before import org.junit.Test @@ -81,12 +84,42 @@ class BasicTests { @Test fun baseRulesetHasNoBugs() { - ruleset.modOptions.isBaseRuleset=true + ruleset.modOptions.isBaseRuleset = true val modCheck = ruleset.checkModLinks() - if(modCheck.isNotOK()) println(modCheck) + if (modCheck.isNotOK()) + println(modCheck.getErrorText(true)) Assert.assertFalse(modCheck.isNotOK()) } + @Test + fun uniqueTypesHaveNoUnknownParameters() { + var noUnknownParameters = true + for (uniqueType in UniqueType.values()) { + for (entry in uniqueType.parameterTypeMap.withIndex()) { + for (paramType in entry.value) { + if (paramType == UniqueParameterType.Unknown) { + val badParam = uniqueType.text.getPlaceholderParameters()[entry.index] + println("${uniqueType.name} param[${entry.index}] type \"$badParam\" is unknown") + noUnknownParameters = false + } + } + } + } + Assert.assertTrue("This test succeeds only if all UniqueTypes have all their placeholder parameters mapped to a known UniqueParameterType", noUnknownParameters) + } + + @Test + fun allUniqueTypesHaveAtLeastOneTarget() { + var allOK = true + for (uniqueType in UniqueType.values()) { + if (uniqueType.targetTypes.isEmpty()) { + println("${uniqueType.name} has no targets.") + allOK = false + } + } + Assert.assertTrue("This test succeeds only if all UniqueTypes have at least one UniqueTarget", allOK) + } + //@Test // commented so github doesn't run this fun statMathStressTest() { val runtime = Runtime.getRuntime()