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

Use hex string compare for byte array mismatch #239

Closed
jart opened this issue Mar 28, 2016 · 14 comments
Closed

Use hex string compare for byte array mismatch #239

jart opened this issue Mar 28, 2016 · 14 comments
Assignees
Labels
type=addition A new feature

Comments

@jart
Copy link

jart commented Mar 28, 2016

It would be nice to have something like this:

expected:<...C6503636F6D000001000[1]> but was:<...C6503636F6D000001000[0]>

Rather than this:

Not true that <(byte[]) [124, 112, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 7, 101, 120, 97, 109, 112, 108, 101, 3, 99, 111, 109, 0, 0, 1, 0, 0]> is equal to <[124, 112, 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 7, 101, 120, 97, 109, 112, 108, 101, 3, 99, 111, 109, 0, 0, 1, 0, 1]>

@cgruber
Copy link
Contributor

cgruber commented Aug 10, 2016

Interesting. I like this. Any strong objections to this?

@cgruber cgruber self-assigned this Aug 10, 2016
@kluever
Copy link
Member

kluever commented Aug 10, 2016

I think I tried this but ran into some issues...let me see if I can dig up
the CL

@jrtom
Copy link
Member

jrtom commented Aug 10, 2016

You can do that when the expected and actual arrays are the same length, certainly.
If they aren't, do you have specific suggestions for how to represent the diffs compactly?

@kluever
Copy link
Member

kluever commented Aug 11, 2016

@cgruber Internal CL is 118401565 if you'd like to patch it and take it over while I'm out.

@dinesh707
Copy link

Hi, can i try to fix this issue ?. From the dates it seems that no one is doing anything in this issue at the moment. I went tough the Subject class and AbstractArraySubject, it looks like the fix may include some cleanup too.

@kluever
Copy link
Member

kluever commented Feb 21, 2017

Hi @dinesh707 thanks for the offer...I actually have a pending change to fix this, but there were some questions about what exactly to put into the failure message.

Do we want just the base16 encoded byte arrays? The byte arrays and the base16 strings? Any thoughts?

@dinesh707
Copy link

dinesh707 commented Feb 21, 2017

One problem we have at the moment is now that we print the whole byte array. It will be really hard to find out which byte/bytes were wrong because of there is too much data. In order to address that we can say

Assertion failed, first mismatch occurred at index 34, expected:<...C6503636F6[D0]...> but was:<...C6503636F6[D1]......> .

But looking at the classes AbstractArraySubject and Subject its not easy to manipulate the outputs (may be for good reasons).

@kluever
Copy link
Member

kluever commented Feb 21, 2017

OK, I've got something pretty reasonable worked up internally that I'm about to submit. It'll change from this message:
Not true that <(byte[]) [0, 1]> is equal to <[1, 0]>
to this:
Not true that <(byte[]) [0, 1]> is equal to <[1, 0]>; expected:<0[100]> but was:<0[001]>

@kluever kluever assigned kluever and unassigned cgruber Feb 21, 2017
@kluever
Copy link
Member

kluever commented Feb 21, 2017

And for really long byte arrays, it'll truncate the common prefix/suffix (it uses JUnit's ComparisonFailure):

expected:<...C6503636F6D000001000[1]> but was:<...C6503636F6D000001000[0]>

@dinesh707
Copy link

Sounds good. But since every Byte is denoted by 2 Hex chars, when we show what went wrong shoudn't we always use a window of 2 chars?

So this
expected:<0[100]> but was:<0[001]>
should be
expected:<[0100]> but was:<[0001]>

@kluever
Copy link
Member

kluever commented Feb 22, 2017

@dinesh707 Good point...without re-writing the string matching behavior from JUnit, there's not much we can do about it though.

@dinesh707
Copy link

Is it worth re-doing it ?, may be as a separate ticket ?

@kluever
Copy link
Member

kluever commented Feb 22, 2017

Feel free to open it, but it'll be a pretty low priority feature request.

@cgruber
Copy link
Contributor

cgruber commented Feb 23, 2017 via email

@dimo414 dimo414 closed this as completed Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type=addition A new feature
Projects
None yet
Development

No branches or pull requests

6 participants