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

Improve internal error messages #4077

Merged
merged 2 commits into from
Oct 12, 2018

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Oct 3, 2018

Currently some usage errors produce very ugly traceback with pytest's internals. For example, mistyping the scope of a fixture produces this:

============================================= ERRORS ==============================================
_____________________________ ERROR collecting test-scope-mismatch.py _____________________________
src\_pytest\fixtures.py:699: in scope2index
    return scopes.index(scope)
E   ValueError: 'modu' is not in list

During handling of the above exception, another exception occurred:
src\_pytest\runner.py:201: in __init__
    self.result = func()
src\_pytest\runner.py:265: in <lambda>
    call = CallInfo(lambda: list(collector.collect()), "collect")
src\_pytest\python.py:475: in collect
    self.session._fixturemanager.parsefactories(self)
src\_pytest\fixtures.py:1321: in parsefactories
    ids=marker.ids,
src\_pytest\fixtures.py:837: in __init__
    scope or "function", descr="fixture {}".format(func.__name__), where=baseid
src\_pytest\fixtures.py:703: in scope2index
    descr, "from {} ".format(where) if where else "", scope
E   ValueError: fixture fix from test-scope-mismatch.py has an unsupported scope value 'modu'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
===================================== 1 error in 0.16 seconds =====================================

This PR handles a specific of exception type (here UsageError for demonstration) so it shows only the exception message instead of the entire traceback. This way we can raise that exception on errors that we know are usage problems and where displaying the entire traceback is not helpful in general. With this PR the error above produces this:

============================================= ERRORS ==============================================
_____________________________ ERROR collecting test-scope-mismatch.py _____________________________
fixture fix from test-scope-mismatch.py has an unsupported scope value 'modu'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
===================================== 1 error in 0.07 seconds =====================================

Which is definitely nicer.

Questions for discussion:

  1. Is this approach sound?
  2. Should we use UsageError or create a new type of exception? Should it be public so plugins can use it too?
  3. Comments about verbosity handling done here?

Refs: #2293, #3867, #2535, #3332


EDIT: in the end we don't need a new exception, we just have to use pytest.fail(..., pytrace=False) instead of general exceptions and make sure we also treat Failed exceptions during collection.

Fixes #2293, Fixes #3867

I will resolve #3332 in a separate PR.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

makes sense to me! We use UserError in an internal tool that follows this pattern and FatalError in pre-commit for the same idea

@nicoddemus
Copy link
Member Author

nicoddemus commented Oct 9, 2018

It turns out that we almost have this already: pytest.fail accepts pytrace=False as an argument, which suppresses the traceback.

I have this working locally, need to review the code for candidates to use pytest.fail instead of raising exceptions like they are doing right now and fix tests.

This will go into features though, the affected functions will raise a different exception now (for example parametrize raises ValueError in some places, will be changed to raise pytest.fail.Exception instead).

@codecov
Copy link

codecov bot commented Oct 10, 2018

Codecov Report

Merging #4077 into features will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4077      +/-   ##
============================================
+ Coverage     94.37%   94.57%   +0.19%     
============================================
  Files           109      109              
  Lines         23942    23945       +3     
  Branches       2363     2361       -2     
============================================
+ Hits          22596    22645      +49     
+ Misses         1028      994      -34     
+ Partials        318      306      -12
Flag Coverage Δ
#doctesting 29.16% <13.33%> (+0.81%) ⬆️
#linux 94.42% <100%> (+0.04%) ⬆️
#nobyte 0% <0%> (ø) ⬆️
#numpy 28.07% <16.66%> (+0.12%) ⬆️
#pexpect 0% <0%> (ø) ⬆️
#py27 92.68% <100%> (+0.15%) ⬆️
#py34 92.12% <100%> (+0.15%) ⬆️
#py35 92.13% <100%> (+0.15%) ⬆️
#py36 92.69% <100%> (+0.2%) ⬆️
#py37 92.33% <100%> (+0.2%) ⬆️
#trial 31.2% <30%> (+0.17%) ⬆️
#windows 93.88% <100%> (?)
#xdist 18.68% <10%> (+0.07%) ⬆️
Impacted Files Coverage Δ
testing/python/fixture.py 99.23% <ø> (ø) ⬆️
src/_pytest/mark/__init__.py 97.26% <ø> (-0.04%) ⬇️
testing/test_mark.py 97.15% <100%> (ø) ⬆️
src/_pytest/fixtures.py 97.3% <100%> (+0.26%) ⬆️
testing/test_runner.py 96.81% <100%> (+0.03%) ⬆️
src/_pytest/nodes.py 94.71% <100%> (+0.91%) ⬆️
src/_pytest/python.py 95.67% <100%> (-0.02%) ⬇️
testing/python/metafunc.py 95.28% <100%> (-0.03%) ⬇️
src/_pytest/mark/structures.py 94.57% <100%> (+0.47%) ⬆️
... and 10 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 602e74c...5436e42. Read the comment docs.

@nicoddemus nicoddemus changed the base branch from master to features October 10, 2018 22:19
@nicoddemus nicoddemus changed the title [WIP] Improve internal error messages Improve internal error messages Oct 10, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.81% when pulling 5436e42 on nicoddemus:short-usage-errors into 602e74c on pytest-dev:features.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.81% when pulling 5436e42 on nicoddemus:short-usage-errors into 602e74c on pytest-dev:features.

@nicoddemus nicoddemus merged commit e8348a1 into pytest-dev:features Oct 12, 2018
@nicoddemus nicoddemus deleted the short-usage-errors branch October 12, 2018 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants