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 verbose output by wrapping skip/xfail reasons with margin #10958

Merged
merged 4 commits into from
May 6, 2023

Conversation

brl0
Copy link
Contributor

@brl0 brl0 commented May 1, 2023

Fixes #10940.

This functionality is covered by an existing test that only needed minor updates to match new output. There were a couple of identical lines in the output used for text matching, so I added numbers to disambiguate.

Let me know if there are any suggestions or changes needed.

@brl0 brl0 mentioned this pull request May 1, 2023
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Hi @brl0 , thanks for the PR!

Here is the output from this branch for test_verbose_skip_reason:

test_verbose_skip_reason.py::test_1 SKIPPED (123)              [  8%]
test_verbose_skip_reason.py::test_2 XPASS (456)                [ 16%]
test_verbose_skip_reason.py::test_3 XFAIL (789)                [ 25%]
test_verbose_skip_reason.py::test_4 XFAIL                      [ 33%]
test_verbose_skip_reason.py::test_5 SKIPPED (unconditional...) [ 41%]
test_verbose_skip_reason.py::test_6 XPASS                      [ 50%]
test_verbose_skip_reason.py::test_7 SKIPPED                    [ 58%]
test_verbose_skip_reason.py::test_8 SKIPPED (888 is great)     [ 66%]
test_verbose_skip_reason.py::test_9 XFAIL                      [ 75%]
test_verbose_skip_reason.py::test_10 XFAIL (It's 🕙 o'clock)   [ 83%]
test_verbose_skip_reason.py::test_long_skip SKIPPED (1 can...) [ 91%]
test_verbose_skip_reason.py::test_long_xfail XFAIL (2 cann...) [100%]

And here is the output for main:

test_verbose_skip_reason.py::test_1 SKIPPED (123)              [  8%]
test_verbose_skip_reason.py::test_2 XPASS (456)                [ 16%]
test_verbose_skip_reason.py::test_3 XFAIL (789)                [ 25%]
test_verbose_skip_reason.py::test_4 XFAIL                      [ 33%]
test_verbose_skip_reason.py::test_5 SKIPPED (unconditional...) [ 41%]
test_verbose_skip_reason.py::test_6 XPASS                      [ 50%]
test_verbose_skip_reason.py::test_7 SKIPPED                    [ 58%]
test_verbose_skip_reason.py::test_8 SKIPPED (888 is great)     [ 66%]
test_verbose_skip_reason.py::test_9 XFAIL                      [ 75%]
test_verbose_skip_reason.py::test_10 XFAIL (It's 🕙 o'clock)   [ 83%]
test_verbose_skip_reason.py::test_long_skip SKIPPED (canno...) [ 91%]
test_verbose_skip_reason.py::test_long_xfail XFAIL (cannot...) [100%]

It does not look like it was doing what was intended, or am I missing something?

changelog/10940.improvement.rst Outdated Show resolved Hide resolved
@@ -426,6 +427,28 @@ def ensure_newline(self) -> None:
self._tw.line()
self.currentfspath = None

def wrap_write(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure "wrap" is appropriated, given "wrap" in this contents is to preserve the text, but wrapping it around at certain width (for example 70). I think what we are doing here is actually truncating, so perhaps truncate_write or something would be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to rename to whatever, but in this case, the new method is only wrapping text. The truncated output you saw was unmodified behavior due to the test testing multiple outputs.
I'm not particularly satisfied with the name because I'm not sure that it feels sufficiently conventional. Perhaps write_wrapped_text or something similar would be better?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I misunderstood the output hence my misguided suggestion. wrap_write is fine, thanks! 👍

),
"because baz is missing due to I don't know what) *",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus,

Thanks for taking a look.

It does not look like it was doing what was intended, or am I missing something?

The test test_verbose_skip_reason runs pytest twice, once with -v, and once with -vv. The output you provided was from the first run, which is less verbose, hence the truncation. I modified the expected output for the second run to expect the output to continue onto a new line in the middle of the output. This comment annotates that modification to help show more clearly that the modified test does check for the changed behavior.
Hopefully that makes sense, but let me know if it needs further clarification or changes to make it more clear. For example, we could break the runs into two separate tests, and move the common inputs and outputs into a fixture rather than the test body.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh right, sorry I missed that, thanks for the explanation. 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot, appreciate the contribution!

I will merge this in the next few days to give others a chance to review. 👍

@nicoddemus nicoddemus merged commit 7d548c3 into pytest-dev:main May 6, 2023
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.

Improve progress alignment
2 participants