Fix SaveTest when certain scenario filenames are present

[bmgxyz]

Changes

• Replace the existing logic in SaveTest.cs for enumerating scenario files with an explicit list of scenario filenames
  • Scenarios whose filenames aren't in the list are skipped
  • Scenarios in the list whose files are missing are also skipped
  • Always skip scenario 7 because parsing is currently broken

Closes #849

Test Plan

1. Create an empty file in $CIV3_HOME/Conquests/Conquests/ with a name that starts with a number, such as 1vsAll.biq
2. Check out Development and run the EngineTests
3. Verify that EngineTests.GameData.SaveTests.LoadAllConquestScenarios fails to parse the empty file created above; this confirms that the test tried to load a scenario file that it shouldn't have
4. Checkout this PR's branch and re-run the tests
5. Verify that all tests now pass

[stavrosfa]

Thanks, It's working as expected!

However, the current test runs for both Conquests/Conquests and Conquests/Scenarios paths here

Prototype/EngineTests/GameData/SaveTest.cs

Line 283 in 1cc3933

CheckScenariosInCiv3Subfolder("Conquests/Conquests", runOneTurn: true);

and the scenarios have different names, so it's silently failing for Conquests/Scenarios because the names don't match exactly.

Perhaps add these in the scenarioNamesToTest array and then have an Assert that checks the saveFiles count?

Regex would be another solution, but then it's not as maintainable, or even user friendly.

Another thing that could be an enchancement, that is not in the original request, but I just saw as I was going through the code now is that the json files the test produces override each other

Prototype/EngineTests/GameData/SaveTest.cs

Line 370 in 1cc3933

game.Save(Path.Combine(testDirectory, "data", "output", $"conquest_{name[0]}.json"));

so it would be nice to have unique names for these, perhaps instead of "conquest_" we could use the subfolder param?

If you have the time for this as well it would be great to have this in this PR, if not that's fine as well

[zack-the-coding-actuary]

Another potential thing. The folks who play live MP Civ 3 usually have their default MP Conquests scenarios name mangled by their own custom scens that pull assets from different libraries than the base ones (e.g. Tethurkan, Medieval Japan, etc)

Not sure if that's a concern at this time though, I think I might be the only one with that problem who would actually be running unit tests.

[stavrosfa]

Yeah, not sure why the MP scenarios are even tested at the moment, but unless we come to an issue like we did with the custom scenario 1vsAll.biq , I think we have to replicate the existing behaviour just to be on the safer side.

I have some ideas on how we could make this more robust/consistent in future, and perhaps figure out why we test the MP files too, but for now this will do

Git history points to @TomWerner for adding the MP files, maybe you can enlighten us on this a bit more

[TomWerner]

Git history points to @TomWerner for adding the MP files, maybe you can enlighten us on this a bit more

Looks like this commit? I think I remember there being some issue that was present in one set of scenarios but not the other, but I don't remember for sure.

[stavrosfa]

Yeah that seems to be the one, thanks! I guess that's good enough for us to leave the tests in then

[bmgxyz]

However, the current test runs for both Conquests/Conquests and Conquests/Scenarios paths here

Prototype/EngineTests/GameData/SaveTest.cs

Line 283 in 1cc3933

CheckScenariosInCiv3Subfolder("Conquests/Conquests", runOneTurn: true);

and the scenarios have different names, so it's silently failing for Conquests/Scenarios because the names don't match exactly.

Perhaps add these in the scenarioNamesToTest array and then have an Assert that checks the saveFiles count?

Good point, I moved the list of filenames up one scope and split it into singleplayer and multiplayer. These are then passed back down, and an assert in the inner function checks the number of loaded files against the length of the list.

Another thing that could be an enchancement, that is not in the original request, but I just saw as I was going through the code now is that the json files the test produces override each other

Prototype/EngineTests/GameData/SaveTest.cs

Line 370 in 1cc3933

game.Save(Path.Combine(testDirectory, "data", "output", $"conquest_{name[0]}.json"));

so it would be nice to have unique names for these, perhaps instead of "conquest_" we could use the subfolder param?

Added a basename parameter to the function that determines the first parts of each filename. The two groups are written separately now instead of overwriting each other.