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

android/assets/jsons/Civ V - Vanilla/UnitPromotions.json

@@ -206,7 +206,7 @@
 	{
 		"name": "Boarding Party III",
 		"prerequisites": ["Boarding Party II"],
-		"uniques": ["+[15]% Strength <vs [Water] units>"],
+		"uniques": ["[+15]% Strength <vs [Water] units>"],
 		"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 <vs [Water] units>"],
 		"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"]
 	},

core/src/com/unciv/models/ruleset/Ruleset.kt

@@ -80,7 +80,7 @@ class Ruleset {
     val units = LinkedHashMap<String, BaseUnit>()
     val unitPromotions = LinkedHashMap<String, Promotion>()
     val unitTypes = LinkedHashMap<String, UnitType>()
-    
+
     val mods = LinkedHashSet<String>()
     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() =
+        fun getErrorText(unfiltered: Boolean = false) =
-            filter { it.errorSeverityToReport != RulesetErrorSeverity.WarningOptionsOnly }
+            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",
+                lines.add(
-                    RulesetErrorSeverity.WarningOptionsOnly)
+                    "`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",
+                lines.add(
-                    RulesetErrorSeverity.WarningOptionsOnly)
+                    "`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
     }
 }

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
+            in knownValues -> null
-            else -> UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific
+            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? {
+                UniqueType.UniqueComplianceErrorSeverity? = null
-            return null
-        }
     };
 
     abstract fun getErrorSeverity(parameterText:String, ruleset: Ruleset): UniqueType.UniqueComplianceErrorSeverity?

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] <if this city has at least [amount] specialists>"), 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] <when not at war>"), 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 <for [All] units> <vs cities> <when attacking>"))
     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

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()