Turn warnigns into errors in the test suite #187

Carreau · 2017-06-07T02:01:07Z

No description provided.

Carreau · 2017-06-07T02:22:51Z

Hum apparently it's in core pytest since 2 weeks ago. Is that ok to require py.test >= 3.1 ?

njsmith · 2017-06-07T03:20:56Z

Requiring a new pytest is totally fine.

You might want to modify appveyor.yml as well.

codecov · 2017-06-07T03:40:35Z

Codecov Report

Merging #187 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #187   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files          53       53           
  Lines        6129     6129           
  Branches      476      476           
=======================================
  Hits         6034     6034           
  Misses         80       80           
  Partials       15       15

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 496493a...78500a8. Read the comment docs.

Carreau · 2017-06-07T04:52:21Z

Wooow. The travis job from my fork got a coredump, hopefully the PR is fine, but it's concerning.

njsmith · 2017-06-07T09:01:55Z

got a coredump

Umm. Wow. That is definitely a bit worrisome. Your link doesn't work for me, though... does it say anything useful?

(I have a suspicion it might be because ResourceWarning and Warning: coroutine '...' was never awaited run in a weird __del__ context and python might not be expecting exceptions here.)

njsmith · 2017-06-07T09:33:14Z

(I should say: specifically what I'm nervous of is that this might be a real bug in Python that we might hit again on random test runs depending on the phase of the moon etc. If so we need to figure it out and do whatever we have to to make sure it never happens again. Flaky tests are a constant issue for an I/O library like trio, so I'm super super vigilant about trying to keep them out. Anything else and you end up with like N different bugs that each causes a random test failure in 1/N times and it's unbearable.)

Carreau · 2017-06-07T17:03:04Z

Link updated.
No nothing useful, as it's on travis I can't get it. Python 3.5.2:

collected 213 items 
�[0m
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_epoll.py .
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_io.py ..................................
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_ki.py .........
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_local.py ......
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_multierror.py ...........
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_parking_lot.py ....
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_result.py ......
../../../../virtualenv/python3.5.2/lib/python3.5/site-packages/trio/_core/tests/test_run.py .....................................ci/travis.sh: line 55:  1366 Aborted                 (core dumped) pytest -W error -ra --run-slow ${INSTALLDIR} --cov="$INSTALLDIR" --cov-config=../.coveragerc

travis_time:end:003d71f4:start=1496806863236690128,finish=1496806912040002488,duration=48803312360
�[0K
�[31;1mThe command "ci/travis.sh" exited with 134.�[0m

Carreau · 2017-06-07T17:19:44Z

And it does trigger on travis about every-other time, but apparently AFAICT only 3.5 and 3.5.2 are affected. Relaunch most of the jobs of this batch a couple of time and only 3.5. and 3.5.2 got failures. So I'm tempted to say it's "fixed".

njsmith · 2017-06-07T19:02:47Z

If it's only 3.5 then that's great for the python devs, but not so helpful for us, since we do want to keep testing on 3.5 :-).

I left pytest -W error trio running in a loop for a while last night on my laptop using debian's version of 3.5, and couldn't reproduce.

I guess next step will be to figure out how to get a stack trace, or otherwise figure out where exactly it's crashing...

njsmith · 2017-06-07T19:05:38Z

Just getting a failing run with -v -s added might be informative. Especially if we can then compare against the same run but with -W always.

Carreau · 2017-06-07T19:12:54Z

Just getting a failing run with -v -s added might be informative. Especially if we can then compare against the same run but with -W always.

I'll try to do that, and see fi I can bisect which test(s) create that.
I may attempt to run that using their docker image to reproduce this.

njsmith · 2017-06-07T19:35:29Z

I just built a copy of 3.5.0 locally, and the first time running ~/src/cpython/b/install/bin/pytest -W error -v trio got a segfault.

It seems to be in trio/_core/tests/test_run.py::test_error_in_run_loop, which is extremely suspicious because that's the one test we have that intentionally triggers a Warning: coroutine '...' was never awaited

njsmith · 2017-06-07T19:38:06Z

Misc/NEWS for 3.5.3 says:

Issue 27811: Fix a crash when a coroutine that has not been awaited is finalized with warnings-as-errors enabled.

In addition to cluttering up the test output now that pytest collects and shows warnings, this causes segfaults on some CI builds when run with -W error (see python-triogh-187).

njsmith · 2017-06-07T19:50:10Z

See gh-189

Carreau · 2017-06-07T19:57:41Z

Ha, good catch thanks !

njsmith · 2017-06-07T20:42:35Z

We still need to come up with something for resource warnings and unawaited coroutine warnings (#169) – ironically these are the two types of warnings that this PR doesn't catch (because they're both generated from __del__ methods, so can't raise errors), and also the two types of warnings that we actually have in our test suite currently :-). But this is definitely helpful to make sure we don't accidentally add more warnings we weren't expecting. Thanks!

Carreau · 2017-06-07T21:26:12Z

In my other PR, I do attach the un awaited coroutines to NonAwaitedCoroutine error; which does allow to catch the exception and potentially consume them. I can or maybe (should ?) extend it with a context manager that keep a ref to all non-awaited coroutines within it ?

njsmith · 2017-06-07T22:51:51Z

Whoops, meant to merge this :-)

Carreau · 2017-06-07T22:53:18Z

Thanks.

njsmith · 2017-06-08T00:47:26Z

...wtf, somehow the segfault still happened after merging into master: https://travis-ci.org/python-trio/trio/builds/240581553

...ugh, yeah, I can reproduce locally too if I re-run it enough times. Apparently #189 didn't actually fix anything.

njsmith · 2017-06-08T00:55:14Z

#190

njsmith · 2017-06-08T10:03:29Z

Oh right, with all this I almost forgot. My rule is anyone whose PR gets merged gets offered a commit bit :-). Want one? You can participate as much or as little as you like.

Carreau · 2017-06-09T03:30:21Z

Oh right, with all this I almost forgot. My rule is anyone whose PR gets merged gets offered a commit bit :-). Want one? You can participate as much or as little as you like.

Happily, that will force me to try to review a bit more code :-)
Side question not to open another issue, are you avoiding type hints on purpose, or by habits ?

njsmith · 2017-06-09T05:03:08Z

Happily, that will force me to try to review a bit more code :-)

Well, if you put it that way... invitation sent ;-)

Side question not to open another issue, are you avoiding type hints on purpose, or by habits ?

I just haven't had a chance to look at them seriously yet and form an opinion. I'm certainly open to providing them if it would be useful.

A minor issue is that sphinxcontrib-trio might need to be taught to handle them in a way that keeps the docs looking good (python-trio/sphinxcontrib-trio#4), but I'm sure that's solvable and probably needs to happen anyway.

Turn warnigns into errors in the test suite

78500a8

Carreau force-pushed the warnings-into-errors branch from bfd7f95 to 78500a8 Compare June 7, 2017 03:40

njsmith mentioned this pull request Jun 7, 2017

Hide intentional "coroutine '...' was never awaited" in test suite #189

Merged

njsmith merged commit c63a8b7 into python-trio:master Jun 7, 2017

Carreau deleted the warnings-into-errors branch June 7, 2017 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn warnigns into errors in the test suite #187

Turn warnigns into errors in the test suite #187

Carreau commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

codecov bot commented Jun 7, 2017 •

edited

Loading

Carreau commented Jun 7, 2017 •

edited

Loading

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 8, 2017

njsmith commented Jun 8, 2017

njsmith commented Jun 8, 2017

Carreau commented Jun 9, 2017

njsmith commented Jun 9, 2017

Turn warnigns into errors in the test suite #187

Turn warnigns into errors in the test suite #187

Conversation

Carreau commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

codecov bot commented Jun 7, 2017 • edited Loading

Codecov Report

Carreau commented Jun 7, 2017 • edited Loading

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 7, 2017

Carreau commented Jun 7, 2017

njsmith commented Jun 8, 2017

njsmith commented Jun 8, 2017

njsmith commented Jun 8, 2017

Carreau commented Jun 9, 2017

njsmith commented Jun 9, 2017

codecov bot commented Jun 7, 2017 •

edited

Loading

Carreau commented Jun 7, 2017 •

edited

Loading