Skip to content

Fix TypeError in report_collect with _collect_report_last_write #4330

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
Merged
merged 1 commit into from
Nov 8, 2018
Merged

Fix TypeError in report_collect with _collect_report_last_write #4330

merged 1 commit into from
Nov 8, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 7, 2018

_collect_report_last_write might be None, when pytest_collection was
not called before. Not sure if this indicates another problem, but it
can be reproduced with testing/test_collection.py::TestCollector::()::test_getcustomfile_roundtrip.

Fixes #4329

@blueyed blueyed added type: bug problem that needs to be addressed topic: reporting related to terminal output and user-facing messages and errors labels Nov 7, 2018
@nicoddemus
Copy link
Member

nicoddemus commented Nov 7, 2018 •
edited
Loading

Thanks @blueyed, you are on 🔥

but it
can be reproduced with testing/test_collection.py::TestCollector::()::test_getcustomfile_roundtrip

What do you mean? This test passes for me, and the output looks OK as well.

Oh just saw the issue, and I can replicate using the command posted there.

But I worry that this was passing before, because we might introduce a regression by mistake in the future.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 7, 2018

But I worry that this was passing before, because we might introduce a regression by mistake in the future.

When doing this initially I assumed that both hooks were called in sequence always, which apparently is not the case here.
This fix is just was I would have done without that assumption, so I think it is good - but the issue itself of both hooks not being called always might be worth investigating..

@nicoddemus
Copy link
Member

This fix is just was I would have done without that assumption, so I think it is good

Fair enough. 👍

blueyed added a commit to blueyed/pytest that referenced this pull request Nov 7, 2018
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [ci skip]
9e0d08b 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
@blueyed blueyed mentioned this pull request Nov 7, 2018
Closed
@blueyed
Copy link
Contributor Author

blueyed commented Nov 7, 2018

#4331 contains a test for this, but fails due to the capture plugin.

@blueyed
Fix TypeError in report_collect with _collect_report_last_write
91404db 
`_collect_report_last_write` might be None, when `pytest_collection` was
not called before.  Not sure if this indicates another problem, but it
can be reproduced with `testing/test_collection.py::TestCollector::()::test_getcustomfile_roundtrip`.

Fixes pytest-dev#4329
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 7, 2018
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [ci skip]
5fd5e07 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
@blueyed
Copy link
Contributor Author

blueyed commented Nov 7, 2018

Included in #4331.

@blueyed blueyed closed this Nov 7, 2018
@blueyed blueyed deleted the fix-report-last-write branch November 7, 2018 22:42
@blueyed blueyed restored the fix-report-last-write branch November 7, 2018 22:48
@blueyed blueyed deleted the fix-report-last-write branch November 7, 2018 22:49
@blueyed blueyed restored the fix-report-last-write branch November 8, 2018 20:02
@blueyed blueyed reopened this Nov 8, 2018
@blueyed blueyed force-pushed the fix-report-last-write branch from 5470533 to 91404db Compare November 8, 2018 20:05
@codecov
Copy link

codecov bot commented Nov 8, 2018

Codecov Report

Merging #4330 into master will decrease coverage by 2.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4330      +/-   ##
==========================================
- Coverage   95.84%   93.72%   -2.13%     
==========================================
  Files         111      111              
  Lines       24880    24875       -5     
  Branches     2430     2427       -3     
==========================================
- Hits        23847    23314     -533     
- Misses        736     1230     +494     
- Partials      297      331      +34
Flag Coverage Δ
#docs ?
#doctesting ?
#linting ?
#linux 93.72% <100%> (-1.92%) ⬇️
#nobyte ?
#numpy ?
#pexpect ?
#py27 91.64% <100%> (-2.38%) ⬇️
#py34 92.02% <100%> (-0.07%) ⬇️
#py35 ?
#py36 92% <100%> (-1.9%) ⬇️
#py37 ?
#trial ?
#windows ?
#xdist ?
Impacted Files Coverage Δ
src/_pytest/terminal.py 91.32% <100%> (-1.71%) ⬇️
testing/test_pdb.py 42.4% <0%> (-52.95%) ⬇️
src/_pytest/debugging.py 51.38% <0%> (-32.64%) ⬇️
testing/python/approx.py 79.84% <0%> (-19.77%) ⬇️
src/_pytest/unittest.py 75.14% <0%> (-19.21%) ⬇️
testing/test_unittest.py 85.32% <0%> (-13.77%) ⬇️
src/_pytest/reports.py 86.9% <0%> (-10.72%) ⬇️
src/_pytest/python_api.py 87.12% <0%> (-10.31%) ⬇️
testing/test_pathlib.py 91.17% <0%> (-8.83%) ⬇️
src/_pytest/assertion/util.py 92.92% <0%> (-5.19%) ⬇️
... and 22 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 64762d2...5470533. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 8, 2018

Going to merge this already, since #4331 (which implicitly tests this) might take longer.

@blueyed blueyed merged commit f06fe43 into pytest-dev:master Nov 8, 2018
@blueyed blueyed deleted the fix-report-last-write branch November 8, 2018 20:06
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 8, 2018
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [ci skip]
255d553 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
blueyed added a commit to blueyed/pytest that referenced this pull request Nov 14, 2018
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [skip appveyor]
ce0ed9c 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
blueyed added a commit to blueyed/pytest that referenced this pull request Feb 8, 2019
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [skip appveyor]
11478c5 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
blueyed added a commit to blueyed/pytest that referenced this pull request Jun 7, 2019
@blueyed
TODO: add failing test_capture_with_testdir_parseconfig [skip appveyor]
27ce36c 
Fails with:

> OSError: reading from stdin while output is captured

Also tests pytest-dev#4330 (i.e. fails
without pytest-dev#4330 earlier already).
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

Assignees
No one assigned
Labels
topic: reporting related to terminal output and user-facing messages and errors type: bug problem that needs to be addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blueyed @nicoddemus