Usage of subtests and asserts for test readability

Usage of subtests and asserts for test readability

This came up in PR #798 for some of the tests added to test_env_tools.py by @RHammond2

Basically - the use of subtests in H2I is pretty nice for debugging tests and tracking down where a test failure may have originated from (ex, LCOE subtest fails and so does the the subtest for the total CapEx, but the subtest for the AEP of the commodity stream stays the same this helps a developer determine that maybe a cost value changed (rather than a performance value). In other cases - the usage of subtests can make the tests overly verbose and harder to follow what is being tested (as is the case in some tests in test_env_tools.py).

Examples of overly verbose subtests

For example, this type of subtest usage in test_env_tools.py is a bit over-the-top:

with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL)"):
        assert env_vars["TEST_CREDENTIAL"] == "my_credential_value"
with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_B)"):
        assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake"
with subtests.test("Empty line and mixed separators (TEST_CREDENTIAL_D)"):
        assert env_vars["TEST_CREDENTIAL_D"] == "testingValue"

The test would be easier to read if the subtest was formatted like this:

with subtests.test("Environment variable values"):
        assert env_vars["TEST_CREDENTIAL"] == "my_credential_value"
        assert env_vars["TEST_CREDENTIAL_B"] == "testing@yahoo.fake"
        assert env_vars["TEST_CREDENTIAL_D"] == "testingValue"

Another example of over-usage of subtests is in h2integrate/core/test/test_framework.py::test_use_commodity_stream_timeseries_finances_error:

    with pytest.raises(ValueError) as excinfo:
        H2IntegrateModel(top_level_config)
    err = str(excinfo.value)

    with subtests.test("Commodity stream name is missing (commodity_stream_output is required)"):
        assert "`commodity_stream_output` is a required input" in err
    with subtests.test(
        "Commodity stream name is missing (use_commodity_stream_timeseries is True)"
    ):
        assert "`use_commodity_stream_timeseries` is True" in err
    with subtests.test("Commodity stream name is missing (finance subgroup `electricity_doc`)"):
        assert "finance subgroup `electricity_doc`" in err

which could be simplified to:

    with pytest.raises(ValueError) as excinfo:
        H2IntegrateModel(top_level_config)
    err = str(excinfo.value)
    with subtests.test("Commodity stream name is missing error message parts"):
        assert "`commodity_stream_output` is a required input" in err
        assert "`use_commodity_stream_timeseries` is True" in err
        assert "finance subgroup `electricity_doc`" in err

Final thoughts

I think that this over-usage is most common in testing of utilities/tools (not so much in testing example or specific models). While it could be harder to determine which part of a subtest failed when assert statements are grouped underneath it, this can still be easily figured out by a user if they use other methods to get more information on test failures. Some examples of this are:

• integrating print statements into the test and running the test with pytest -vs so that print statements are output to the terminal
• using the VSCode Test debugger (beaker icon on the right of the VSCode screen)

I'm mostly making this issue to gauge people's thoughts on this before any changes are made.

Start by reviewing the subtest patterns in test_env_tools.py and h2integrate/core/test/test_framework.py, along with the examples in this issue. Identify where related assertions can be grouped without losing useful failure context, then confirm the proposed scope with maintainers before changing tests. Done means the affected tests are more readable while retaining meaningful subtest diagnostics.