SmartSim icon indicating copy to clipboard operation
SmartSim copied to clipboard

Remove implict fixtures dependencies from other fixtures

Open MattToast opened this issue 2 years ago • 0 comments

Description

SmartSim has many fixtures in its test suite that provide a static class with some number of methods to preform common operations across the test suite (create files, make tmp dirs, create SS entities, etc.). Some of these fixtures require one another, either expecting that other fixtures will be used before itself having a dependency on a fixture in order to be able to utilize a method.

The most egregious offender of this anti pattern is the ColoUtils class (exposed as the coloutils fixture) which explicitly requires the fileutils fixture be passed as a parameter for all of its utility methods.

@pytest.fixture
def coloutils() -> t.Type[ColoUtils]:
    return ColoUtils


class ColoUtils:
    @staticmethod
    def setup_test_colo(
        fileutils: t.Type[FileUtils],
        ...
    ) -> Model:
        # `fileutils` is expected to be the yeilded result from
        # the `fileutils` fixture
        ...
    ...

which requires developers looking to use the coloutils fixture to know that they almost certainly will need to require the fileutils fixture in order to be able to effectively utilize coloutils

def test_things(coloutils, fileutils):
    #                      ^^^^^^^^^
    # This test requires that I manually request this fixture
    model = coloutils.setup_test_colo(fileutils, ...)
    #                                 ^^^^^^^^^
    # Even though I will only ever use it as a dependency to another
    # fixture. Outside of this use, fileutils may be completely unused
    # in this test.
    ...
    assert things

Justification

Manually passing fixtures to other fixtures requires that the developer writing the unit tests know the relationships between all existing SmartSim fixtures and knowing which fixtures rely on each other. This can confuse and put off outside contributors who may be willing to contribute to SmartSim, but are unwilling to deal with the ins-and-outs of the opaque test suite.

On a more general level, manually passing fixtures to other fixtures in the body of the unit test adds clutter to the test suite that adds mental noise and obfuscates from the intended meaning of the test.

Implementation Strategy

Rather than having fixtures return static classes, fixtures should instance a utility class whose state is all of the fixtures required in order to make calls to its methods.

For e.g.

@pytest.fixture
def coloutils(fileutils: FileUtils) -> ColoUtils:
    return ColoUtils(fileutils)

class ColoUtils:
    def __init__(self, fileutils: FileUtils) -> None:
        self._fileutils = fileutils

    def setup_test_colo(
        # Now only pass args immediately related to how to setup the colo test
        ...
    ) -> Model:
        # and use the instance ref to `self._fileutils` for file ops
        ...

This then simplifies the test suite API because as a developer, I do not need to know which fixtures require which other fixtures. I can simply just request the tools needed for the immediate task

def test_something_with_colo(coloutils):
    # I, as an engineer writing a unit test, do not need
    # to be aware of the existence of `fileutils` to utilize
    # this abstraction
    my_colo_model = coloutils.setup_test_colo(...)
    ...
    assert test_things

Acceptance Criteria

  • [ ] Identify existing SmartSim fixtures that implicitly rely on each other (at least ones that directly require another fixture to be passed as a parameter)
  • [ ] Refactor these fixtures so that they handle their own set up, construction, and tear down independently
  • [ ] Refactor the tests so that the test suite passes using the refactored fixtures API

MattToast avatar Nov 08 '23 17:11 MattToast