Skip to content

Testing keyword argument defaults #22

Closed

Description

@asmeurer

We need to make sure all the function tests test the default values for keyword arguments, that is, calling the function without passing the keyword argument. This explicitly isn't tested in the signature tests because those tests aren't sophisticated enough to check that the correct default value is used.

I think we can abstract this somewhat in the hypothesis helpers. Something like

class DefaultValue:
    """
    Stand-in value for a keyword argument not being passed
    """

def kwarg(default, kwarg_values):
    return one_of(DefaultValue, just(default), kwarg_values)

with a strategy helper kwarg that would work like

@given(x=arrays(), offset=kwarg(0, integers()))
def test_diagonal(x, offset):
    if offset is DefaultValue:
        res = diagonal(x)
        offset = 0
    else:
        res = diagonal(x, offset=offset)
 
    # Test behavior here

Maybe we could even abstract this further by generating using the function stubs. I'm not sure. The stubs won't contain enough information to infer what strategy values should be drawn from (even with type hints, it won't contain things like bounds or only specific sets of dtypes), but it does have the default values.

Activity

  1. honno commented on Sep 29, 2021

    @honno
    Member

    Ah this is interesting. How about a pattern where we always generate kwargs for the function tests?

    @given(..., kw=one_of(just({}), ...))
    def test_foo(..., kw):
        result = foo(..., **kw)
        ...

    If I'm correct in saying, Python interprets unpacking of an empty dict like foo(..., **{}) exactly the same as foo(...).

    A helper method therefore could be:

    @st.composite
    def kwargs(draw, **kw):
        result = {}
        for k, strat in kw.items():
            if draw(st.booleans()):
                result[k] = draw(strat)
        return result

    So for example:

    @given(
        shape=shapes,
        fill_value=xps.from_dtype(shared_dtypes),
        kw=kwargs(dtype=one_of(none(), shared_dtypes)),
    )
    def test_full(shape, fill_value, kw):
        a = full(shape, fill_value, **kw)
        if "dtype" in kw.keys() and kw["dtype"] is not None:
            assert a.dtype == kw["dtype"]
        ...

    Personally I prefer using <keyword> not in kw.keys() to indicate default values, as opposed to the cognitive load of introducing a custom class like DefaultValue. I also like that Hypothesis errors will print exactly what is passed to the functions being tested. I'm not sure if I'm missing something with this solution though.

  2. asmeurer commented on Sep 29, 2021

    @asmeurer
    MemberAuthor

    Yes, your way is much better.

    We would also want some way to normalize the keyword arguments with the actual default values. So for instance in my diagonal example, we can just check that offset does the right thing and when offset is omitted it is tested as the default value of 0. Something like

    def test_diagonal(...):
        all_kwargs = add_defaults(kwargs)
        offset = all_kwargs['offset'] # Will be offset if provided or 0, the default, if not

    Or maybe we could make a pseudo-strategy that uses shared to pass in the default values as arguments to the test function. I'm not sure what the best way is.

  3. honno commented on Sep 29, 2021

    @honno
    Member

    Oh I see. I think this pattern should be sufficient for anytime we want to test defaulting assumptions.

    def test_diagonal(..., kw):
        res = diagonal(x, **kw)
        offset = 0 if "offset" not in kw else kw["offset"]
        ...
  4. asmeurer commented on Sep 29, 2021

    @asmeurer
    MemberAuthor

    Let's just do that for now. If it gets too repetitive, we can see if something more streamlined makes sense, perhaps by extracting the default automatically from the stubs.

  5. honno commented on Oct 5, 2021

    @honno
    Member

    @asmeurer What do you think about the current use of kw in test_creation_functions.py? I didn't quite appreciate your defaulting idea, as I'm not super happy going kw.get("dtype", None) is None all the time... I think I still need to try the pattern out more, but we might want to implement your add_defaults() idea in the future.

  6. asmeurer commented on Oct 5, 2021

    @asmeurer
    MemberAuthor

    Yeah, working through the linalg tests, which have a lot of keyword arguments, I'm also thinking it would be a good idea, especially if we can extract the defaults automatically from the stubs.

  7. honno commented on Jan 18, 2022

    @honno
    Member

    All the primary tests do this well now I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions