Color the full diff that pytest shows as a diff #11530

BenjaminSchubert · 2023-10-21T10:34:21Z

Overview

Previously, it would get printed all in red, which makes it hard to read and actually understand.

However, the diffs shown are standard and have a supported lexer in Pygments. As such, use this to color the output when pygments is available.

This is a step towards #11520 but does not address all the discussion points yet.

Visual changes

Previously

With this change

Questions

I am unsure about two things here:

How we expose the lexers on the terminal writer. Having to pass a string and hoping it will do the right thing is not ideal, I believe it is slightly better than providing one when doing the call as, that way, only the terminalwriter has to handle whether pygments is available or not
The testing might be a bit clunky and prone to breaking. If needed we could reduce the strictness of checks, let me know what you prefer

Previously, it would get printed all in red, which makes it hard to read and actually understand. However, the diffs shown are standard and have a supported lexer in Pygments. As such, use this to color the output when pygments is available.

nicoddemus · 2023-10-21T11:47:58Z

Thanks a lot @BenjaminSchubert, this looks great!

How we expose the lexers on the terminal writer. Having to pass a string and hoping it will do the right thing is not ideal, I believe it is slightly better than providing one when doing the call as, that way, only the terminalwriter has to handle whether pygments is available or not

Yeah this seems fine to me. 👍

The testing might be a bit clunky and prone to breaking. If needed we could reduce the strictness of checks, let me know what you prefer

Yeah, I agree. Perhaps we can do some really shallow testing there: we don't need to test the full diff coloring, we just need to ensure that diff coloring has been applied. So we can check that some coloring has been applied in a diff output in a normal run, and then no diff coloring in a 2nd run of the same code but with --colors=no.

BenjaminSchubert · 2023-10-21T11:56:45Z

@nicoddemus thanks for the quick feedback :)

Yeah, I agree. Perhaps we can do some really shallow testing there: we don't need to test the full diff coloring, we just need to ensure that diff coloring has been applied. So we can check that some coloring has been applied in a diff output in a normal run, and then no diff coloring in a 2nd run of the same code but with --colors=no.

I have pushed a fixup to reduce the amount of checks, which should make it easier to maintain on the long run. I can still trim it down more by just matchin we have a green +, but I think as of now it looks potentially better?

I just need to figure out why the CI is failing in some cases only now, which I am currently unable to reproduce locally

nicoddemus · 2023-10-21T12:00:08Z

I have pushed a fixup to reduce the amount of checks, which should make it easier to maintain on the long run. I can still trim it down more by just matchin we have a green +, but I think as of now it looks potentially better?

I was thinking of testing a single sample code, but I think this is better now and good to go now. 👍

I just need to figure out why the CI is failing in some cases only now, which I am currently unable to reproduce locally

Yeah me neither. I noticed however the failures happen in runs using xdist, but I also was unable to reproduce locally (using xdist too).

BenjaminSchubert · 2023-10-21T12:05:12Z

Yeah me neither. I noticed however the failures happen in runs using xdist, but I also was unable to reproduce locally (using xdist too).

No worries, I'll investigate more in depth, it does seem triggered by my change, as a run of the main branch does indeed pass

nicoddemus · 2023-10-21T12:23:01Z

It is really a head scratcher.

Because this cannot be reproduced this locally it seems, to debug/investigate this I would push a series of commits:

Reduce the build matrix to just run one of the breaking environments (for example py310-ubuntu) -- mostly to avoid running a ton of environments while we debug.
Comment out the new test, to see if the test itself is somehow affecting the others.
Start to slowly revert and commit each change, one by one (for example, start by reverting the changes in conftest.py).

Hopefully at some point this approach will point out to a small change that causes the other tests to break, and from there it might facilitate discovering what's going on.

BenjaminSchubert · 2023-10-21T13:07:27Z

@nicoddemus That's a weird one. 7cbe6ef is enough to fix it. I am unsure why it breaks otherwise but don't think it's worth investigating more?

Let me know if you want me to squash the commits or if anything else needs updating :)

BenjaminSchubert · 2023-10-21T13:15:50Z

Apologies for the noise, this was not it

nicoddemus · 2023-10-21T14:03:34Z

Apologies for the noise, this was not it

Oh OK, thanks for the investigation!

Let me know if you want me to squash the commits or if anything else needs updating :)

Don't worry about squashing, we can do so before merging. 👍

BenjaminSchubert · 2023-10-21T14:50:48Z

I have a reproduction for the bug... I have no idea what's going on there.

main...BenjaminSchubert:pytest:test-me-bug-repro has most cases of python<3.11 failing (except some??) and barely does any changes.

The changes to the test.yml are just to get the tests to run from a fork, so the only change is actually main...BenjaminSchubert:pytest:test-me-bug-repro#diff-e37001cd4b585aab5d9ddae3a0b6c2658adb2f096545acd8b50b778d12beb5ca

Which is basically:

import pytest


# To fail you need at least:
#   - 2 parametrize, with at least 2 value each. Content of the value does not
#     seem to matter
#   - use pytester
@pytest.mark.parametrize("foo", [True, False])
@pytest.mark.parametrize("bar", [True, False])
def test_bug(pytester, foo, bar) -> None:
    pass

And this is the check run: https://github.com/BenjaminSchubert/pytest/actions/runs/6597999568

I'll see if I can write the tests differently without them becoming non-sensical

BenjaminSchubert · 2023-10-21T15:00:53Z

The only thing that all the failing tests have in common is xdist. It looks like running this test under xdist fails.

BenjaminSchubert · 2023-10-21T15:10:29Z

I can reproduce locally. pytest -n2 on the whole codebase does trigger the bug. pytest -n{3,4,5,6,7,8,9,10,16} does not.

Running pytest -n2 testing/test_assertion.py testing/test_junitxml.py does not trigger it

nicoddemus · 2023-10-21T18:30:57Z

Found the problematic test, this fails locally:

pytest testing/logging/test_fixture.py::test_change_level_logging_disabled testing/test_junitxml.py::TestPython::test_failure_function[xunit1-log]

This also fails on main, so unrelated to this PR.

I will work on another PR fixing this, and then we can rebase yours to fix this.

Thanks for the patience.

Note: I used https://github.com/esss/pytest-replay to get the list of tests running in a specific node (because of xdist), and then manually bisected the failure (I tried using https://github.com/asottile/detect-test-pollution but got an error).

Logging has many global states, and we did foresee this by creating a ``cleanup_disabled_logging`` fixture, however one might still forget to use it and failures leak later -- sometimes not even in the same PR, because the order of the tests might change in the future, specially when running under xdist. This problem surfaced during pytest-dev#11530, where tests unrelated to the change started to fail.

BenjaminSchubert · 2023-10-21T19:35:47Z

@nicoddemus good catch, I was still very far from finding it :D Thank you very much

nicoddemus · 2023-10-21T19:37:51Z

@nicoddemus good catch, I was still very far from finding it :D Thank you very much

Sure thing!

I opened #11531 also, we can rebase this branch once that gets merged.

bluetech

I like this change a lot, thanks @BenjaminSchubert. The code looks good to me. As you said there are a few more places that can benefit from this, but can do it in follow up PRs.

bluetech · 2023-10-23T13:39:16Z

src/_pytest/assertion/util.py

@@ -189,7 +190,8 @@ def assertrepr_compare(
    explanation = None
    try:
        if op == "==":
-            explanation = _compare_eq_any(left, right, verbose)
+            writer = config.get_terminal_writer()
+            explanation = _compare_eq_any(left, right, writer, verbose)


I don't like that we pass the full TerminalWriter to these functions, because it makes it seem like the functions actually write to the terminal, while they only need it for the pure _highlight function. But it's OK for now, not asking you to refactor :)

If you prefer, we could instead pass a highlight function (Callable[[str], str]) which would be TerminalWriter._highlight

I like that idea.

Ok, I have pushed a fixup for this. I wasn't sure where to add the type definition, feel free to update/let me know if there's a better place

I think that looks great, thanks!

Logging has many global states, and we did foresee this by creating a ``cleanup_disabled_logging`` fixture, however one might still forget to use it and failures leak later -- sometimes not even in the same PR, because the order of the tests might change in the future, specially when running under xdist. This problem surfaced during pytest-dev#11530, where tests unrelated to the change started to fail.

BenjaminSchubert mentioned this pull request Oct 21, 2023

Improve pytest's Full diff by coloring it's output #11520

Closed

fixup! Color the full diff that pytest shows as a diff

2f649bf

fixup! fixup! Color the full diff that pytest shows as a diff

7cbe6ef

nicoddemus mentioned this pull request Oct 21, 2023

Ensure logging tests always cleanup after themselves #11531

Merged

Update changelog

c836919

bluetech approved these changes Oct 23, 2023

View reviewed changes

nicoddemus and others added 3 commits October 23, 2023 11:00

Merge branch 'main' into bschubert/colored-diffs

bc11201

fixup! Color the full diff that pytest shows as a diff

13459c3

fixup! fixup! Color the full diff that pytest shows as a diff

7e477ac

nicoddemus approved these changes Oct 24, 2023

View reviewed changes

nicoddemus merged commit fbe3e29 into pytest-dev:main Oct 24, 2023
21 of 22 checks passed

BenjaminSchubert deleted the bschubert/colored-diffs branch October 24, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color the full diff that pytest shows as a diff #11530

Color the full diff that pytest shows as a diff #11530

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023 •

edited

Loading

BenjaminSchubert commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023 •

edited

Loading

BenjaminSchubert commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023 •

edited

Loading

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

bluetech left a comment

bluetech Oct 23, 2023

BenjaminSchubert Oct 23, 2023

nicoddemus Oct 23, 2023

BenjaminSchubert Oct 23, 2023

nicoddemus Oct 24, 2023

Color the full diff that pytest shows as a diff #11530

Color the full diff that pytest shows as a diff #11530

Conversation

BenjaminSchubert commented Oct 21, 2023

Overview

Visual changes

Previously

With this change

Questions

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023 • edited Loading

BenjaminSchubert commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023 • edited Loading

BenjaminSchubert commented Oct 21, 2023

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023 • edited Loading

BenjaminSchubert commented Oct 21, 2023

nicoddemus commented Oct 21, 2023

bluetech left a comment

Choose a reason for hiding this comment

bluetech Oct 23, 2023

Choose a reason for hiding this comment

BenjaminSchubert Oct 23, 2023

Choose a reason for hiding this comment

nicoddemus Oct 23, 2023

Choose a reason for hiding this comment

BenjaminSchubert Oct 23, 2023

Choose a reason for hiding this comment

nicoddemus Oct 24, 2023

Choose a reason for hiding this comment

nicoddemus commented Oct 21, 2023 •

edited

Loading

BenjaminSchubert commented Oct 21, 2023 •

edited

Loading

nicoddemus commented Oct 21, 2023 •

edited

Loading