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

_diff_text: use repr with escape characters #6702

Closed

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Feb 10, 2020

This helps especially with comparing strings containing terminal escape
sequences, but also newlines already.

Ref: #3443

TODO:

@blueyed blueyed changed the base branch from features to master February 12, 2020 15:08
testing/test_assertion.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
r"- 'spam\n'",
r"+ 'eggs\n'",
r" 'bar'",
]
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this change (and others) - do you consider a newline a non-printable/zero-width character? Why? It seems quite confusing to me to see a multi-line output but also \n, i.e. with newlines represented twice.

Copy link
Contributor Author

@blueyed blueyed Feb 14, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be made smarter though maybe to not show them when not at the end of a line?

Copy link
Member

Choose a reason for hiding this comment

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

For illustration, on master:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 1 bar
E         ? ^
E         + 2 bar
E         ? ^

On this branch:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E         NOTE: Strings contain non-printable/zero-width characters. Escaping them using repr().
E           'foo\n'
E         - '1 bar'
E         ?  ^
E         + '2 bar'
E         ?  ^

Is this what you would prefer @The-Compiler ?

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E         NOTE: Strings contain non-printable/zero-width characters. Escaping them using repr().
E         - 'foo\n1 bar'
E         ?       ^
E         + 'foo\n2 bar'
E         ?       ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For illustration, on master:

    def test_eq_similar_text():
        x ="foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 1 bar
E         ? ^
E         + 2 bar
E         ? ^

On master it has a different order (which I find still wrong - it should be left-to-right, top-to-bottom, with only +/- swapped, but off topic here - maybe therefore edited manually?):

    def test_eq_similar_text(self):
        x = "foo\n1 bar"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar' == 'foo\n2 bar'
E           foo
E         - 2 bar
E         ? ^
E         + 1 bar
E         ? ^

How about?

    def test_eq_similar_text(self):
        x = "foo\n1 bar\n"
>       assert x == "foo\n2 bar"
E       AssertionError: assert 'foo\n1 bar\n' == 'foo\n2 bar'
E         NOTE: Strings contain different line-endings. Escaping them using repr().
E         - 'foo\n1 bar\n'
E         ?       ^    --
E         + 'foo\n2 bar'
E         ?       ^

However, with longer strings it is useful to split them on newlines, of course.

>       assert x == "foo\n1 bar"
E       AssertionError: assert 'foo\n1 bar\n' == 'foo\n1 bar'
E         NOTE: Strings contain different line-endings. Escaping them using repr().
E           foo
E         - 1 bar\n
E         ?      --
E         + 1 bar
E         -

FWIW it always looked a bit strange to me seeing:

- foo
+ foo
     ^

(it could also be a space etc)

E         - 'foo\n1 bar'
E         ?       ^
E         + 'foo\n2 bar'
E         ?       ^

This could also be triggered via some minimal length (related: blueyed#218, where I split it onto separate lines with a certain length).

Copy link
Member

@The-Compiler The-Compiler Feb 16, 2020

Choose a reason for hiding this comment

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

Hmm, I guess explicit is indeed better than implicit in this case. I agree having the ^ marker pointing to "nothing" is odd, and I remember people being confused about that.

src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
testing/test_assertion.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

WIP to handle different line endings only

src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
src/_pytest/assertion/util.py Outdated Show resolved Hide resolved
@blueyed blueyed force-pushed the diff-text-repr-escape-upstream branch from 43b11af to 2ba59a1 Compare February 15, 2020 14:06
@blueyed blueyed added topic: rewrite related to the assertion rewrite mechanism type: enhancement new feature or API change, should be merged into features branch labels Feb 15, 2020
blueyed added a commit to blueyed/pytest that referenced this pull request Feb 20, 2020
blueyed added a commit to blueyed/pytest that referenced this pull request Feb 20, 2020
@blueyed blueyed closed this Mar 27, 2020
@blueyed blueyed deleted the diff-text-repr-escape-upstream branch March 27, 2020 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rewrite related to the assertion rewrite mechanism type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants