assert: better formatting for Len() error by kevinburkesegment · Pull Request #1485 · stretchr/testify

kevinburkesegment · 2023-10-11T18:54:37Z

Previously, the use of %s with array objects meant you would get an error like this:

"[%!s(int=1) %!s(int=2) %!s(int=3)]\" should have 4 item(s), but has 3

Use %v instead, which provides a much nicer error.

"[1 2 3]" should have 4 item(s), but has 3

Fixes #1482.

dolmen

The test suite must cover many more types. I think this will show that %#v is more appropriate.

We should also handle the case where the len is large (ex: 1000 items). Displaying the full value is just pollution in that case. That could be handled in a separate MR, but at least the test suite must be structured to allow this future change.

kevinburkesegment · 2023-10-12T06:51:19Z

We should also handle the case where the len is large (ex: 1000 items). Displaying the full value is just pollution in that case.

To be clear I think the current "%s" formatting also does not do a great job in this case

kevinburkesegment · 2023-10-12T19:56:15Z

I added the types from the other len() tests, except for the channel types, which show up as a pointer address, that will change every single time.

dolmen

We don't need 2 loops on the same test cases.
Let's merge the old tests on the cases into your loop.

kevinburkesegment · 2023-10-13T06:18:57Z

Let's merge the old tests on the cases into your loop.

How do you want me to handle the channel tests?

dolmen · 2023-10-13T07:03:55Z

@kevinburkesegment Skip (just log) the format test if the want string is "".

dolmen · 2023-10-16T12:05:24Z

I just noticed we also have #1405 which is an alternative.

kevinburkesegment · 2023-10-16T16:00:24Z

I'll update the patch today

…

On Mon, Oct 16, 2023 at 5:05 AM Olivier Mengué ***@***.***> wrote: I just noticed we also have #1405 <https://urldefense.com/v3/__https://github.com/stretchr/testify/pull/1405__;!!NCc8flgU!aN8Sfyd7a7RW94068ciiBzy4dnwZuy4CSl3z6LPi4gmw4CvV8W7KnoX9rfl3G5bVjUIKqgRv7LbA7C6tU41cQwlDxag$> which is an alternative. — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/stretchr/testify/pull/1485*issuecomment-1764331060__;Iw!!NCc8flgU!aN8Sfyd7a7RW94068ciiBzy4dnwZuy4CSl3z6LPi4gmw4CvV8W7KnoX9rfl3G5bVjUIKqgRv7LbA7C6tU41cZlPvo_8$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AW5SL4U72JQO2HGHDXRM6XTX7UPJBAVCNFSM6AAAAAA54OPUQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRUGMZTCMBWGA__;!!NCc8flgU!aN8Sfyd7a7RW94068ciiBzy4dnwZuy4CSl3z6LPi4gmw4CvV8W7KnoX9rfl3G5bVjUIKqgRv7LbA7C6tU41cyQY816k$> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kevinburkesegment · 2023-10-16T17:09:25Z

OK, updated. I also shortened the test cases

kevinburkesegment · 2023-10-23T22:17:35Z

Hi, just following up - let me know if you'd like further changes here.

dolmen

Good improvements. We're nearly done.

assert/assertions_test.go

k1ng440 · 2023-10-31T07:10:23Z

LGTM.

dolmen · 2023-10-31T14:49:12Z

@k1ng440 Use the "Review changes" button to post a review, not a comment.

wwade

LGTM

dolmen

LGTM.

@kevinburkesegment I'm sorry for the time it took and the multiple exchanges, but we achieved together a short patch with a comprehensive test suite. Thank you!

kevinburkesegment · 2023-11-24T22:55:44Z

No worries! Anything I need to do to get this ready for merge?

kevinburkesegment · 2023-12-04T16:49:30Z

Hi, anything I can help with here?

kevinburkesegment · 2023-12-15T06:27:59Z

Hey, just wondering if there's anything I can do to get this ready for merge.

arjunmahishi · 2024-02-15T18:30:37Z

Hi @kevinburkesegment!

There seem to be a few panics in the tests. Can you please take a look at that?

Previously, the use of %s with array objects meant you would get an error like this: "[%!s(int=1) %!s(int=2) %!s(int=3)]\" should have 4 item(s), but has 3 Use %v instead, which provides a much nicer error. "[1 2 3]" should have 4 item(s), but has 3 Fixes stretchr#1482.

kevinburkesegment · 2024-02-15T18:36:47Z

I just rebased against latest master and the tests are passing. I checked the panics and I don't see how they are related to the change that I made.

arjunmahishi · 2024-02-15T18:43:20Z

Cool thanks!
Let me find a maintainer who has access to merge this.

dolmen added bug pkg-assert Change related to package testify/assert labels Oct 12, 2023

dolmen requested changes Oct 12, 2023

View reviewed changes

kevinburkesegment force-pushed the len-format branch 2 times, most recently from 9a7e703 to 2aa3316 Compare October 12, 2023 19:55

dolmen added the hacktoberfest-accepted Hacktoberfest label Oct 13, 2023

dolmen requested changes Oct 13, 2023

View reviewed changes

kevinburkesegment force-pushed the len-format branch from 6249b03 to 14c1b0e Compare October 16, 2023 17:09

dolmen requested changes Oct 24, 2023

View reviewed changes

assert/assertions_test.go Show resolved Hide resolved

kevinburkesegment force-pushed the len-format branch from 14c1b0e to aa1cb1a Compare October 24, 2023 20:25

dolmen requested changes Oct 30, 2023

View reviewed changes

assert/assertions_test.go Outdated Show resolved Hide resolved

kevinburkesegment force-pushed the len-format branch 2 times, most recently from 77c9475 to 33e4088 Compare October 30, 2023 21:43

dolmen mentioned this pull request Oct 31, 2023

assert.Len: use %v when formatting value #1086

Closed

wwade approved these changes Oct 31, 2023

View reviewed changes

kevinburkesegment force-pushed the len-format branch from 33e4088 to ab9aa4a Compare October 31, 2023 22:30

k1ng440 approved these changes Nov 1, 2023

View reviewed changes

dolmen approved these changes Nov 23, 2023

View reviewed changes

kevinburkesegment added 3 commits February 15, 2024 11:35

assert: shorten cases

f6ed021

try to do the whole format string

89c0872

kevinburkesegment force-pushed the len-format branch from c2db8b0 to 89c0872 Compare February 15, 2024 18:35

brackendawson merged commit 0e75f99 into stretchr:master Feb 16, 2024

brackendawson modified the milestones: v1.8.5, v1.9.0 Feb 23, 2024

renovate bot mentioned this pull request May 2, 2024

fix(deps): update minor go packages janritter/aws-lambda-live-tuner#60

Merged

1 task

Conversation

kevinburkesegment commented Oct 11, 2023

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

kevinburkesegment commented Oct 12, 2023

Uh oh!

kevinburkesegment commented Oct 12, 2023

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

kevinburkesegment commented Oct 13, 2023

Uh oh!

dolmen commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dolmen commented Oct 16, 2023

Uh oh!

kevinburkesegment commented Oct 16, 2023 via email

Uh oh!

kevinburkesegment commented Oct 16, 2023

Uh oh!

kevinburkesegment commented Oct 23, 2023

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

k1ng440 commented Oct 31, 2023

Uh oh!

dolmen commented Oct 31, 2023

Uh oh!

wwade left a comment

Choose a reason for hiding this comment

Uh oh!

dolmen left a comment

Choose a reason for hiding this comment

Uh oh!

kevinburkesegment commented Nov 24, 2023

Uh oh!

kevinburkesegment commented Dec 4, 2023

Uh oh!

kevinburkesegment commented Dec 15, 2023

Uh oh!

arjunmahishi commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinburkesegment commented Feb 15, 2024

Uh oh!

arjunmahishi commented Feb 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dolmen commented Oct 13, 2023 •

edited

Loading

arjunmahishi commented Feb 15, 2024 •

edited

Loading