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.

Sign up for GitHub

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

Jump to bottom

Assertion rewrite breaks for objects that reimplement __getattr__ #4632

Merged
merged 3 commits into from
Jan 11, 2019
Merged

Assertion rewrite breaks for objects that reimplement __getattr__ #4632

merged 3 commits into from
Jan 11, 2019

Conversation

AnjoMan
Copy link
Contributor

@AnjoMan AnjoMan commented Jan 10, 2019 •
edited
Loading

This fix addresses #4631.

@AnjoMan AnjoMan force-pushed the dont-rewrite-objects-with-failing-getattr branch from 60d0636 to 266c0ae Compare January 10, 2019 19:31
@AnjoMan AnjoMan mentioned this pull request Jan 10, 2019
Closed
@AnjoMan AnjoMan changed the title Dont rewrite objects with failing getattr Assertion rewrite breaks for objects that reimplement __getattr__ Jan 10, 2019
nicoddemus
nicoddemus requested changes Jan 10, 2019
View reviewed changes
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR, LGTM!

Can you please update the changelog entry then rebase and target master instead? This is a bugfix so it should go in the next patch version 4.1.1. 👍

nicoddemus
nicoddemus reviewed Jan 10, 2019
View reviewed changes
4631.bugfix.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #4632 into features will decrease coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4632      +/-   ##
============================================
- Coverage     95.76%   95.74%   -0.02%     
============================================
  Files           111      111              
  Lines         24683    24702      +19     
  Branches       2446     2447       +1     
============================================
+ Hits          23637    23652      +15     
- Misses          739      743       +4     
  Partials        307      307
Flag Coverage Δ
#docs 29.54% <5%> (+0.05%) ⬆️
#doctesting 29.54% <5%> (+0.05%) ⬆️
#linting 29.54% <5%> (+0.05%) ⬆️
#linux 95.56% <80%> (-0.02%) ⬇️
#nobyte 92.37% <80%> (ø) ⬆️
#numpy 93.19% <80%> (ø) ⬆️
#pexpect 42.09% <10%> (-0.04%) ⬇️
#py27 93.78% <80%> (-0.02%) ⬇️
#py34 91.85% <80%> (+0.05%) ⬆️
#py35 91.87% <80%> (+0.05%) ⬆️
#py36 91.9% <80%> (+0.05%) ⬆️
#py37 93.92% <80%> (ø) ⬆️
#trial 93.19% <80%> (ø) ⬆️
#windows 93.93% <80%> (-0.01%) ⬇️
#xdist 93.76% <80%> (-0.04%) ⬇️
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.32% <66.66%> (-0.29%) ⬇️
testing/test_assertrewrite.py 83.5% <85.71%> (+0.04%) ⬆️

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 5dcb370...266c0ae. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 10, 2019

Codecov Report

Merging #4632 into features will decrease coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4632      +/-   ##
============================================
- Coverage     95.76%   95.74%   -0.02%     
============================================
  Files           111      111              
  Lines         24683    24702      +19     
  Branches       2446     2447       +1     
============================================
+ Hits          23637    23652      +15     
- Misses          739      743       +4     
  Partials        307      307
Flag Coverage Δ
#docs 29.54% <5%> (+0.05%) ⬆️
#doctesting 29.54% <5%> (+0.05%) ⬆️
#linting 29.54% <5%> (+0.05%) ⬆️
#linux 95.56% <80%> (-0.02%) ⬇️
#nobyte 92.37% <80%> (ø) ⬆️
#numpy 93.19% <80%> (ø) ⬆️
#pexpect 42.09% <10%> (-0.04%) ⬇️
#py27 93.78% <80%> (-0.02%) ⬇️
#py34 91.85% <80%> (+0.05%) ⬆️
#py35 91.87% <80%> (+0.05%) ⬆️
#py36 91.9% <80%> (+0.05%) ⬆️
#py37 93.92% <80%> (ø) ⬆️
#trial 93.19% <80%> (ø) ⬆️
#windows 93.93% <80%> (-0.01%) ⬇️
#xdist 93.76% <80%> (-0.04%) ⬇️
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.32% <66.66%> (-0.29%) ⬇️
testing/test_assertrewrite.py 83.5% <85.71%> (+0.04%) ⬆️

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 5dcb370...266c0ae. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 10, 2019 •
edited
Loading

Codecov Report

Merging #4632 into features will decrease coverage by 0.19%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           features    #4632     +/-   ##
===========================================
- Coverage     95.76%   95.56%   -0.2%     
===========================================
  Files           111      111             
  Lines         24683    24702     +19     
  Branches       2446     2447      +1     
===========================================
- Hits          23637    23607     -30     
- Misses          739      774     +35     
- Partials        307      321     +14
Flag Coverage Δ
#docs 29.46% <5%> (-0.03%) ⬇️
#doctesting 29.46% <5%> (-0.03%) ⬇️
#linting 29.46% <5%> (-0.03%) ⬇️
#linux 95.56% <80%> (-0.02%) ⬇️
#nobyte 91.73% <80%> (-0.65%) ⬇️
#numpy 42.09% <10%> (-51.1%) ⬇️
#pexpect 42.09% <10%> (-0.04%) ⬇️
#py27 93.64% <80%> (-0.15%) ⬇️
#py34 91.73% <80%> (-0.08%) ⬇️
#py35 91.75% <80%> (-0.08%) ⬇️
#py36 91.72% <80%> (-0.13%) ⬇️
#py37 93.75% <80%> (-0.17%) ⬇️
#trial 42.09% <10%> (-51.1%) ⬇️
#windows ?
#xdist 93.61% <80%> (-0.19%) ⬇️
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.32% <66.66%> (-0.29%) ⬇️
testing/test_assertrewrite.py 83.5% <85.71%> (+0.04%) ⬆️
testing/test_pathlib.py 91.17% <0%> (-8.83%) ⬇️
testing/test_tmpdir.py 94.88% <0%> (-3.98%) ⬇️
src/_pytest/capture.py 90.95% <0%> (-3.17%) ⬇️
src/_pytest/pathlib.py 88.46% <0%> (-2.2%) ⬇️
src/_pytest/cacheprovider.py 95.75% <0%> (-1.42%) ⬇️
testing/acceptance_test.py 97.19% <0%> (-1.08%) ⬇️
src/_pytest/nodes.py 94.52% <0%> (-1%) ⬇️
src/_pytest/pytester.py 87.01% <0%> (-0.43%) ⬇️
... and 2 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 5dcb370...6c89c89. Read the comment docs.

@AnjoMan
Test rewriting assertion when __name__ fails
acb3e8e 
Pytest rewrites assertions so that the items on each
side of a comoparison will have easier-to-read names
in case of an assertion error.

Before doing this, it checks to make sure the object
doesn't have a __name__ attribute; however, it uses
`hasattr` so if the objects __getattr__ is broken then
the test failure message will be the stack trace
for this failure instead of a rewritten assertion.
@AnjoMan
Don't fail if hasattr fails when rewriting assertions
3241fc3 
When rewriting assertions, pytest makes a call to
`__name__` on each object in a comparision. If one of
the objects has reimplemented `__getattr__`, they could
fail trying to fetch `__name__` with an error other than
`AttributeError`, which is what `hasattr` catches.

In this case, the stack trace for the failed `__getattr__`
call will show up in the pytest output, even though
it isn't related to the test failing.

This change fixes that by catching exceptions
that `hasattr` throws.
@AnjoMan AnjoMan force-pushed the dont-rewrite-objects-with-failing-getattr branch from 7535b4c to 6c89c89 Compare January 11, 2019 01:45
@AnjoMan AnjoMan changed the base branch from features to master January 11, 2019 01:46
@AnjoMan AnjoMan force-pushed the dont-rewrite-objects-with-failing-getattr branch from 6c89c89 to 77da4f1 Compare January 11, 2019 01:49
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Jan 11, 2019

@nicoddemus I think this is ready to go now.

@nicoddemus
Copy link
Member

Yes, thanks again!

@nicoddemus nicoddemus merged commit 3efb26a into pytest-dev:master Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asottile asottile asottile approved these changes

@nicoddemus nicoddemus nicoddemus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AnjoMan @nicoddemus @asottile