Add countables for worked tiles/population

[SeventhM]

1. I would really like to work on stuff like this at the library, so I'm just not going to look at what's necessary for test coverage until later when I'm at the library
2. I haven't looked too hard at countables and their relationship to UniqueParameterType, so if I've made some mistakes in regards to how it's supposed to be implement, please say so
3. I don't know if it makes sense to merge the countables together so there's only 2 additional countables here instead of 4
4. I'm surprised that this is the first countable the is city specific. Doubly so since... cityWide resources is a thing (or even just stuff like checking production or city specific happiness)

[SomeTroglodyte]

Sorry, that was me who made unit test coverage for countables mandatory. That's the reason the checks fail. I know you mentioned "test coverage later", just saying for anybody wondering about the red mark.

But it will be a good thing - city scope being a first will need some scrutiny anyway - whether the game engine will be nice and provide a correct GameContext.

[RobLoach]

Would a [cityFilter] be worth having?

[RobLoach]

Maybe add a [cityFilter] too? What do you think? Then you could get number of working peeps across all religious cities, or something.

[RobLoach]

Actually, you're right. Having a [cityFilter] would allow for combining the two, as [cityFilter] has a in this city

[SeventhM]

Hmm....
Not what I meant by combining the two (I meant having the countable for population and tiles covered by the same countable), but maybe. I guess my main issue is I'm not entirely sure what the application is there for [cityFilter] to be here. Like are we having uniques for the worked tiles of all cities on the map? That seems almost out of place since not even OwnedTiles does that. Are we going to check the workeded tiles of holy cities? Why not just do a self building building and check in this city? There seems just to be too many limitations to [cityFilter] as opposed to a self building building where I'm not sure the goal

[SeventhM]

But it will be a good thing - city scope being a first will need some scrutiny anyway - whether the game engine will be nice and provide a correct GameContext.

I.... actually disagree here. For 2 main reasons

1. Plenty of aspects of the game that are mod focused/mod only doesn't have test coverage, so it seems half out of place that we demand test coverage here. It's not that big a deal, but it is a notable oddity
2. Tbh, most of the tests done already are... sus at best, downright worthless at worst

Like, to give an example, testFilteredTechnologies doesn't actually test the countable in any meaningful way. It's much closer to something like models.ruleset.tech.EraTests.testMatchesFilter. This is because there's nothing really to test FilteredTechnologies on since its implementation is so trivial. Unironically a direct test of TechManager.matchesFilter combined with a test that GameContext is sent at all in countables is much more useful

Like, an inaccurate test for this PR would be the following

    @Test
    fun testWorkedtiles() {
        setupModdedGame()
        city.population.addPopulation(3)
        // Clear worked tiles
        for (position in city.workedTiles) {
            city.population.stopWorkingTile(position)
        }
        val tile = city.tilesInRange.first { it.position != city.getCenterTile().position }
        city.workedTiles.add(tile.position)
        val context = GameContext(city)
        runTestParcours("Worked tiles countable", { filter: String ->
            Countables.getCountableAmount("Worked [$filter] Tiles", context)
        },
            "All", 1,
        )
    }

Despite following the same standards as the other tests, we kinda aren't testing anything important besides whether we can filter worked tiles at all. There's no reason to assume the gameContext wouldn't carry over since you know, we directly sent it. A better test is this

    @Test
    fun testCityContext() {
        setupModdedGame()
        city.population.addPopulation(3)

        // Clear worked tiles
        for (position in city.workedTiles) {
            city.population.stopWorkingTile(position)
        }
        val tiles = city.tilesInRange.filter { it.position != city.getCenterTile().position }
            .take(3)
            .map { it.position }
        city.workedTiles.addAll(tiles)
        
        val currentPopulation = city.population.population
        val building = game.createBuilding("[+1] population [in this city] <for every [Worked [All] Tiles in this city]>")
        city.cityConstructions.addBuilding(building)
        val context = GameContext(city)
        runTestParcours("Population countable", { filter: String ->
            Countables.getCountableAmount("Worked [$filter]", context)
        },
            "Population", currentPopulation + 3,
        )
    }

Btw...

• Did you know that such a countable doesn't work, no matter what we used here? Not even for every [3]? CityConstructions.triggerNewBuildingUniques only gets the uniqueObjects of the building and triggers them. It doesn't multiply them correctly. This error might be in more places in the codebase than just here
• Did you know that CityConstructions.triggerNewBuildingUniques incorrectly checks city.civ.getTriggeredUniques instead of city.getTriggeredUniques for the uniques not directly on the building itself?

[SomeTroglodyte]

notable oddity

I counter-disagree. Knowingly having aspects without test coverage and accepting it is odd. Having a chance to ensure test coverage in a systematical way shouldn't be wasted.

downright worthless at worst

Good to hear - if you analyzed so far then you have all the tools needed to improve them.

since you know, we directly sent it

I know that train of thought. But unit tests sometimes are worth something even when at the time you do them they seem trivial: "seem trivial" is an expectation, and they guard that expectation against someone else breaking stuff who doesn't "get" those trivialities? Maybe.

Did you know

No. Much of the more complicated unique stuff eludes me. What I do know is that I'm not happy with how we express modifier interoperability. RulesetValidator cannot - in many cases - correctly check what modifier will work in what context / cooperate with other modifiers. In other words, around modifiers and triggers, when I code some new feature, I cannot constrain it properly so it's only used the way my new code can support. Some of that misery is brought on by having the definition "what is a conditional or what is a trigger" as UniqueTarget and not UniqueFlag, so I can no longer constrain what container IHasUniques types are supported. To refactor that - too late, too much interdependency already ingrained in the codebase...

[SeventhM]

I counter-disagree. Knowingly having aspects without test coverage and accepting it is odd. Having a chance to ensure test coverage in a systematical way shouldn't be wasted.

The problem is when that test coverage is for unnecessary stuff or already tested stuff e.g testPoliciesCountables is mostly identical in most ways to the aforementioned models.ruleset.tech.EraTests.testMatchesFilter. There's nothing we're testing for here that wouldn't have been caught either by looking over the PR or by better tests

I know that train of thought. But unit tests sometimes are worth something even when at the time you do them they seem trivial: "seem trivial" is an expectation, and they guard that expectation against someone else breaking stuff who doesn't "get" those trivialities? Maybe.

You're more or less explaining how we should test if a function can access a variable it was directly sent. If we expect some levels of testing here to be necessary, something has already gone horribly wrong and our test really isn't catching anything new. Again, other tests check this much better than enforcing a test here of all places. At that point, why do we not enforce test coverage over conditionals or unique parameter types? Both of those probably have much more pressing concerns and either one will likely give us full overlap with countables

Like, that's kinda my issue with enforcing test coverage on countables. Unless someone really screws it up, there's very, very, very, very, very little that countables do that doesn't overlap with something else that's far more critical. Want to test tech filters? Then let's do that directly. Want to test tile filters and ownership? Then let's do that directly (which we already kinda do). Want to test era filters? We have a whole file dedicated to that (that's also in the wrong package). Why bother with duplicate tests on something we know shouldn't have issues by just reading the eval functions? This isn't the more complicated stuff like expressions

No. Much of the more complicated unique stuff eludes me.

Ok, sure. But like.... We could've easily tested that instead of whatever it is this file is supposed to be testing. Why not test a building with countables on triggers? Or countables on stats? That's like... The whole point of them? If we're doing trivial meaningless tests, why are they *not* through buildings or units like they would be in reality? That seems like the easiest thing to test first and find out isn't working. Again, my problem is the pursuit of test coverage has us testing irrelevant stuff we already can easily tell won't fail (or if it does fail, we're testing redundant stuff that we already will know will fail). We aren't some cooperation asking for meaningless tests, we don't need this

[SeventhM]

Edit: models.ruleset.tech.EraTests.testMatchesFilter is most similar to testEraNumberCountable and testFilteredTechnologies, not testPoliciesCountables. I can't read

Point is still the same: creating a standalone filter test has fully overlap with checking solely the eval functions of the countables

[SeventhM]

Changing to a draft while I wait on a larger PR I'm working on removing the test requirement

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[SeventhM]

Shhhh... I'm definitely working on this and am not getting distracted on my progress on reworking the tests