Skip to content

Improve ParameterSet with nested ParameterSet objects #9074

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

Open
vgorkavenko opened this issue Sep 5, 2021 · 23 comments
Open

Improve ParameterSet with nested ParameterSet objects #9074

vgorkavenko opened this issue Sep 5, 2021 · 23 comments
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@vgorkavenko
Copy link

vgorkavenko commented Sep 5, 2021

What's the problem this feature will solve?

I want the possibility to combinate ParameterSet objects to parametrize tests

Describe the solution you'd like

I want nested ParameterSet objects to be unpacked and combined their ids and marks
For example, I have this test:

@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            pytest.param(1, id="this_is_one"),
            pytest.param(2, id="this_is_two"),
            pytest.param(3, id="this_is_three"),
        )
    ]
)
def test_example(a, b, c):
    assert (1, 2, 3) == (a, b, c)

This test won’t pass now, because nested ParameterSet objects throw in the test "as-is".

FAILED test.py::test_example[a0-b0-c0] - AssertionError: assert (1, 2, 3) == (ParameterSet...)

This is logical behavior, but useless.

I suggest unpack nested ParameterSet objects and merge their ids and marks. If parent ParameterSet has an id - it rewrites nested ids.
The example above will in collect:
<Function test_example[this_is_one-this_is_two-this_is_three]>

Here are a few more examples

1. ID rewriting

@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            pytest.param(1, id="this_is_one"),
            pytest.param(2, id="this_is_two"),
            pytest.param(3, id="this_is_three"),
            id="one_two_three"
        )
    ]
)

<Function test_example[one_two_three]>

2. Use simple values with ParameterSet

@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            1,
            pytest.param(2, id="this_is_two"),
            3,
        )
    ]
)

<Function test_example[1-this_is_two-3]>

3. Merge marks

@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            pytest.param(1, id="this_is_one", marks=[pytest.mark.one]),
            pytest.param(2, id="this_is_two", marks=[pytest.mark.two]),
            pytest.param(3, id="this_is_three", marks=[pytest.mark.three]),
            id="one_two_three",
            marks=[pytest.mark.full]
        )
    ]
)

In this case, marks merged in one list - [one, two, three, full]

I have a solution that works, but I wrote it "on a napkin" - vgorkavenko@99c0712

@RonnyPfannschmidt
Copy link
Member

Do you have a concrete example where this helps,

If it boils down to covariant and correlated values, it's not exactly a good representation

@vgorkavenko
Copy link
Author

vgorkavenko commented Sep 5, 2021

@RonnyPfannschmidt Suppose we have a function that creates ParemeterSet, where value is the path to the file and id is the name of the file.

def server_config(path_to_file, marks: list):
    return pytest.param(path_to_file, id=path_to_file.split("/")[-1], marks=marks)

Also, we have a function that creates ParameterSet, where value is the database connection config dictionary and id is alias:

def conn_config(conn_config: dict, name: str, marks: list):
    return pytest.param(conn_config, id=name, marks=marks)

Now we need to check that certain server configurations work with certain connection configurations.
To test each of these combinations we need to perform the same actions, but with different configurations. The easiest thing is to write one test and parametrize it. Some combinations we need to mark because the server in a certain configuration runs long:

@pytest.mark.parametrize(
    "server, connection", [
        pytest.param(
            server_config("simple_server.yaml"),
            conn_config({}, name="simple_connection"),
        ),
        pytest.param(
            server_config("extended_server.yaml", marks=[pytest.mark.full]),
            conn_config({"foo": 1}, name="hard_connection"),
            marks=[pytest.mark.long]
        ),
        ...
    ]
)
def test_example(server, connection):
    assert check(server, connection)

@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 Sep 5, 2021
@RonnyPfannschmidt
Copy link
Member

So we ought to talk about the idea of a single parameter, not Sets of those, I'll come back to the computer in about 3 hours and 400 km

@vgorkavenko
Copy link
Author

@RonnyPfannschmidt Yep. My idea is to recursively unpack nested ParameterSet objects and use their contents in one ParameterSet object

@RonnyPfannschmidt
Copy link
Member

i think its a good idea, we need to decide where exactly it should unpack, and whether or not a extra indication is needed

@The-Compiler
Copy link
Member

I'm -1 on this. It uses the same thing to both mean "a set of paramters", but also "a single argument in that set", which seems like a lot of room for confusion.

@vgorkavenko
Copy link
Author

@RonnyPfannschmidt In my commit, I do unpack in ParameterSet _for_parametrize class method, but there are two points:

  1. There is a good place because unpacking takes place before check that all parameter sets have the correct number of values. Placing in this place will allow us to do this:
@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            pytest.param(1, id="this_is_one"),
            pytest.param(2, 3, id="this_is_two_and_three"),
        )
    ]
)

<Function test_example[this_is_one-this_is_two_and_three]>
  1. There is a bad place because ids for simple values are set in parametrize method. To get the behavior as in the example below, need to place unpacking in parametrize method
@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            1,
            pytest.param(2, id="this_is_two"),
            3,
        )
    ]
)

<Function test_example[1-this_is_two-3]>

@vgorkavenko
Copy link
Author

vgorkavenko commented Sep 5, 2021

@The-Compiler I just want to improve current behavior and do it more friendly. I’m not sure that a common pytest user wants ParameterSet objects as parameter values, not the values itself

@The-Compiler
Copy link
Member

That part we agree on: It probably doesn't make sense to use pytest.param as an argument value.

What I don't agree on is that your described behavior is an improvement or more user friendly. In my eyes in mixes two things together that don't really belong together, thus introducing more confusion that it solves.

Take the combination of markers, for example: It doesn't make sense for a single argument value to have a marker. Viewed in isolation, the approach of taking each argument value's marker and combining them together to get the tests's markers might make sense, but I'm not convinced it's useful in general - it suddenly means that markers can be applied to a tests individual argument values, which just seems weird and confusing to me.

Just my $0.02 though, let's see what others think.

@vgorkavenko
Copy link
Author

Actually, "combining marks" is one of thinking nice to have. The main aim is getting values from nested ParameterSet objects and merging their ids. In my case, I have many different functions, that build params for tests and I want to combine their values for different tests

@RonnyPfannschmidt
Copy link
Member

can you outline your use-case further, ti may be a good idea to have a completely own mechnaism instead, but without actually seeing what you need to do, all we can do is guesswork

i agree with the sentient that params and parametersets are different "bags of things" in a sense

@vgorkavenko
Copy link
Author

In the term 'ParameterSet' I mean only the current pytest class. The main case is #9074 (comment) and I think this #9074 (comment) resolve it. This idea resolve many other cases, which I can only surmise, but the main point of they - use pre-created pytest.param as part of another pytest.param. It improves reuse codebase for preparing test data and test names readability in collect

@RonnyPfannschmidt
Copy link
Member

@vgorkavenko i think i need a expansion of that, a particular usage-side doesn't reveal the full use-case

i'd like to avoid the X-Y-Problem

for historic context, parameter-set got introduced to remove a really ugly hack that would transfer marks passed as parameterset into split sets of parameters and marks (which came about by organic growth in minimal steps)

this additional feature you propose as i see it right now might be that type of minimal step one wants to avoid,
so i want to understand what the larger step might look like

@vgorkavenko
Copy link
Author

I agree that infinite recursive unpacking in the future will make some ugly things. Maybe do limit nesting so that the user is not tempted to do something like this:

@pytest.mark.parametrize(
    "a,b,c", [
        pytest.param(
            pytest.param(
                1, 
                pytest.param(2, 3, id="this_is_two_and_three"), id="this_is_one"
            ),
            id="all",
        )
    ]
)

On the other hand, this equates pytest.param to actually a simple value, just with an alias and marks
which I think sounds good 🤔

@RonnyPfannschmidt
Copy link
Member

maybe it would help to introduce a explicit flattening helper, your specific use-case is still not clear to me

i dont want to run a open ended experiment with this as time is sparse as is already

@vgorkavenko
Copy link
Author

As I said above, in the main case I want to use pre-created pytest param as part of the set in another pytest param (to use they values and ids and marks) and do this maximal intuitive. It seems to me that we do not understand each other,
so I propose to write you some points that will explain why we cannot unpack pytest params inside one pytest param. I feel like I'm confused :S

@RonnyPfannschmidt
Copy link
Member

i do feel there is a missunderstanding, which is why i want to unroll the use-case,

i'm currently under the impression that your specific use-case is served better by a specific helper as "generalized parameterset flattening" has to figure some holes in the spec for details like ids, marker transfer, composing the final parameterset

@The-Compiler
Copy link
Member

The-Compiler commented Sep 6, 2021

I tend to agree with @RonnyPfannschmidt (though I feel like the example in #9074 (comment) is pretty concrete).

If you have a custom server_config and conn_config helpers, you might as well have a custom configs_to_param helper (rather than pytest.param), which combines them and returns a pytest.param object.

It'd certainly be some less magic than the current proposal - then the remaining question is whether such a helper should live in the core. I think it shouldn't, unless there are other use cases it solves. On first sight, this seems like a quite exotic thing to me.

@RonnyPfannschmidt
Copy link
Member

@The-Compiler i agree that is "concrete in a sense" but as you mention, its a very specific case that can be handled much more simple than a more generalized unpacking that has to handle the edge-cases

i can also envision a more restricted variant of combining them, (where we would allow that when a normal tuple/list is transformed into a parameter-set, we could consider first level parameterset instances that add marks/ids for single values (which could make useful constants for testcase parameter lists)
then the initial presented use-case would be covered, however its still a breaking change unless there is a opt-in/out

@vgorkavenko
Copy link
Author

@RonnyPfannschmidt I like ur idea. Indeed, recursive unpacking is too loud solution of this issue. It will be enough to consider only the first level

@vgorkavenko
Copy link
Author

Needs anything from me to "push" this issue?

@RonnyPfannschmidt
Copy link
Member

currently i dont have the bandwidth for feature work

@vgorkavenko
Copy link
Author

@RonnyPfannschmidt I created PR, please look at it when you find the time

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

4 participants