Skip to content

Compact way to combine tests that raise with tests that return values #10089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
phil20686 opened this issue Jun 29, 2022 · 5 comments
Closed
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@phil20686
Copy link

phil20686 commented Jun 29, 2022

What's the problem this feature will solve?

Improved useability in simple cases

Describe the solution you'd like

See code sample and attached examples

Additional context

This is some code that I have written again and again and it removes a lot of boilerplate in writing tests. Seems like it would be a useful addition at source in pytest.

def assert_equal(value, expect, **kwargs):
    """
    Test utility for assorted types, see pd.testing.assert_frame_equal and siblings for supported keywords.

    Parameters
    ----------
    value: the value
    expect: the expectation
    kwargs: passed through depending on the type.

    Raises
    ------
    AssertionError: if not equal
    """
    if isinstance(value, pd.DataFrame):
        assert_frame_equal(value, expect, **kwargs)
    elif isinstance(value, pd.Series):
        assert_series_equal(value, expect, **kwargs)
    elif isinstance(value, pd.Index):
        assert_index_equal(value, expect, **kwargs)
    else:
        assert value == expect

def _is_exception_type(expect):
    return isinstance(expect, type) and issubclass(expect, Exception)

def _is_exception_instance(expect):
    return isinstance(expect, Exception)

def assert_or_raise(func, expect, *args, **kwargs):
    """
    Calls func(*args, **kwargs) and asserts that you get the expected error. If no error is specified,
    returns the value of func(*args, **kwargs)
    """
    if _is_exception_type(expect):
        with pytest.raises(expect):
            func(*args, **kwargs)
    elif _is_exception_instance(expect):
        with pytest.raises(type(expect), match=str(expect)):
            func(*args, **kwargs)
    else:
        return func(*args, **kwargs)


def assert_call(func, expect, *args, test_kwargs: Optional[Dict] = None, **kwargs):
    val = assert_or_raise(func, expect, *args, **kwargs)
    if not (
            _is_exception_type(expect) or _is_exception_instance(expect)
    ):
        assert_equal(val, expect, **(test_kwargs or {}))
    return val

And then you can use it like this:

@pytest.mark.parametrize(
    "input_string, expect",
    [
        (
            "21:00:05.5 [America/New_York]",
            TimeOfDay(
                datetime.time(21, 0, 5, microsecond=500000),
                tz=pytz.timezone("America/New_York")
            )
        ),
        (
            "21,00:05.5 [America/New_York]",
            ValueError(
                "Time string.*did not match.*"
            )
        )
    ]
)
def test_time_of_day_builder(input_string, expect):
    assert_call(
        TimeOfDay.from_str,
        expect,
        input_string
    )

This code now covers a wide variety of easy cases and leads to extremely compact test cases. To summarise the behaviour of assert call is as follows:

  • If expect is an exception instance, calls it "with raises" and uses the instance error message as the argument to match.
  • If expect is an exception type, calls it with raises but with no argument to match
  • If expect is anything else, compares equality.
    Moreover the equality operator will check for pandas and numpy types, and delegates to their standard equality methods depending on type, allowing you to pass "test_kwargs" such as atol or rtol to the numeric data types.

Without writing functions like this, you end up either having to separate tests that cause errors from tests that do not, or you must put branching logic separately in each test. In my experience the construction above allows you to write much simpler cleaner tests where 90+% of your library unit tests will be a call to assert_call.

@The-Compiler
Copy link
Member

Instead of doing a lot of magic based on the type of the value, you can just parametrize the raises context manager - see #1830 for lots of previous discussion about this.

@kalekundert
Copy link
Contributor

I can really relate to this problem, but I think that having an assert_call() function like this is too limiting in what checks you can make. For example, what if I'm testing a function that returns an object, and I only want to make assertions about some of its attributes? Or what if I want to test for identity (is) instead of equality (==)? Another symptom of this is the hard-coded support for pandas in assert_equal(): if more expressive assertions were possible, third-party dependencies like this wouldn't be necessary.

I think a better approach is the one I outlined in this stack overflow answer. Basically, use either pytest.raises or nullcontext as a context manager, and use either the a real value or unittest.mock.Mock() as the expected value. This approach also doesn't require any changes to be made to pytest itself (although perhaps it would be convenient to add the two short helper functions it uses).

@asottile
Copy link
Member

#4682 addressed this -- though I really think you said the correct thing out loud and that your tests are trying to be too clever:

Without writing functions like this, you end up either having to separate tests that cause errors from tests that do not

this is a good thing

@Zac-HD Zac-HD added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Jun 30, 2022
@Zac-HD Zac-HD closed this as completed Jun 30, 2022
@phil20686
Copy link
Author

I can really relate to this problem, but I think that having an assert_call() function like this is too limiting in what checks you can make. For example, what if I'm testing a function that returns an object, and I only want to make assertions about some of its attributes? Or what if I want to test for identity (is) instead of equality (==)? Another symptom of this is the hard-coded support for pandas in assert_equal(): if more expressive assertions were possible, third-party dependencies like this wouldn't be necessary.

I think a better approach is the one I outlined in this stack overflow answer. Basically, use either pytest.raises or nullcontext as a context manager, and use either the a real value or unittest.mock.Mock() as the expected value. This approach also doesn't require any changes to be made to pytest itself (although perhaps it would be convenient to add the two short helper functions it uses).

Got so say, I disagree pretty strongly with the sentiment here. From a usability perspective, covering the core cases in the most compact way possible is extremely valuable. 90% of all unit tests test extremely simple functions across small domains which include one or two error conditions. It is not a problem to be limited in this way, because there is no requirement to use convenience functions, if you want to do more complex tests you can always revert to writing things the long way, but that is usually a minority of cases. When projects have tens of thousands of unit tests then even small advantages in brevity and readability add up. The stack overflow answer you link to seems far less readable than the above proposal, argument unpacking inside the test tuples is not great.

I appreciate that from pytest's perspective you probably don't want to tie yourself to dependencies on numpy, pandas etc, personally I work a lot on the scientific stack so most functions return those types.

The context manger @The-Compiler mentioned is useful for when you want to test many different exceptions cleanly, and I did not know/consider that approach, so thanks.

Anyway, its clear I have wandered into a very old argument so thank you for your time and for maintaining a great project. Its hardly a burden to write things like the above into specific projects where they are useful anyway. It might make more sense as a mini-library for people who test mainly scientific of numerical code.

@RonnyPfannschmidt
Copy link
Member

@phil20686 i can understand the sentinent for compactness, but its important to keep some domains separate for better understanding

the functions you propose cover a wide range of domains that an individual test writer may not even care about,
so all that convenience and deduplication can look like a dreadfull miss-optimization

i intent to get together with @asottile in an effort to provide better data matchers and that hopefully will help to trim down those needs while also helping people to come up with better input/expectation tables

that being said, its still valuable to be able to compactly express the intents when it comes to data expectations
however the structure and the implied fuzzyness vs strictness of those tools is important, i have had my fair share of test helpers, that in trying to bee too helpful and too easy, would actually hide test errors/testing issues

my own subjective impression is, that the example helper you showed makes it very easy to err into the direction of deceptive convenience, and that tends to bite very painful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

6 participants