Skip to content

Don't skip fixtures that are substrings of params #926

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 11, 2015
Merged

Don't skip fixtures that are substrings of params #926

merged 3 commits into from
Aug 11, 2015

Conversation

untitaker
Copy link
Contributor

Fix #736

untitaker
untitaker reviewed Aug 8, 2015
View reviewed changes
_pytest/python.py
@@ -1777,7 +1777,7 @@ def pytest_generate_tests(self, metafunc):
if fixturedef.params is not None:
func_params = getattr(getattr(metafunc.function, 'parametrize', None), 'args', [[None]])
# skip directly parametrized arguments
if argname not in func_params and argname not in func_params[0]:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't figure out why this line is needed. It has been introduced with https://bitbucket.org/pytest-dev/pytest/pull-requests/257/allow-to-override-parametrized-fixtures

I suppose it's due to strings like value,othervalue, but it doesn't seem like it's actually needed (see new python/collect.py-testcases). In other words, no item of func_params ever contains a comma.

@untitaker
Copy link
Contributor Author

cc @bubenkoff

nicoddemus
nicoddemus reviewed Aug 8, 2015
View reviewed changes
testing/python/collect.py

@pytest.mark.parametrize('other,value', [('foo', 'overridden')])
def test_overridden_via_multiparam(other, value):
assert value == 'overridden'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for completeness, can you add a assert other == 'foo' here?

@nicoddemus
Copy link
Member

Thanks for the PR @untitaker! 😄

Could you please also add a note to the CHANGELOG (make sure to add a sentence like "Thanks to Markus Unterwaditzer for the PR") and add yourself to AUTHORS? Thanks!

untitaker added a commit to untitaker/pytest that referenced this pull request Aug 8, 2015
@untitaker
Update changelog for pytest-dev#926
2cbdca2 
Also fix some inconsistencies in the changelog on the way.
@untitaker
Copy link
Contributor Author

Done. I'm not really sure what I did was the right fix though -- I think this should be confirmed somehow.

@nicoddemus
Copy link
Member

Thanks! I'm not familiar with that part of the code, but based from the tests it seems the change is OK to me.

Let's wait for @bubenkoff's feedback. 😄

nicoddemus
nicoddemus reviewed Aug 8, 2015
View reviewed changes
CHANGELOG
@@ -1,10 +1,14 @@
2.8.0.dev (compared to 2.7.X)
-----------------------------

- Fix issue736: Fix a bug where fixture params would be discarded when combined
with parametrization markers.
Thanks to Markus Unterwaditzer for the PR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add yourself to AUTHORS.

@untitaker
Copy link
Contributor Author

Added myself and also resorted list.

@nicoddemus
Copy link
Member

Thanks! 🍻

@RonnyPfannschmidt
Copy link
Member

The code should use a string split there I think, and the implementation seems incomplete to begin with

this one seems tricky

@untitaker
Copy link
Contributor Author

The thing is, I couldn't craft a testcase that fails with this patch.

On 8 August 2015 06:22:41 CEST, Ronny Pfannschmidt [email protected] wrote:

The code should use a string split there I think, and the
implementation seems incomplete to begin with

this one seems tricky

Reply to this email directly or view it on GitHub:
#926 (comment)

Sent from my phone. Please excuse my brevity.

@RonnyPfannschmidt
Copy link
Member

since its an improvements i'm + 1 on merging it anyway, but i suspect we will find an issue at some point in future

@untitaker
Copy link
Contributor Author

If you like, I could add an extra assertion:

assert not any(',' in x for x in func_params)

@RonnyPfannschmidt
Copy link
Member

i don't think that would help, i'd really love to have marker signatures at that point

func params is the argument tuple of the marker and it should check the first argument and always turn it into a tuple of names for that particular check (i.e. if its a string, do a ','. split, then do the in check

@untitaker
Copy link
Contributor Author

My point is that this particular piece of code apparently never gets to see a list of strings that contain commas. I can add that assertion and the tests still pass.

untitaker added a commit to untitaker/pytest that referenced this pull request Aug 8, 2015
@untitaker
Update changelog for pytest-dev#926
a9495cd 
Also fix some inconsistencies in the changelog on the way.
@untitaker
Copy link
Contributor Author

Rebased to resolve conflicts in CHANGELOG.

@untitaker
Copy link
Contributor Author

Anything else needed to get this merged?

On 8 August 2015 13:41:26 CEST, Ronny Pfannschmidt [email protected] wrote:

i don't think that would help, i'd really love to have marker
signatures at that point

func params is the argument tuple of the marker and it should check the
first argument and always turn it into a tuple of names for that
particular check (i.e. if its a string, do a ','. split, then do the in
check

Reply to this email directly or view it on GitHub:
#926 (comment)

Sent from my phone. Please excuse my brevity.

@nicoddemus
Copy link
Member

Looks 👍 to me!

@RonnyPfannschmidt, if you're OK with it, could you please merge this? 😄

@RonnyPfannschmidt
Copy link
Member

ok on the merge, will pull in once im at home

github really needs a merge strategy for CHANGELOG

@The-Compiler
Copy link
Member

If GitHub reads .gitattributes, we could add something like CHANGELOG merge=union to it. I'll experiment with it later today if I have time.

@bubenkoff
Copy link
Member

nice!

On 10/08/2015, Florian Bruhin [email protected] wrote:

If GitHub reads .gitattributes, we could add something like CHANGELOG merge=union to it. I'll experiment with it later today if I have time.

Reply to this email directly or view it on GitHub:
#926 (comment)

Anatoly Bubenkov

@RonnyPfannschmidt
Copy link
Member

@The-Compiler great idea 👍

@nicoddemus
Copy link
Member

@The-Compiler great idea 👍

👍

@nicoddemus
Copy link
Member

I did a quick test in pytest-qt by adding a .gitattributes like @The-Compiler said, and it seems to work fine in my local tests. 😄

@nicoddemus
Copy link
Member

Hmm I just pushed a .gitattributesto master in 681e502, but GitHub is still marking this PR as conflicted...

untitaker added a commit to untitaker/pytest that referenced this pull request Aug 10, 2015
@untitaker
Update changelog for pytest-dev#926
0cacd32 
Also fix some inconsistencies in the changelog on the way.
@untitaker
Copy link
Contributor Author

Rebase worked fine/nicer though.

@nicoddemus
Copy link
Member

Oh well, let's keep an eye on future PRs then. 😄

untitaker added a commit to untitaker/pytest that referenced this pull request Aug 10, 2015
@untitaker
Update changelog for pytest-dev#926
8c05ebc 
Also fix some inconsistencies in the changelog on the way.
@untitaker
Copy link
Contributor Author

Just temporarily undid the rebase and it indeed seems that GitHub doesn't care about the .gitattributes file. :(

@nicoddemus
Copy link
Member

😑

@untitaker
Update changelog for pytest-dev#926
cafd71e 
Also fix some inconsistencies in the changelog on the way.
@untitaker
Add myself to authors
1d5215a
nicoddemus added a commit that referenced this pull request Aug 11, 2015
@nicoddemus
Merge pull request #926 from untitaker/multifixture
73fdda0 
Don't skip fixtures that are substrings of params
@nicoddemus nicoddemus merged commit 73fdda0 into pytest-dev:master Aug 11, 2015
@nicoddemus
Copy link
Member

Thanks again @untitaker!

This was referenced Aug 28, 2015
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bubenkoff bubenkoff

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@untitaker @nicoddemus @RonnyPfannschmidt @The-Compiler @bubenkoff