Issue 2836 fixture collection bug#2862
Issue 2836 fixture collection bug#2862RonnyPfannschmidt merged 13 commits intopytest-dev:masterfrom tom-dalton-fanduel:issue-2836-fixture-collection-bug
Conversation
nimbol
left a comment
nimbol
left a comment
There was a problem hiding this comment.
Just wanted to input my €0.02. Overall this looks like a solid improvement to me.
_pytest/fixtures.py
Outdated
| return tuple(self._matchfactories(fixturedefs, nodeid)) | ||
|
|
||
| @classmethod | ||
| def _splitnode(cls, nodeid): |
There was a problem hiding this comment.
It appears that this can be a staticmethod instead. The class isn't referenced anywhere in this method.
There was a problem hiding this comment.
I'm not a fan of staticmethods, though mainly for inheritance related reasons and doubt anyone is subclassing fixtruemanager, let alone doing so and then wanting to change these. It feels like they should probably be plain functions in a 'node' module somewhere but hopefully someone more familiar with pytest can guide me on that... (see also my comment about _splitnode in the description).
There was a problem hiding this comment.
since both helpers are completely unrelated to the class, how about just moving them out of the class instead
_pytest/fixtures.py
Outdated
| if '::' in last_part: | ||
| # Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()' | ||
| parts[-1:] = last_part.split("::") | ||
| return parts |
There was a problem hiding this comment.
Just a suggestion to simplify the code: remove the special case for the empty nodeid. The following would return functionally equivalent results (it would just return [''] instead of []) without special-casing anything:
sep = py.path.local.sep
parts = nodeid.split(sep)
parts[-1:] = parts[-1].split("::")
return parts
There was a problem hiding this comment.
Thanks, I've shortened this down a bit (and made the docstring longer to compensate ;-))
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
at first glance this is a good and practical approach given that we didn't make the node tree available out there
i believe the helper functions should be put into normal functions for now
personally i hope we can eventually put in a better way to match fixtures <> nodes than walking strings, but since that takes time i'm rather glad you came up with a simple and easy to understand solution to the problem at hand
👍
changelog/2836.bug
Outdated
| @@ -0,0 +1,3 @@ | |||
| Attempted fix for https://github.com/pytest-dev/pytest/issues/2836 | |||
There was a problem hiding this comment.
i propose match fixture paths against actual path segments in order to avoid matching folders which share a prefix
CC @nicoddemus
_pytest/fixtures.py
Outdated
| return tuple(self._matchfactories(fixturedefs, nodeid)) | ||
|
|
||
| @classmethod | ||
| def _splitnode(cls, nodeid): |
There was a problem hiding this comment.
since both helpers are completely unrelated to the class, how about just moving them out of the class instead
…ode-splitting code :-/
|
Thanks a lot for the feedback - I've pushed changes as suggested. Have put the node functions in a new module but let me know if you'd rather they remained in |
|
the linting issues are newly found issues we need to addto the ignore list CC @nicoddemus |
Also fixed 'E704 multiple statements on one line (def)' in python_api
|
Thanks a lot @tom-dalton-fanduel for the PR, we appreciate it! I have added the two flake8 new exceptions to the ignore list (E722: "do not use bare except" and E741: "ambiguous variable name 'l'") and fixed another one. After CI passes I we can merge this. 👍 |
|
Tests are failing on Windows, I suspect we also need to split by |
_pytest/nodes.py
Outdated
| if nodeid == '': | ||
| # If there is no root node at all, return an empty list so the caller's logic can remain sane | ||
| return [] | ||
| parts = nodeid.split(py.path.local.sep) |
There was a problem hiding this comment.
https://github.com/pytest-dev/pytest/blob/master/_pytest/main.py#L519-L527 seems to generally replace the path separator with '/' for consistent node ids across operating systems, i think introducing a constant for that is necessary
There was a problem hiding this comment.
I've created a constant SEP in the nodes module, and used that in all the places I could see that used a literal '/' in the context of node ids. I subsequently noticed there is a Node class in main.py (I hadn't noticed that before now), but I can't see an (easy) way to move the new plain functions into that class, since they're used with strings rather than actual node objects.
…lton-fanduel/pytest into issue-2836-fixture-collection-bug
|
@nicoddemus is the failing Windows build visible (to me) anywhere? I don't see anything obviously windows-y in Travis... Edit: Nevermind - I see that Appveyor is the Windows CI builds 🤦♂️ |
|
this now is a beautiful cleanup 👍 - the ci issues are from a flake8 update and a bug that @nicoddemus is currently working on |
|
merging as the failures are unrelated, good work 👍 |
|
|
|
Hahaha that's an awesome gif! 😄 |
|
Yep - while searching for an appropriate "I am officially a contibutor to pytest, hooray" gif, I came across Party Parrot as a Service and couldn't resist! |
|
I can say with all certainty that this is one of the most useful services of all time. 😆 |
5 participants
Attempted fix for #2836.
The existing handling of 'nodeid' when determining what fixtures are visible is overly simplistic - it considers 'tests/food' to be a 'child' node of 'tests/foo', when in fact they are sibling nodes. This causes fixtures from 'tests/foo/conftest.py' to be used in tests like 'tests/food/test_food.py'.
This PR makes the hanlding more sophisticated - splitting the node id into components and then ensuring the base node id parts fully match the target node id's initial parts. This is still a little imperfect, in that would treat
foo/bar::Baz::()as if it was the same asfoo/bar/Baz/(). That seems a somewhat unlikely situation...I'm not super keen on having
_splitnodeand_ischildnodeas members of FixtureManager, but I don't really see any obvious other place for them and I was reluctant to create a while new module/class for these two 'nodeid' helper functions. I'm not 100% sure where the spec/definiton of a node id comes from, so maybe there is a better place, very open to suggestions on that. If someone with some better knowledge of what 'nodeid's look like can have a look over this it would be good - hopefulyl I haven't missed any edge cases but without having a good definition I am slightly worried about it.The fixtures module didn't (seem to) have any existing unit tests, so I've added one for my new
FixtureManager_ischildnodemethod (mainly for my own sanity). I've also added a more 'integration'-y test totest_collection.pywhich will catch regression and (hopefully) follows the style of other tests there too. Hopefullytest_collectionis the right place for that test - certainly the fixture code I've modified seems to be run as part of the collection phase.