Skip to content

Assertion rewrite breaks for objects that reimplement __getattr__#4632

Merged
nicoddemus merged 3 commits into
pytest-dev:masterfrom
AnjoMan:dont-rewrite-objects-with-failing-getattr
Jan 11, 2019
Merged

Assertion rewrite breaks for objects that reimplement __getattr__#4632
nicoddemus merged 3 commits into
pytest-dev:masterfrom
AnjoMan:dont-rewrite-objects-with-failing-getattr

Conversation

@AnjoMan

@AnjoMan AnjoMan commented Jan 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed Jan 10, 2019
View reviewed changes
Comment thread testing/test_assertrewrite.py
nicoddemus
nicoddemus requested changes Jan 10, 2019
View reviewed changes

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

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
Comment thread 4631.bugfix.rst Outdated
@codecov

codecov Bot commented Jan 10, 2019

Copy link
Copy Markdown

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

codecov Bot commented Jan 10, 2019

Copy link
Copy Markdown

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

codecov Bot commented Jan 10, 2019 •
edited
Loading

Copy link
Copy Markdown

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

AnjoMan commented Jan 11, 2019

Copy link
Copy Markdown
Contributor Author

@nicoddemus I think this is ready to go now.

@nicoddemus

nicoddemus commented Jan 11, 2019

Copy link
Copy Markdown
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

@nicoddemus nicoddemus nicoddemus approved these changes

@asottile asottile asottile 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