Refactor difficulty adjustment mod combinations test

[smoogipoo]

I wanted to check whether this handled the MultiMod case correctly (see TestMultiModFlattening) and found that it was very hard to read.
This refactors the tests to be explicit about expected types in each mod combination.

[bdach]

I don't know if it's just me, but the way this is written is confusing me, for two reasons. First off, actualTypes is computed in the inner loop, even though its value actually is the same in every iteration of the loop. Worse yet, the flattening of all actualCombinations and the containment check weakens the test.

Consider TestIncompatibleWithSameInstanceViaMultiMod(): the expected combinations are:

expectedCombinations = {{ModNoMod}, {ModA}, {ModA, ModB}}

But the FlattenMods operation returns a flat list instead, yielding

actualTypes = {ModNoMod, ModA, ModA, ModB}

And because the containment assert is checking each individual mod from the expected combination at once rather than the entire set, you can get false positives. As in, imagine that the last element in actualCombinations was {ModB}, rather than {ModA, ModB}; actualTypes would still contain ModA, even though the behaviour would be erroneous. And conversely, if you change {ModA, ModB} in expectedCombinations to just {ModB}, the test passes erroneously for the same reason.

If I were to write this method myself, I would write:

private void assertCombinations(Type[][] expectedCombinations, Mod[] actualCombinations)
{
    Assert.AreEqual(expectedCombinations.Length, actualCombinations.Length);

    Assert.Multiple(() =>
    {
        for (int i = 0; i < expectedCombinations.Length; ++i)
        {
            Type[] expectedTypes = expectedCombinations[i];
            Type[] actualTypes = ModUtils.FlattenMod(actualCombinations[i]).Select(m => m.GetType()).ToArray();

            Assert.That(expectedTypes, Is.EquivalentTo(actualTypes));
        }
    });
}

which would correctly fail in the cases I gave, because entire sets of mods are being checked, rather than only individual parts of these sets.

[smoogipoo]

Good catch, I've applied your fix.

[bdach]

To approach this from another angle, I think that the big part of why the original test was confusing was that multi mods were checked out of order rather than along with everything else. As in, I feel like something like below would be an improvement over master:

[Test]
public void TestDoubleIncompatibleMods()
{
    var combinations = new TestLegacyDifficultyCalculator(new ModA(), new ModB(), new ModIncompatibleWithA(), new ModIncompatibleWithAAndB()).CreateDifficultyAdjustmentModCombinations();

    Assert.AreEqual(8, combinations.Length);

    Assert.IsTrue(combinations[0] is ModNoMod);

    Assert.IsTrue(combinations[1] is ModA);

    Assert.IsTrue(combinations[2] is MultiMod);
    Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModA);
    Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModB);

    Assert.IsTrue(combinations[3] is ModB);

    Assert.IsTrue(combinations[4] is MultiMod);
    Assert.IsTrue(((MultiMod)combinations[4]).Mods[0] is ModB);
    Assert.IsTrue(((MultiMod)combinations[4]).Mods[1] is ModIncompatibleWithA);

    Assert.IsTrue(combinations[5] is ModIncompatibleWithA);

    Assert.IsTrue(combinations[6] is MultiMod);
    Assert.IsTrue(((MultiMod)combinations[6]).Mods[0] is ModIncompatibleWithA);
    Assert.IsTrue(((MultiMod)combinations[6]).Mods[1] is ModIncompatibleWithAAndB);

    Assert.IsTrue(combinations[7] is ModIncompatibleWithAAndB);
}

That said, if the logic error pointed out in my review above is fixed, I don't really have a strong personal preference for either. But the fact that that logic was wrong may indicate that it would be better to just keep it simple.

[peppy]

Reads saner.