Skip to content

Strange test name, used idstyle=None #154

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
arut-grigoryan opened this issue Dec 2, 2020 · 15 comments · Fixed by #155
Closed

Strange test name, used idstyle=None #154

arut-grigoryan opened this issue Dec 2, 2020 · 15 comments · Fixed by #155

Comments

@arut-grigoryan
Copy link

arut-grigoryan commented Dec 2, 2020

Python 3.6.8
pytest-6.1.2
cases-2.7.1

After solving problem #151, some tests took the name from @case(id), but others began to look strange.
Example:
test_file

@pytest.fixture
def validationOff():
    app.config["validation"] = False
    yield
    app.config["validation"] = True

@parametrize_with_cases("inputs,request_data", prefix="case_create_", idstyle=None)
@pytest.mark.usefixtures("clearMongo", "mocked_oid", "mocked_uuid")
def test_create(db, flaskClient, inputs, request_data):
    ...

db, flaskClient, clearMongo, mocked_oid, mocked_uuid - fixtures

case file

class CreateCases(object):
    @case(id="current_object")
    def case_create_1(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="not_valid")
    @pytest.mark.usefixtures("validationOff")
    def case_create_2(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="history_object")
    def case_create_3(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="xml_without_need_namespace")
    def case_create_4(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="xml_without_need_namespace_not_valid")
    @pytest.mark.usefixtures("validationOff")
    def case_create_6(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="xml_without_need_name")
    def case_create_7(self):
       inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="xml_without_need_name_not_valid")
    @pytest.mark.usefixtures("validationOff")
    def case_create_8(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

    @case(id="xml_invalid")
    def case_create_9(self):
        inputs = ["foo", "bar"]
        request_data = {"foo": "dfg", "data": "asd"}
        return inputs, request_data

Names:
test_create[current_object]
test_create[not_valid]
test_create[test_create_inputs_request_data_is_P2toP3-history_object] ?? fixture validationOff not used
test_create[test_create_inputs_request_data_is_P2toP3-xml_without_need_namespace] ?? fixture validationOff not used
test_create[xml_without_need_namespace_not_valid]
test_create[xml_without_need_name]
test_create[xml_without_need_name_not_valid]
test_create[xml_invalid]

Without idstyle=None names:
test_create[current_object]
test_create[inputs_request_data_is_not_valid]
test_create[inputs_request_data_is_P2toP3-history_object] ?? fixture validationOff not used
test_create[inputs_request_data_is_P2toP3-xml_without_need_namespace] ?? fixture validationOff not used
test_create[inputs_request_data_is_xml_without_need_namespace_not_valid]
test_create[xml_without_need_name]
test_create[inputs_request_data_is_xml_without_need_name_not_valid]
test_create[xml_invalid]

@smarie
Copy link
Owner

smarie commented Dec 2, 2020

the P2toP3 means "from parameter 2 to parameter 3". However I'm not sure it is supposed to appear when idstyle is None.

Also when idstyle is not none, it is a bit strange that "inputs_request_data_is_" is not present in all ids ?

I'll try to reproduce this one of these days and check what happens.

Do you confirm that the usefixtures mark behaves correctly here ?

@arut-grigoryan
Copy link
Author

arut-grigoryan commented Dec 2, 2020

Usefixtures mark works correctly.
As I understand it "inputs_request_data_is_" does not appear because there I am not using fixture validationOff in these tests.

@smarie
Copy link
Owner

smarie commented Dec 3, 2020

Here is a summary of the mess. I modded your example to reference a parametrized fixture too.

image

To convert it into a list of facts for current @parametrize implem, to decide what to do:

  1. idstyle is only used when at least one parameter is a fixture reference. So on the left column we see that top and bottom lead to the same ids

  2. when at least a fixture reference is present, a fixture union is created. So a new parameter appears upfront, doing the switch between alternatives P0, not_valid, P2-P3, xml_without_namespace_notvalid, etc. Two things make it hard/unintuitive to understand for users:

    • I thought that it would be overkill to create one fixture for each parameter, this is why I create a fixture for each consecutive group of parameters (P2-P3). Therefore the id prefix is the same for them.
    • I have put some "smart simplification" (maybe not a good thing after all) so that alternatives with just a single parameter (P0) are not parametrized and therefore do not have the prefix id
  3. the "no style" ids

    a. have a trailing underscore for fixture refs. This is due to the fact that the fixtures created automatically for the cases have an auto-generated name that should not conflict with existing symbols. For this trailing _ are created. This happens for the no style test, because the no style test appears second in the tests order. If the "explicit style" was second, it would have been the one with trailing underscores everywhere

    b. are not taken into account at all for parameter groups (P2toP3).

@smarie
Copy link
Owner

smarie commented Dec 3, 2020

Proposed solutions so far:

  • add a custom id parameter to fixture_ref so that we can fix issue 3 a
  • try to track down where 3b comes from and fix it

Pending questions:

  • should I de-simplify some of the params-related "smarts" in 2. ? I do not really know the cost of creating one fixture per case in pytest, as opposed to trying to create less. But at least I could de-simplify the one for single parameters, as it breaks consistency.

smarie pushed a commit that referenced this issue Dec 3, 2020
smarie pushed a commit that referenced this issue Dec 3, 2020
 - added an `alternative_index` attribute in `UnionFixtureAlternative` and uniformized the id making process for fixture unions and parametrize alternatives.
 - in particular added an `id` argument to `fixture_ref` and filled this `id` in `@parametrize_with_cases` so that the id is always the one from the case_id
@smarie
Copy link
Owner

smarie commented Dec 3, 2020

I have done most of the harmonization work now with #155

The last thing that needs to be settled is the templates for the id styles. Indeed I feel that they could be improved to be more explicit. Here is a proposal using slash / to denote a fixture union alternative

  • 'explicit' would lead to <arg_names>/<argvalue_idx>-<argvalue_id> , so something like inputs_request_data/P0-current_object or inputs_request_data/P2toP3-history_object

  • 'compact' would lead to /<argvalue_idx>-<argvalue_id> so /P0-current_object or or /P2toP3-history_object

  • 'none' would lead to <argvalue_id> so current_object or or history_object. Indeed here it does not seem easily feasible to prepend /

What do you think about this ? any other suggestion welcome. I also thought about putting the index numbers in brackets [P0] or [P2toP3].. but that's a double bracket

@smarie
Copy link
Owner

smarie commented Dec 4, 2020

@plammens you might have an opinion on this last comment ? do not hesitate to jump in :)

@arut-grigoryan
Copy link
Author

Thanks for your hard work. I don't really understand why the name of the test should change if there is a fixture in the arguments of the case function but it is not parameterized?
#154 (comment) - very good idea i like)

@plammens
Copy link
Contributor

plammens commented Dec 4, 2020

@plammens you might have an opinion on this last comment ? do not hesitate to jump in :)

Personally I find these aspects of the current naming system confusing:

  • Referring to two "unpacked" case-parametrized parameters by just joining the names with underscores, e.g., the parameters inputs, request_data are shown as inputs_request_data. As you add more and more parameters and fixtures, the test node id becomes increasingly unreadable; it's just a long string of underscores and dashes (which at least to me are not easily distinguishable at first sight). I would make this more explicit by using something like "(inputs, request_data)" or "inputs, request_data" (perhaps without the space) as the argnames id.

  • I'm not sure the argvalue index adds any useful information—if I actually understand what it's doing, which I believe is just indicating the indices of the parameters of the test function being "filled". If we already have the parameter names, the index is just duplicating information, because the parameters of the test function will always have unique names, right? Moreover I find the P0 and P2toP3 notation style a bit ugly. I would either remove this entirely (even from the "explicit" style) or change the notation.

    • First, by the looks of things, these are using 0-based indices, correct? If so I find it confusing that they use inclusive ranges (i.e. P2toP3 means indices 2 and 3), since usually ranges in 0-based indexing are half-open, while in 1-based indexing they're closed. I would find 2-4 more consistent.

    • An alternative notation would be as you said to use brackets. I wouldn't say nested square brackets is too much of a problem, as long as they're balanced :) . Otherwise I would use angle brackets, e.g. <0> or <2-4> (I don't like the whole P thing). Or maybe we can even use slice notation, which should be intuitive for Python developers: [2:4] (in this case it would be really important to use half-open ranges to mimick slice behaviour).

  • I'm also not convinced by using dashes to separate <argvalue_idx> and <argvalue_id>, as dashes are already used to separate entirely distinct parameters or parameter groups, which again can be visually confusing. As mentioned in the previous bullet I would either remove the argvalue index entirely or use some other notation.

  • If it were to be kept, I would also put the argvalue idx on the other side of the slash: it's providing information about the parameters/argument names, not the particular test case.

  • (Also, unrelated to this particular issue, but while I'm at it... I don't like the foo_is_bar notation for fixture unions. Again, here I would introduce some other notation, eg. foo::bar, as if foo were a "namespace".)

My stance can be summarised as "introduce more symbols and alternative notations in order to make each have a(n almost) unique meaning, so that the test node id string can become more compact and readable" 🙂

So, to put everything I've said into a single example, it would look something like this:

(inputs,request_data)<2-4>/history-object

In any case what you've proposed, @smarie, is already a great improvement, I would say. These are just my personal itches :)

@arut-grigoryan
Copy link
Author

Hello. What is the status of the issue?

@smarie
Copy link
Owner

smarie commented Dec 11, 2020

Thanks to both of you for the feedback ! I am finalizing a PR with the following principles:

fixture_union

  • the notation appearing in the test ids should be easily identifiable as "this is a fixture union alternative". I prefer not to choose :: (even if it the suggestion is cool!) because this symbol is used already in pytest ids to separate the python file, test class and test function. So I go for /. So a fixture_union("a", [b, c]) will result in ids ["/b", "/c"] by default.

  • idstyle in fixture unions:

    • "compact" will lead to ["/b", "/c"]. The default style. It makes visible that each parameter is a fixture union alternative and not a "standard" parameter. However it does not overwhelm the user with too long ids

    • "explicit" will lead to ["a/b", "a/c"]. This is mostly for debugging, we prepend the name of the union fixture.

    • "none" will lead to ["b", "c"]. This is or users who do not like the / prefix.

    • <a callable>. For example str will lead to ["a/0/b", "a/1/c"], which is an easy way to get the finest debug information on union fixture and its alternatives

@parametrize

  • by default, the user should not feel any special pain/change in parametrize due to fixture_ref being present. So by default @parametrize will have an idstyle=None. So @parametrize("a", [1, fixture_ref(b), 2, 3]) will lead to ids ["1", "b", "2", "3"]

  • ids works exactly the same way as in @pytest.mark.parametrize. So passing a callable will receive the argvalues [1, fixture_ref(b), 2, 3].

  • idstyle in parametrize is mostly for debug:

    • "none" will lead to ["1", "b", "2", "3"]. This is the default behaviour to mimic @pytest.mark.parametrize (users don't care about the fixture union created behind the scenes)

    • "compact" will lead to ["/1", "/b", "/P2:4-2", "/P2:4-3"]. This is for debug. It makes visible that there are 3 (and not 4) fixture union alternatives and that the third one is parametrized with two params.

    • "explicit" will lead to ["a/1", "a/b", "a/P2:4-2", "a/P2:4-3"]. This is for debugging, we prepend the argname. In case of multiple argnames, the prefix will be (a1, a2) as suggested by @plammens

    • <a callable>. For example str will lead to "(argnames,)/<idx>/<argvalues>", which is an easy way to get the finest debug information on union fixture and its alternatives

    • if custom ids are provided, they will be used to reformat the <argvalue> part of the idstyle. But users will still be able to debug by setting an "explicit" style, even with a custom list/generator/callable of ids.

@parametrize_with_cases

  • this will work entirely similar to @parametrize: idstyle=None by default

  • If a callable is provided as ids, it will receive the case functions instead of the lazy_value and fixture_refs created behind the scenes. Users will be able to use get_case_id(f) in this callable to get the case id easily.

@smarie
Copy link
Owner

smarie commented Dec 16, 2020

Finally, all tests pass of all versions of python and pytest: https://travis-ci.com/github/smarie/python-pytest-cases/builds/209061483 . I'll merge this PR #155 and release version 2.8.0 in a few minutes.

I opened an issue on pytest because it was quite hard to make the extra useless id from the multi-param fixture disappear in pytest 5.4+ : pytest-dev/pytest#8155

Please let me know of any feedback about the new id system ! Thanks again to both of you for challenging me with this topic, that was the "less interesting" in terms of test running mechanism but was extremely challenging to get done properly including all combinations of custom ids and marks everywhere (and all versions of python and pytest)

@smarie
Copy link
Owner

smarie commented Dec 16, 2020

3.0.0 released (I finally chose to make it a major version bump since it was quite a major refactoring!). Let me know !

@arut-grigoryan
Copy link
Author

Thank you very much, we are working on)

@plammens
Copy link
Contributor

Yes, thank you for all the hard work @smarie !

@smarie
Copy link
Owner

smarie commented Dec 17, 2020

You're welcome. Completing this made me realize that there is still progress to do in terms of simplification, I opened #170 .

Double-thanks to both of you for taking time to try the features and let me know of the issues !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants