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

Fix/improve comparison of byte strings #5267

Closed
wants to merge 1 commit into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented May 15, 2019

Fixes #5260.

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #5267 into features will increase coverage by 1.16%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5267      +/-   ##
============================================
+ Coverage     92.07%   93.24%   +1.16%     
============================================
  Files           115      115              
  Lines         26159    26356     +197     
  Branches       2578     2623      +45     
============================================
+ Hits          24086    24575     +489     
+ Misses         1746     1465     -281     
+ Partials        327      316      -11
Impacted Files Coverage Δ
testing/test_assertion.py 97.69% <100%> (+0.02%) ⬆️
src/_pytest/assertion/util.py 98.16% <100%> (+0.01%) ⬆️
testing/python/raises.py 88.51% <0%> (-6.09%) ⬇️
testing/test_capture.py 99.33% <0%> (+0.08%) ⬆️
src/_pytest/fixtures.py 97.93% <0%> (+0.13%) ⬆️
src/_pytest/pytester.py 86.66% <0%> (+0.14%) ⬆️
src/_pytest/capture.py 95.28% <0%> (+0.18%) ⬆️
testing/python/collect.py 99.14% <0%> (+0.21%) ⬆️
testing/test_runner.py 60.3% <0%> (+0.65%) ⬆️
testing/python/metafunc.py 95.22% <0%> (+2.94%) ⬆️
... and 4 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 c0e53a6...58b3d2a. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2019

Azure failure is unrelated.
(cannot restart it myself?!)

@blueyed blueyed closed this May 15, 2019
@blueyed blueyed reopened this May 15, 2019
if PY3:
assert diff == [
"b'spam' == b'eggs'",
"At index 0 diff: 115 != 101",
Copy link
Member

Choose a reason for hiding this comment

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

hmmm I thought that we wouldn't see this line, but only see the - b'spam' + b'eggs' bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - it just skips the "contains more items" now.
I think it is fine / useful, although it could mention that those a bytes / ordinals.
If so, how should it sound like then?
Or should we skip any handling (i.e. as a sequence) for bytes?

Copy link
Member

Choose a reason for hiding this comment

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

hmmm I'd like to still see the reprdiff since I think that's useful

Copy link
Member

Choose a reason for hiding this comment

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

How about using the slice operator instead?

>>> b'foo'[0]
102
>>> b'foo'[0:1]
b'f'

We will then be able to see "At index 0 diff: b's' != b'e'", which is readable and has no downsides as far as I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a special case for bytes on py3 then, or for sequences in general?

Copy link
Member

@nicoddemus nicoddemus May 17, 2019

Choose a reason for hiding this comment

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

Yes, as a special case for bytes on py3. For sequences it is not clear to me it would be better than what we have now, at least for me.

@blueyed
Copy link
Contributor Author

blueyed commented May 23, 2019

I do not feel strong enough about this myself currently, closing for now.
Let's keep it for later - almost there.. ;)

@blueyed blueyed closed this May 23, 2019
@blueyed blueyed deleted the cmp-bytes branch May 23, 2019 07:46
@blueyed blueyed restored the cmp-bytes branch May 23, 2019 07:49
@blueyed blueyed reopened this May 23, 2019
@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 May 23, 2019
@nicoddemus
Copy link
Member

Let's keep it for later - almost there.. ;)

By "later" you mean after dropping Python 2 in 4.6/4.7?

@nicoddemus
Copy link
Member

Gentle ping :)

@blueyed
Copy link
Contributor Author

blueyed commented Jun 17, 2019

Busy pong.. ;)
Feel free to pick it up or close it to get off your radar.. :)

@nicoddemus
Copy link
Member

Superseded by #5495

@nicoddemus nicoddemus closed this Jun 25, 2019
asottile added a commit that referenced this pull request Jun 26, 2019
Improve comparison of byte strings (supersedes #5267)
@blueyed blueyed deleted the cmp-bytes branch September 16, 2019 14:51
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.

3 participants