assert: improve uint formatting on mismatch#1223
assert: improve uint formatting on mismatch#1223mnotti wants to merge 3 commits intostretchr:masterfrom
Conversation
|
@boyan-soubachov could i get some quick eyes on this one if you get some cycles? |
|
@mnotti Could you rebase your branch on current master? |
|
Simplified test case to confirm the issue on latest release: https://go.dev/play/p/6xEleN5v45P |
7fa3412 to
ebb036f
Compare
Done. |
|
Hey @dolmen would you be able to take a look at this PR, please? Just spent some time debugging a test case thinking that |
| case time.Duration: | ||
| return fmt.Sprintf("%v", expected), fmt.Sprintf("%v", actual) | ||
| case time.Duration, uint, uint8, uint16, uint32, uint64: | ||
| return fmt.Sprint(expected), fmt.Sprint(actual) |
There was a problem hiding this comment.
Formatting these with "%[1]T(%[1]v)" would be better so that we don't say two apparently identical numbers are different. Otherwise we regress assert.Equal(t, int(1), uint(1)).
There was a problem hiding this comment.
Also, using this format for all numeric types would fix #912, but this is creeping somewhat, perhaps better in a follow up PR.
There was a problem hiding this comment.
Reformatting a test suite is not something we want in the same PR as a change of the feature tested.
Please, add tests if necessary, but don't change existing tests, even if that is just reformatting. Because that makes review harder.
Reformatting of a test suite should be done in a PR that does only that.
|
I'm happy doing the work to review that the changes in the unit test are correct in this PR. I see an argument where these additions tip the threshold of making this into a tabled test. My previous review comments are still un-addressed though. |
Merged from stretchr#1223 The issue is described at stretchr#400 Thanks to the original contributor @mnotti Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Merged from stretchr#1223 The issue is described at stretchr#400 Thanks to the original contributor @mnotti Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Merged from stretchr#1223 The issue is described at stretchr#400 Thanks to the original contributor @mnotti Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Summary
Cleans the formatting on uint mismatches. AND refactors unit tests as a little bonus :) .
Changes
I added uint types to switch on type in
formatUnequalValuesto use%vinstead of%#vwhich was giving weird hex display.I also refactored the accompanying unit test to use a more conventional testCase struct loop.
Motivation
All uints show weird hex value when displaying diffs which is confusing and annoying when debugging failed tests.
https://go.dev/play/p/hEUdfo-pVlP
Seems to be a known issue since 2017 and no one has done anything so I thought I'd take a quick stab at it.
(#400)
Related issues
Closes #400