Repository navigation
coverage flapping in acontextmanager #140
Description
Kind of annoying, e.g. this is a doc-only PR: https://codecov.io/gh/python-trio/trio/pull/139/changes
The solution is probably to write proper tests for acontextmanager.
Kind of annoying, e.g. this is a doc-only PR: https://codecov.io/gh/python-trio/trio/pull/139/changes
The solution is probably to write proper tests for acontextmanager.
Activity
imrn commentedon Apr 24, 2017
Are you going to maintain acontextmanager and friends in your
project or are there any plans to have them in python stdlib?
njsmith commentedon Apr 24, 2017
In the long run I expect there'll be something like this in the stdlib, but not before 3.7, and it'll be a while before we can drop support for 3.5 and 3.6. What might make sense until then is to move
acontextmanagerinto some other library, like maybe myasync_generator. But another consideration is that trio might actually need to keep this so that it can have full control over the keyboard interrupt handling (see my blog post about control-C).In any case I should probably bite the bullet and write some tests :-)
njsmith commentedon Jun 12, 2017
@kyrias: oh, if you're looking for a very straightforward place to get your feet wet, this is also an option :-). Basically the issue is just that we need some tests written for
trio.util.acontextmanagerto systematically check all the different paths in__aexit__, both because this is a good idea in general and because it's causing annoying issues where codecov keeps complaining about random lines here becoming covered/uncovered. (Apparently there's something in the test suite that randomly covers a few lines or not, so codecov keeps thing random unrelated changes are causing coverage regressions.)(And note for anyone else stumbling across this later, if it's no longer mid-June 2017 and the issue is still open then you can probably assume that @kyrias didn't take this up and you're welcome to it!)
njsmith commentedon Aug 8, 2017
Actually, I'm baffled about this. Here it is happening again now: https://codecov.io/gh/python-trio/trio/pull/269/changes
But that code clearly has a test that should cover it in our current testsuite:
trio/trio/tests/test_util.py
Lines 95 to 97 in 8229f06
Stop acontextmanager coverage from flapping all the time
njsmith commentedon Aug 8, 2017
(whoops, fat-fingered the close button I think)
Oh, I just had a silly misunderstanding -- to cover those lines we need a test where the context manager allows a
StopAsyncIterationto pass through, which we didn't have -- the test I pointed at raisesStopAsyncIterationbut then the context manager catches it. #270 has a test like this.njsmith commentedon Aug 18, 2017
This just came back: https://codecov.io/gh/python-trio/trio/pull/292/changes
WTF
njsmith commentedon Aug 18, 2017
Yeah, I've reviewed the code and we definitely have an unconditional test that hits those two lines, ran just that test locally and coverage said it hit those two lines, and we're running the test suite like 10 times or something and merging the coverage, so it definitely should be hit. And yet.
Add another test that should fix python-triogh-140
Merge pull request #293 from njsmith/issue-140-again
cjerdonek commentedon Aug 19, 2017
Have you considered filing a bug with codecov?
njsmith commentedon Aug 20, 2017
I have! But I'm not sure what I would say. It's coverage.py that generates the actual coverage info, and if it's generating something weird then they can't help much with that. And if I don't have any better idea what's going on, it's hard to file a bug that doesn't just sound like "please debug my test suite for me". I did track down the raw uploads that codecov received and they do in fact say that in
trio/_util.py, line 124 has a missing branch and line 128 is uncovered. That doesn't rule out there being some weird bug in their uploader tool or something, but... I dunno. The other thing I thought of is that it might somehow have been reporting coverage for some other older revision, but I checked and the reports say thattrio/__init__.pyhad 84 lines, which only became true in the commit right before #292, so it does seem to be reporting on the right thing.I was hoping #297 would help shed some light, and while it did uncover a weird bug in how github/travis/codecov interact, I don't think it can explain this issue.
Maybe it will... go away and leave us alone now? 🤞
njsmith commentedon Aug 20, 2017
(Also, note for future reference: the secret to getting the raw codecov logs from a PR report is: you have to first go to the "commits" tab, pick a specific commit, and then you can go to the "build" tab.)
cjerdonek commentedon Aug 20, 2017
Out of curiosity, what important "value adds" does codecov.io provide that "manually" running coverage in CI wouldn't provide? If you do it manually, at least that way there will be less indirection (e.g. if codecov.io introduces weirdness that you have no control over / visibility into), and so you'd have a greater ability to reproduce and diagnose.
njsmith commentedon Aug 20, 2017
Basically, aggregating across all the ci builds, flagging issues directly in the pr (who looks at build logs when the build is green?), and providing a ui to examine what happened after something got flagged.
The aggregation is a big deal, because otherwise it's really hard to spot actually uncovered code hidden among the bits of code that only run on Windows, or pypy, or py36, or...