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

Emit warning for broken object #5404

Merged
merged 1 commit into from Jun 15, 2019

Conversation

Projects
None yet
4 participants
@Zac-HD
Copy link
Member

commented Jun 5, 2019

Closes #5080.

@RonnyPfannschmidt - I'm unconvinced that we should do anything more than close the issue, but if you want a fix I think this is it.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

this needs a test, and its absolutely necessary for people understand what the heck is wrong in certain cases

anything else is hiding issues/errors under a incomprehensible mumble

@Zac-HD Zac-HD force-pushed the Zac-HD:helpful-mock-unwrapper branch from eb06200 to b619a2d Jun 5, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 5, 2019

Codecov Report

Merging #5404 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5404      +/-   ##
==========================================
- Coverage   95.98%   95.97%   -0.02%     
==========================================
  Files         114      114              
  Lines       25502    25523      +21     
  Branches     2479     2480       +1     
==========================================
+ Hits        24479    24495      +16     
- Misses        717      723       +6     
+ Partials      306      305       -1
Impacted Files Coverage Δ
testing/test_doctest.py 100% <100%> (ø) ⬆️
src/_pytest/doctest.py 93.68% <100%> (-1.76%) ⬇️

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 40c5a9d...c5a549b. Read the comment docs.

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@RonnyPfannschmidt
I agree that it needs a test, but only half of it appears to have been covered before already...

@Zac-HD Zac-HD force-pushed the Zac-HD:helpful-mock-unwrapper branch 5 times, most recently from 223c7e7 to 3ea7181 Jun 7, 2019

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

@blueyed - I've got a test, and it's definitely covering that branch locally, but for some reason codecov hasn't seen it. Any idea what's going on?

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

@Zac-HD
Check the build logs, which call coverage report -m and see if it is missing there already.

The partial coverage of the decorator looks weird also: https://codecov.io/gh/pytest-dev/pytest/compare/4f597f011d3d45dc33230f6c3b0cf36f88976030...3ea718182938bb8da0d90bedb13e283890ea4340/diff#D1-1236

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

Hmm. https://travis-ci.org/pytest-dev/pytest/jobs/543079937#L5068 and https://travis-ci.org/pytest-dev/pytest/jobs/543079940#L417 don't list the line as missed (though they do have a missed 377-exit branch), so I suspect a problem where codecov is combining them. I can try if/return;return instead of if/return;else/return...

@Zac-HD Zac-HD force-pushed the Zac-HD:helpful-mock-unwrapper branch 2 times, most recently from df289fb to 012551e Jun 9, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

@Zac-HD
The missing coverage here is due to the parametrized functions not being called, isn't it?
Pushed a fixup.

@Zac-HD
Copy link
Member Author

left a comment

Thanks @blueyed! I'm happy to just squash-merge instead of combining commits locally, if you think it's ready?

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

squash-merging is disabled here.. :/ (#4361)
I could have done it already, but was not sure if force-pushing to your branch is ok for you.

@Zac-HD Zac-HD force-pushed the Zac-HD:helpful-mock-unwrapper branch from cc5f8ce to c5a549b Jun 9, 2019

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

I appreciate the thought - "please don't without warning" probably covers my view on force-pushes. Anyway, I've squashed and force-pushed it now so once CI is done you should be able to merge it 😄

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Thanks @Zac-HD!

I noticed we don't need the real_unwrap = getattr(inspect, "unwrap", None) line in _patch_unwrap_mock_aware as it was introduced in Python 3.4, but it is better to clean that up after this has been backported. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@blueyed want to take another look or can we merge this? 😁

@blueyed blueyed merged commit cf27af7 into pytest-dev:master Jun 15, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch 100% of diff hit (target 95.98%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +4.01% compared to 40c5a9d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20190609.11 succeeded
Details
@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Thanks! :)

Zac-HD added a commit to Zac-HD/pytest that referenced this pull request Jun 16, 2019

Zac-HD added a commit to Zac-HD/pytest that referenced this pull request Jun 17, 2019

Zac-HD added a commit to Zac-HD/pytest that referenced this pull request Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.