Real Unit Tests: Settings and resolve_settings

[flagrama]

This is a small part of a much larger project I want to start working on. I want to make actual unit tests instead of the test the randomizer currently have which are more integration tests. I've wanted to do this for a long time, but did not have enough experience with writing software with tests to do it effectively, but I think I do now.

Once many tests are in place I also want to work on refactoring the randomizer some. I don't have too many specific goals but I do want to separate plando and the fill algorithm if I can to make both easier to work on for example. I expect to be discussing this in #dev-public-talk a lot when this happens, but that's a long way off. I need the unit tests in place to ensure such refactors don't cause regressions.

The plan for this PR is just to get tests for all of the methods of the Settings class and Main.resolve_settings function tested. I may also want to move some stuff around to make testing these easier (e.g. resolve_settings creates and returns a Rom object but does nothing with it, at the same time it's mutating the settings object sent to it in every other part. Shouldn't the Rom object creation be pulled out either into its own function or directly in main? Wouldn't resolve_settings fit better as a method of Settings?, or to reduce duplicated code if it is there. (e.g. why do Settings.__init__ and Main.resolve_settings both check that world count is not below 1 and not above 255? Is there a way to rely on only one of them?)

[flagrama]

aaaaaaa test_reset_distribution isn't being run independently and is affecting the new test_update_with_settings_string I'm trying to make.

def test_update_with_settings_string(self):
    default_settings_string = "BSAWDNCAX2TB2XCHGA3UL62ANEBBABADFAAAAAAEAASAAAAAJAAAAAAAAAAAE2FKA6A86AAJNJACAAF2D"
    default_settings = Settings.Settings({})
    test_settings = Settings.Settings({})

    test_settings.update_with_settings_string(default_settings_string)

    self._assert_settings_objects_are_equal(default_settings, test_settings, True)

def test_reset_distribution(self):
    disabled_location = "KF Midos Top Left Chest"
    test_settings = Settings.Settings({})
    test_settings.disabled_locations.append(disabled_location)

    test_settings.reset_distribution()

    self.assertIn(
        disabled_location, test_settings.distribution.world_dists[0].locations
    )
    # Perhaps LocationRecord should be iterable e.g. locations[disabled_location]
    self.assertEqual(
        "#Junk",
        test_settings.distribution.world_dists[0]
        .locations.get(disabled_location)
        .item,
    )

def _assert_settings_objects_are_equal(self, original_settings: Settings, modified_settings: Settings, skip_seed: bool):
    for setting in modified_settings.settings_dict:
        self.assertTrue(
            setting in original_settings.settings_dict,
            f"Setting {setting} was removed.",
            )

        if setting == "seed" and skip_seed:
            continue

        self.assertEqual(
            modified_settings.settings_dict[setting],
            original_settings.settings_dict[setting],
            f"{setting} was not copied. Was {modified_settings.settings_dict[setting]} "
            + f"is now {original_settings.settings_dict[setting]}",
            )
        self.assertEqual(
            modified_settings.__getattribute__(setting),
            original_settings.__getattribute__(setting),
        )

disabled_locations was not copied. Was [] is now ['KF Midos Top Left Chest']
['KF Midos Top Left Chest'] != []

Expected :[]
Actual   :['KF Midos Top Left Chest']
<Click to see difference>

Traceback (most recent call last):
  File "/home/vcunningham/PycharmProjects/OoT-Randomizer/tests/unit/test_settings.py", line 108, in test_update_with_settings_string
    self._assert_settings_objects_are_equal(default_settings, test_settings, True)
  File "/home/vcunningham/PycharmProjects/OoT-Randomizer/tests/unit/test_settings.py", line 246, in _assert_settings_objects_are_equal
    self.assertEqual(
AssertionError: Lists differ: [] != ['KF Midos Top Left Chest']

Second list contains 1 additional elements.
First extra element 0:
'KF Midos Top Left Chest'

- []
+ ['KF Midos Top Left Chest'] : disabled_locations was not copied. Was [] is now ['KF Midos Top Left Chest']

self._assert_settings_objects_are_equal is also called in test_copy where it works unless renamed to test_zcopy to ensure it comes after test_distribution_reset.

Anybody have any idea what is going on here?

[flagrama]

etc on the Discord reminded me that using mutable values as default arguments causes this kind of weirdness. I'll have to handle this in my tests I guess and leave a nice nasty # BUG: comment next to it to remember to fix it once refactoring can actually happen.

[flagrama]

I feel like this is in a good enough position for merging now. I plan to deal with some of the more rigid tests fairly quickly, but would prefer to do so in a separate PR (mainly so this one doesn't touch any production code and actually can be merged)