-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix issue #24: LazyFixture only resolved when used directly, ... #40
base: master
Are you sure you want to change the base?
fix issue #24: LazyFixture only resolved when used directly, ... #40
Conversation
…t not in lists etc.
Hey! Thanks for pr!
Все норм, только чутка холодновато сейчас :) |
Hello @TvoroG. Have you had a chance to review the code? Do you have any comments? Will the PR be merged? Regards, |
def _rec_part(val): | ||
if is_lazy_fixture(val): | ||
return item._request.getfixturevalue(val.name) | ||
elif type(val) == tuple: | ||
return tuple(item._request.getfixturevalue(v.name) if is_lazy_fixture(v) else _rec_part(v) for v in val) | ||
elif type(val) == list: | ||
return list(item._request.getfixturevalue(v.name) if is_lazy_fixture(v) else _rec_part(v) for v in val) | ||
elif isinstance(val, dict): | ||
return {key: item._request.getfixturevalue(v.name) if is_lazy_fixture(v) else _rec_part(v) | ||
for key, v in val.items()} | ||
return val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it is a correct way to solve the problem, because it will try to find lazy fixture inside of every test' pytest.fixture(params)
. At least it should be disabled by default and maybe through the marks to enable this behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry is a problem with dictionaries only? Or with list too?
Actually the library checks it now, so only some more levels are added :)
But I think decrease amount of checks is a good idea. But how to do it?
Add new attr to pytest.fixture?
@pytest.fixture(params=[. . .], check_collections_for_lazy_fixtures=True)
or add specific object:
@pytest.fixture(params=[check_collections_for_lazy_fixtures, . . .])
Will it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the better option:
add an option to pytest.ini lookup for lazy_fixtures (turn off by default).
using decorator @pytest.mark.lookup_for_lazy_fixtures().
If global config is turned on we always look, if it turned off we check for decorator in item.own_markers.
BTW do you concern about slowing down performance, because of lazy_fixtures lookup?
I am not sure it will have a big impact, because usually (at least on project I worked on :) ) test doesn't use big collections with a lot of nested collections inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think about using marks too. I will take a look a little bit later though.
BTW do you concern about slowing down performance, because of lazy_fixtures lookup?
Yep :) Also I don't like unknown structure of the param. Maybe it will be a good idea to specify format so we will know where to lookup lazy_fixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to run some benchmark, but something broke:
C:\Users\artem\PycharmProjects\pytest-lazy-fixture\tests\test_lazyfixture.py:18:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Program Files\python\lib\site-packages\_pytest\pytester.py:993: in getitems
modcol = self.getmodulecol(source)
C:\Program Files\python\lib\site-packages\_pytest\pytester.py:1020: in getmodulecol
self.config = config = self.parseconfigure(path, *configargs)
C:\Program Files\python\lib\site-packages\_pytest\pytester.py:959: in parseconfigure
config._do_configure()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <_pytest.config.Config object at 0x0000016F434B08C8>
def _do_configure(self):
> assert not self._configured
E AssertionError
C:\Program Files\python\lib\site-packages\_pytest\config\__init__.py:634: AssertionError
Do you have any idea what's wrong?
Also I don't like unknown structure of the param. Maybe it will be a good idea to specify format so we will know where to lookup lazy_fixture?
I got your concern, but I think specifying format is a source of many possible issues, the users can muddle up or forget to specify it, also there can be a problem with parsing the format, and it can create bugs in lib, also that can be slower than just checking all collections. BTW we decided to keep it simple and specifying format is not very intuitive and convenient for users. :)
def lazy_fixture(names=None, *args, **kwargs): | ||
if isinstance(names, string_type) and not args and not kwargs: | ||
return LazyFixture(names) | ||
else: | ||
return [LazyFixture(name) for name in names] | ||
elif not kwargs: | ||
names = [names] if isinstance(names, string_type) else names | ||
names, is_tuple = (list(names), True) if isinstance(names, tuple) else (names, False) | ||
names.extend(args) | ||
if is_tuple: | ||
return tuple(LazyFixture(name) for name in names) | ||
else: | ||
return [LazyFixture(name) for name in names] | ||
elif kwargs and not (args or names): | ||
return {key: LazyFixture(value) for key, value in kwargs.items()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is an unnecessary to create lazy fixtures this way. I regret adding a way to pass a list of names to create multiple fixtures :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I find it pretty convenient. Yes it is not so obvious as direct dict or list creating, but at least it helps to make your lines less then 120 symbol lengths. :)
Less code is better. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it simple. If you need dict, list or tuple of lazy fixtures just use appropriate python notation to create this objects. If you are doing it in many places just abstract this logic to your own function.
Hey!
Forget to send comments :) |
Hi @TvoroG and @ArtyomKaltovich, Is this PR in the works? If not, I'm willing to take over, I really need this kind of functionality. |
Hello @elilutsky. Looking up every collection drammatically slow down the performance of the tests, I've tried to do it optional, but failed :) You can try it. |
What if you could somehow explicitly mark a dictionary, list, ... that it needs to be searched for nested things? E.g.:
And maybe there a way of adding some optional syntactic sugar, like this
|
Hey, @ArtyomKaltovich 👋🏼, r u gonna finish this issue, or mb you wanna reassign to somebody else? |
Hello @ruslan-korneev. You can take it if you want. |
Hello the PR has following motivation:
Usage:
More samples can be found in tests.
Extends current syntax for creating list of fixtures:
pytest.lazy_fixture(['one', 'two'])
with more possibilities:
Do not hesitate to express your opinion about code and syntax.
Regards,
Artyom.
P.S.
Как там Казань? Почему творог, а не эчпочмак? :)