Fix pytest.mark.parametrize when the argvalue is an iterator

[asottile]

Resolves #5354

[Sup3rGeo]

Thanks @asottile ! If I understand correctly, we were consuming the generator when checking for parametrization arguments to ignore at first so when the real parametrization kicked in there was nothing left for the parameters, right?

[asottile]

Thanks @asottile ! If I understand correctly, we were consuming the generator when checking for parametrization arguments to ignore at first so when the real parametrization kicked in there was nothing left for the parameters, right?

that is exactly right :D

[codecov]

Codecov Report

Merging #5356 into master will decrease coverage by 1.11%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #5356      +/-   ##
=========================================
- Coverage   95.32%   94.2%   -1.12%     
=========================================
  Files         115     115              
  Lines       26384   26391       +7     
  Branches     2607    2607              
=========================================
- Hits        25150   24862     -288     
- Misses        924    1219     +295     
  Partials      310     310

Impacted Files	Coverage Δ
testing/test_mark.py	99.39% <100%> (ø)	⬆️
src/_pytest/mark/structures.py	92.73% <100%> (+0.08%)	⬆️
testing/python/show_fixtures_per_test.py	17.64% <0%> (-82.36%)	⬇️
testing/python/setup_plan.py	20% <0%> (-80%)	⬇️
testing/test_warnings.py	48.63% <0%> (-47%)	⬇️
testing/test_pytester.py	58.3% <0%> (-30.99%)	⬇️
testing/test_tmpdir.py	75.8% <0%> (-23.12%)	⬇️
testing/python/setup_only.py	80.35% <0%> (-19.65%)	⬇️
testing/test_pluginmanager.py	49.57% <0%> (-4.67%)	⬇️
src/_pytest/python.py	89.09% <0%> (-4.25%)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 917195e...0654c34. Read the comment docs.

[nicoddemus]

Thanks @asottile!

Please open the backport PR after this gets merged. 👍

[asottile]

#5357

The procedure I used here was:

git checkout origin/4.6-maintenance -b backport_5356
git cherry-pick ...

[blueyed]

@asottile @nicoddemus
Removed "needs backport" label - I think we should do this after the backport was done, to track closed PRs that still need one.
Agreed?

[RonnyPfannschmidt]

this pr doenst test critical cases in, and it should fail on iterators

[RonnyPfannschmidt]

this breaks down on marker/generator reuse

[blueyed]

@RonnyPfannschmidt please consider creating a new issue - or do you know what's up with the comment, @asottile ?

[asottile]

I think @RonnyPfannschmidt is suggesting this won't work (not at a computer)

m = pytest.mark.parametrize('a', gen()) 

@m 
def test1(a): pass

@m 
def test2(a): pass

[asottile]

But if it doesn't, it never worked so 🤷‍♂️ probably?

[RonnyPfannschmidt]

we already had a issue about reuse of generators

[asottile]

yeah that example above is indeed broken, but wasn't the regression being targetted by this fix:

$ pytest t.py -vv
============================= test session starts ==============================
platform linux -- Python 3.6.7, pytest-4.5.0, py-1.8.0, pluggy-0.12.0 -- /tmp/venv/bin/python3
cachedir: .pytest_cache
rootdir: /tmp
plugins: celery-4.3.0
collected 4 items                                                              

t.py::test1[1] PASSED                                                    [ 25%]
t.py::test1[2] PASSED                                                    [ 50%]
t.py::test1[3] PASSED                                                    [ 75%]
t.py::test2[a0] SKIPPED                                                  [100%]

===================== 3 passed, 1 skipped in 0.01 seconds ======================

[nicoddemus]

Removed "needs backport" label - I think we should do this after the backport was done, to track closed PRs that still need one.
Agreed?

Agreed, I had the same idea yesterday. 👍

[asottile]

Let's not lose track of "did we backport this?" let's replace the label when competed with a backported label

[blueyed]

Ok.
Although there is / should typically be a referencing comment anyway.

We should/could also do backports in batches then - resulting in a single CI run, instead of one for every backport.

[nicoddemus]

Agree with having a label: it is easy to see the labels when listing the PRs. 👍