Restructure truncation of assertion messages by mattduck · Pull Request #1995 · pytest-dev/pytest

mattduck · 2016-10-10T22:50:49Z

This addresses ref #1954.

The current truncation logic for assertion explanations does not deal with long lines
properly:

Previously if lines were too long it would display a "-n more lines"
message.
999e7c6 introduced a bug where long lines can
cause index errors if there are < 10 lines.

Extract the truncation logic into its own file and ensure it can deal with
long lines properly.

This addresses ref pytest-dev#1954. The current truncation for assertion explanations does not deal with long lines properly: - Previously if lines were too long it would display a "-n more lines" message. - 999e7c6 introduced a bug where long lines can cause index errors if there are < 10 lines. Extract the truncation logic into its own file and ensure it can deal with long lines properly.

mattduck · 2016-10-10T22:56:32Z

+    return verbose < 2 and not _running_on_ci()
+
+
+def _running_on_ci():


@flub previously noted this:

Not sure it's right to use this _running_on_ci() condition. It's a very hidden configuration option. Either it needs to be configured separetely and then it probably needs a new command line option or config file option or it's fine being linked to -vN.

I haven't looked into this yet, but I agree that this shouldn't be so hidden.

What's the reason for truncating output on CI?

Actually, reverse question... CI is displaying full output. What's the reason that we can't solve this by running on CI using -vv?

This is addressed in the original commit message, it attempts to provide some utility for the user:

Always show full comparison output if on CI. When you don't get enough information with a test running on a CI, it's quite frustrating, for various reasons: - It's more likely to be a flaky test, so you might not be able to reproduce the failure. - Passing -vv is quite bothersome (creating a temporary commit and reverting it) For those reasons, if something goes wrong on CI, it's good to have as much information as possible.

Hey @The-Compiler, what makes passing -vv bothersome? Is it because it makes the output (other than your failing test) too verbose? I'm not sure I understand why you need a temporary commit. Thanks!

That's one reason, the other is that it's just a sane default IMHO (because you can't easily debug failures on CI, and I'd imagine most people don't use -vv on CI by default)

@The-Compiler cool, thanks.

it's just a sane default IMHO (because you can't easily debug failures on CI

This is a good point, but I also agree with @flub that it's quite hidden / implicit behaviour. Do you think it's worth adding --help documentation for this? Or moving it up to config.py and linking it to -vv?

Either way I think this is a separate feature that should be changed (or not) on another branch.

I don't understand what you mean with "Or moving it up to config.py and linking it to -vv?".

I'm not sure where an explanation would fit. To be honest, I don't think it needs more documentation, just like the truncation in the first place (or that you can use -vv to turn it off) isn't explicitly documented.

coveralls · 2016-10-10T22:59:19Z

Coverage increased (+0.02%) to 92.845% when pulling b629da4 on mattduck:feat/restructure-assert-truncation into 4667b4d on pytest-dev:features.

coveralls · 2016-10-10T23:27:10Z

Coverage increased (+0.02%) to 92.844% when pulling 0061d9b on mattduck:feat/restructure-assert-truncation into 4667b4d on pytest-dev:features.

nicoddemus

Thanks @mattduck! Feel free to ignore the error on py35-trial (#1989).

nicoddemus · 2016-10-11T15:48:38Z

+
+def _truncate_by_char_count(input_lines, max_chars):
+    # Check if truncation required
+    if len("".join(input_lines)) <= max_chars:


This check is not really required because you already did the same check in line 56.

I'm pointing this out because this might be expensive if the output is quite big, although in general I'm sure it won't matter.

An option would be to pass the full text to this function.

Hey @nicoddemus, the check in 56 is not the same as the check here, because line 61 truncates input_lines to 8 lines, so the total length might have changed.

Do you think this should be restructured? Or is it worth renaming the variables to make it clear that the input_lines arg that gets passed to _truncate_by_char_count() might not be the same as the input_lines that gets passed to _truncate_explanation()?

nicoddemus · 2016-10-11T21:22:51Z

+        ])
+
+        result = testdir.runpytest('-vv')
+        result.stdout.fnmatch_lines([


The output from this runpytest call is:

def test_many_lines(): a = list([str(i)[0] * 100 for i in range(7)]) b = a[::2] a = '\n'.join(map(str, a)) b = '\n'.join(map(str, b)) > assert a == b E assert '000000000000...6666666666666' == '0000000000000...6666666666666' E 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 E - 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 E 2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222 E - 3333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333 E 4444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444 E - 5555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555 E 6666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666666 test_full_output_truncated.py:6: AssertionError ========================== 1 failed in 0.01 seconds ===========================

Because "*- 5* will match even when truncating the output as the match above this one show, I think this fnmatch_lines call and the next should match against the 6* line to demonstrate the the output is not being truncated, something like:

result.stdout.fnmatch_lines([ "* 6*", ])

Good shout - have fixed this.

Fix issue with truncation tests for -vv and CI, where the test for non-truncated output would return a positive match even on truncated output.

coveralls · 2016-10-16T20:18:53Z

Coverage increased (+0.02%) to 92.844% when pulling acee88a on mattduck:feat/restructure-assert-truncation into 4667b4d on pytest-dev:features.

mattduck · 2016-10-16T21:25:57Z

@nicoddemus thanks for the feedback. I've fixed the test issue and replied on the other point. Let me know if you want to make any other changes.

nicoddemus · 2016-11-07T23:07:21Z

@mattduck thanks for following up.

Anybody else wants to review this?

nicoddemus · 2016-11-13T21:07:42Z

Thanks again @mattduck!

mattduck commented Oct 10, 2016

View reviewed changes

Fix flake8 (unused import, trailng whitespace)

0061d9b

nicoddemus requested changes Oct 11, 2016

View reviewed changes

Fix truncation tests: pyruntest() match

acee88a

Fix issue with truncation tests for -vv and CI, where the test for non-truncated output would return a positive match even on truncated output.

nicoddemus approved these changes Nov 7, 2016

View reviewed changes

nicoddemus merged commit 6876ba9 into pytest-dev:features Nov 13, 2016

nicoddemus mentioned this pull request Sep 29, 2017

Assertion truncation: properly handle long lines #1954

Closed

		return verbose < 2 and not _running_on_ci()


		def _running_on_ci():

Uh oh!

Conversation

mattduck commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 10, 2016

Uh oh!

coveralls commented Oct 10, 2016

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 16, 2016

Uh oh!

mattduck commented Oct 16, 2016

Uh oh!

nicoddemus commented Nov 7, 2016

Uh oh!

nicoddemus commented Nov 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicoddemus Oct 11, 2016 •

edited

Loading