The Wayback Machine - https://web.archive.org/web/20201208144826/https://github.com/google/gvisor/issues/4664
Skip to content

/ gvisor

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

Jump to bottom

NUD tests incorrectly report `cmp.Diff` #4664

Open
tamird opened this issue Oct 28, 2020 · 4 comments · May be fixed by #4705
Open

NUD tests incorrectly report `cmp.Diff` #4664

tamird opened this issue Oct 28, 2020 · 4 comments · May be fixed by #4705
Labels
area: networking area: tests good first issue type: enhancement

Comments

@tamird
Copy link
Contributor

@tamird tamird commented Oct 28, 2020

Per the example in https://godoc.org/github.com/google/go-cmp/cmp#Diff the form should be:

if diff := cmp.Diff(want, got); diff != "" {
    t.Errorf("MakeGatewayInfo() mismatch (-want +got):\n%s", diff)
}

however, NUD tests (and possibly others) are littered with the reverse, e.g.:

gvisor/pkg/tcpip/stack/neighbor_cache_test.go

Lines 252 to 254 in 8fa18e8

if diff := cmp.Diff(nudDisp.events, []testEntryEventInfo(nil)); diff != "" {
t.Errorf("nud dispatcher events mismatch (-got, +want):\n%s", diff)
}

Fix this.

@puradox cc @ghanan94
@puradox
Copy link
Contributor

@puradox puradox commented Oct 29, 2020

Does this break anything? Looks like we use (-) with got and (+) with want, which is preference with nothing necessarily broken.
@tamird
Copy link
Contributor Author

@tamird tamird commented Oct 29, 2020

The output is backwards, when tests fail "got" is presented as "want" and vice versa.
@tamird
Copy link
Contributor Author

@tamird tamird commented Oct 29, 2020

On second thought, this might be fine. I think I was confused by this output when I was attempting #4663.

Feel free to close this if you find the current output satisfactory.
@puradox
Copy link
Contributor

@puradox puradox commented Oct 30, 2020

I feel this output is satisfactory, as it states its definition of (+) and (-), but I see your point. I don't mind changing this, but have higher priority items to work on right now. Let's mark this as a good first bug if anyone else wants to take this on.
@puradox puradox removed their assignment Oct 30, 2020
@mlevesquedion mlevesquedion linked a pull request that will close this issue Oct 30, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: networking area: tests good first issue type: enhancement
3 participants
@ianlewis @puradox @tamird
You can’t perform that action at this time.