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

Display message from reprcrash in short test summary #5013

Merged
merged 14 commits into from May 8, 2019

Conversation

Projects
None yet
5 participants
@blueyed
Copy link
Contributor

commented Mar 29, 2019

This is useful to see common patterns easily, but also for single
failures already.

There might be a better API to get to the message?!

See #5002 (comment) for some discussion/ideas to change the format in general / make it configurable.

@codecov

This comment has been minimized.

Copy link

commented Mar 29, 2019

Codecov Report

Merging #5013 into features will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5013      +/-   ##
============================================
+ Coverage     96.11%   96.16%   +0.04%     
============================================
  Files           115      115              
  Lines         25924    26285     +361     
  Branches       2562     2591      +29     
============================================
+ Hits          24917    25276     +359     
- Misses          704      708       +4     
+ Partials        303      301       -2
Impacted Files Coverage Δ
testing/acceptance_test.py 98.04% <ø> (+0.01%) ⬆️
src/_pytest/skipping.py 96.15% <ø> (+0.03%) ⬆️
testing/test_skipping.py 97.47% <ø> (ø) ⬆️
src/_pytest/terminal.py 93.42% <100%> (+0.31%) ⬆️
testing/test_terminal.py 99.84% <100%> (+0.01%) ⬆️
src/_pytest/debugging.py 89.18% <0%> (-0.23%) ⬇️
testing/python/fixtures.py 99% <0%> (-0.08%) ⬇️
src/_pytest/capture.py 95.09% <0%> (-0.05%) ⬇️
testing/code/test_excinfo.py 96.76% <0%> (-0.03%) ⬇️
src/_pytest/_code/code.py 95.5% <0%> (-0.02%) ⬇️
... 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 2b11b2c...f339147. Read the comment docs.

@blueyed blueyed requested review from wimglenn and asottile Apr 3, 2019

@asottile

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

What does this look like for longer messages? The example in the test looks fine for a trivial assert 0 but I suspect for something like raise TypeError('very long message blah blah blah') this might get unwieldy

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

this might get unwieldy

Indeed, we use very long messages (even multi-line ones) at work. How about truncating up until the terminal width?

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

It uses the first line currently already only.
Truncating up until the terminal width sounds good (reminded me of the not so good working truncation of ini-options in pytest --help, which appears to use a fixed width of 80 (likely an argparse issue)).

Display message from reprcrash in short test summary
This is useful to see common patterns easily, but also for single
failures already.

@blueyed blueyed force-pushed the blueyed:short-summary-message branch from 3c5815b to 3d0ecd0 Apr 4, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Updated. Good for adding a changelog?

@nicoddemus
Copy link
Member

left a comment

Almost there I think, feel free to add a CHANGELOG too.

Show resolved Hide resolved src/_pytest/skipping.py Outdated
Show resolved Hide resolved src/_pytest/skipping.py Outdated
@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Noticed that having ":" at the end of the nodeid is bad for copy'n'pasing them..
Change to "VERB node::id - msg" maybe?

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Change to "VERB node::id - msg" maybe?

FAILED testing/test_cacheprovider.py::TestNewAPI::test_config_cache_makedir: assert 0

On my terminal (Cmder on Windows), double clicking on the test line only selects the node id, the : at the end is left out. For you double clicking also includes the :?

Anyway, I'm fine with using - as separator. 👍

@blueyed blueyed force-pushed the blueyed:short-summary-message branch from 85b848b to 1597044 Apr 5, 2019

@blueyed blueyed changed the title [RFC] Display message from reprcrash in short test summary Display message from reprcrash in short test summary Apr 5, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Offtopic:

reminded me of the not so good working truncation of ini-options in pytest --help, which appears to use a fixed width of 80

This is due to me using pytest --help | less, where stdout then is redirected, and shutil.get_terminal_size() fails (python/cpython#12697).

edit: see also #3836

if max_len_msg >= len_ellipsis:
if len_msg > max_len_msg:
msg = msg[: (max_len_msg - len_ellipsis)] + ellipsis
line += sep + msg

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 5, 2019

Member

Oh just realized this: is msg unicode-safe in Python 2? because we will be joining a str/bytes with a potential unicode message, so there's a good chance this might blow up

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 5, 2019

Author Contributor

Would appreciate a failing example.. do you mean we should prefix sep etc also with u?

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 5, 2019

Member

Hmm actually no, this is safe: if msg is unicode, Python 2 will try to elevate sep to unicode using ascii (which is fine), not the other way around. Sorry for the brain fart. 👍

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 5, 2019

Member

If you want to play it safe and ensure we don't regress though, you can change the message to use a smiling face emoji instead (😄), but is up to you. 👍

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 5, 2019

Author Contributor

Added.. but it shows that we're not handling terminal cells there really - i.e. it would wrap due to this.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 5, 2019

Member

Oh. Hmm not even sure what the solution would be, TBH.

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 5, 2019

Author Contributor

added a commit using wcwidth.. not sure if it is worth the extra dependency, but it worked well.. ;)

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 10, 2019

Member

I think adding wcwidth is fine: small, license is OK, seems to be maintained

blueyed added some commits Apr 5, 2019

max_len_msg -= len_ellipsis
msg = msg[:max_len_msg]
while wcswidth(msg) > max_len_msg:
msg = msg[:-1]

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 5, 2019

Author Contributor

This could be optimized probably if we're taking it - there is also wcwidth.

@blueyed blueyed requested a review from nicoddemus Apr 6, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

I think in the long run the terminal writer should become aware of cells instead of chars.

@nicoddemus
Copy link
Member

left a comment

Looks good then, could you please fix the Python 2 tests and add another changelog entry mentioning the new dependency?

I suggest 5013.trivial.rst:

pytest now depends on `wcwidth <https://pypi.org/project/wcwidth>`__ to properly track unicode character sizes for more precise terminal output.
if max_len_msg >= len_ellipsis:
if len_msg > max_len_msg:
msg = msg[: (max_len_msg - len_ellipsis)] + ellipsis
line += sep + msg

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 10, 2019

Member

I think adding wcwidth is fine: small, license is OK, seems to be maintained

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hmm, py2 only fails on Windows and MacOS..

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Hmm, py2 only fails on Windows and MacOS..

Oh right, I saw Travis and thought it was Linux, didn't realize it was the MacOS job.

I can take a look at the Windows failure tomorrow if you like

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@nicoddemus
Yes, please take a look.

Show resolved Hide resolved src/_pytest/skipping.py Outdated

blueyed added some commits Apr 17, 2019

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@asottile and @wimglenn, any other comments or can we merge this?

Show resolved Hide resolved src/_pytest/terminal.py Outdated
@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

changelog entry mentioning the new dependency?

This appears to be missing still.

max_len_msg -= len_ellipsis
msg = msg[:max_len_msg]
while wcswidth(msg) > max_len_msg:
msg = msg[:-1]

This comment has been minimized.

Copy link
@blueyed

blueyed May 7, 2019

Author Contributor

Said this earlier already, but this could certainly be done better, e.g. by using wcwidth from the end/tail (substracting it), but well.. I think when we use this in other places it can be improved then still.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 7, 2019

This appears to be missing still.

Ahh thanks for remembering this.

I've pushed my suggestion, feel free to edit it as you see fit. 👍

@@ -0,0 +1 @@
pytest now depends on `wcwidth <https://pypi.org/project/wcwidth>`__ to properly track unicode character sizes for more precise terminal output.

This comment has been minimized.

Copy link
@blueyed

blueyed May 7, 2019

Author Contributor

Thanks! ❤️

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Anything else here @blueyed? @asottile ?

@blueyed blueyed merged commit 5eeb5ee into pytest-dev:features May 8, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch 100% of diff hit (target 96.11%)
Details
codecov/project 96.16% (+0.04%) compared to 2b11b2c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20190507.11 succeeded
Details

@blueyed blueyed deleted the blueyed:short-summary-message branch May 8, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Let's 🐑 🇮🇹.

@The-Compiler

This comment has been minimized.

Copy link
Member

commented May 15, 2019

ERROR tests/end2end/features/test_open_bdd.py::test_opening_a_quickmark - ass...

well, thanks pytest, I guess? 😆

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

At least we've not used ", " as delimiter here.. ;)

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.