Skip to content
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

WIP: debug pypy3 failures due to "Unroll calls to any #5062 (#5103)" #5334

Closed
wants to merge 8 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented May 29, 2019

This reverts commit 2b9ca34, reversing
changes made to 0a57124.

This is when pypy3 started to fail (https://travis-ci.org/pytest-dev/pytest/builds/537992776).

Ref: #5317

/cc @Tadaboody

This reverts commit 2b9ca34, reversing
changes made to 0a57124.
@nicoddemus
Copy link
Member

How about if instead of reverting this completely, we don't try to unroll calls to any in pypy3 instead? It seems to me this would make it easier to fix the problem than trying to reintroduce the entire feature later.

@blueyed
Copy link
Contributor Author

blueyed commented May 29, 2019

How about if instead of reverting this completely

This is just for testing / investigating. I do not want / plan to revert it actually (as-is).

@nicoddemus
Copy link
Member

Oh OK!

@blueyed
Copy link
Contributor Author

blueyed commented May 29, 2019

The build passed: https://travis-ci.org/pytest-dev/pytest/jobs/538946951
But IIRC it is rather flaky instead, due to something appearing on stderr.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #5334 into features will decrease coverage by 1.49%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           features    #5334     +/-   ##
===========================================
- Coverage     94.71%   93.22%   -1.5%     
===========================================
  Files           115      115             
  Lines         26333    26249     -84     
  Branches       2601     2591     -10     
===========================================
- Hits          24942    24470    -472     
- Misses         1080     1458    +378     
- Partials        311      321     +10
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.77% <ø> (+0.23%) ⬆️
testing/test_assertrewrite.py 84.06% <ø> (-0.23%) ⬇️
testing/test_reports.py 52.48% <0%> (-47.52%) ⬇️
testing/test_pytester.py 54.04% <0%> (-35.25%) ⬇️
testing/python/collect.py 81.66% <0%> (-17.7%) ⬇️
testing/test_terminal.py 84.6% <0%> (-14.94%) ⬇️
testing/python/integration.py 80.71% <0%> (-10.72%) ⬇️
testing/python/setup_only.py 92.85% <0%> (-7.15%) ⬇️
src/_pytest/pytester.py 86.11% <0%> (-3.72%) ⬇️
src/_pytest/reports.py 92.11% <0%> (-2.96%) ⬇️
... and 18 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 882f3a4...b474341. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented May 29, 2019

From the warnings summary:

.tox/pypy3-xdist/site-packages/_pytest/fixtures.py:1270
/home/travis/build/pytest-dev/pytest/.tox/pypy3-xdist/site-packages/_pytest/fixtures.py:1270: ResourceWarning: unclosed file <_io.FileIO fd=445 mode='rb' closefd=True>
marker = getfixturemarker(obj)

@blueyed blueyed changed the title WIP: Test: Revert "Unroll calls to any #5062 (#5103)" WIP: debug pypy3 failures due to "Unroll calls to any #5062 (#5103)" May 30, 2019
@nicoddemus
Copy link
Member

nicoddemus commented May 30, 2019

Thanks for the last commmit.

Argh this makes it harder to debug because I can't get pypy3 to work on Windows. 😞

FTR:

Python 3.6.1 (784b254d6699, Apr 16 2019, 12:10:48)
[PyPy 7.1.1-beta0 with MSC v.1910 32 bit] on win32
Type "help", "copyright", "credits" or "license" for more information.
And now for something completely different: ``no random bugs in random, after
all''
>>>> from nt import _getfinalpathname
>>>> _getfinalpathname('c:/foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError
>>>>

(_getfinalpathname is used internally by pathlib)

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

Good first shot though I guess.
Might be worth to check if there are warnings also in pypy already, and/or if this is really just maybe flaky due to "output on stderr" (and happened before, but not that much).

@nicoddemus
Copy link
Member

Might be worth to check if there are warnings also in pypy already, and/or if this is really just maybe flaky due to "output on stderr" (and happened before, but not that much).

Hmmm interesting, we can try with -s next.

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

Hmmm interesting, we can try with -s next.

-s appears to be pretty broken in general already (started debugging / looking into it in #5337 - which is against master)

@nicoddemus
Copy link
Member

-s and xdist don't play nice together. We might try pypy3 without xdist and only a single test file, to see if we can reproduce the issue with a smaller number of tests and shorter feedback. 🤔

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

I could imagine that rewriting might happen in a place where PyPy does some resource handling itself (https://github.com/pytest-dev/pytest/pull/5103/files#diff-a54d1eaf9d064e9a7d25b1152476bb3dR967).

Bisecting/debugging this quickly is not fun, since there are warnings also on master already.
(Would be easier now with enforced --lsof being in place before already)

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

@nicoddemus
Copy link
Member

sigh now travis won't start

@nicoddemus
Copy link
Member

It was because it had a merge conflict then.

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

btw, just out of interest: could you use pypy3 in Docker maybe?
Also the nt._getfinalpathname should get reported/fixed probably.

@nicoddemus
Copy link
Member

btw, just out of interest: could you use pypy3 in Docker maybe?

I have one or two VMs laying around, I guess there's no other way but try this.

Also the nt._getfinalpathname should get reported/fixed probably.

Definitely, didn't do that yesterday as it was late and I was about to clock out. Will do it later.

@nicoddemus
Copy link
Member

nicoddemus commented May 30, 2019

Spent the hour or so investigating this. Some observations:

  1. I get warnings by just running tox -e pypy3 -- testing/acceptance_test.py.
  2. If I add --runpytest=subprocess, I don't see any warnings. 🤔

I will clock out now, might give this another try tomorrow.

But I'm not confident we will find the problem anytime soon. The question is, should this block the 4.6 release? IMHO no; while it is unfortunate that we are getting more of those warnings now, it seems they affect mostly pytest's own test suite, so this is something we can deal later.

@blueyed
Copy link
Contributor Author

blueyed commented May 30, 2019

should this block the 4.6 release?

No.
We should either allow failures there, or just remove it temporarily.

@nicoddemus
Copy link
Member

I agree with allowing failures. I believe you had a PR with that approach right? Could you resurrect it please? 😁

@blueyed
Copy link
Contributor Author

blueyed commented Jun 3, 2019

Closing for now.

@blueyed blueyed closed this Jun 3, 2019
@blueyed blueyed deleted the test-revert branch June 3, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants