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.

Sign up for GitHub

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

chore(deps): Bump github.com/stretchr/testify to v1.10.0 #1114

Merged
merged 1 commit into from
Jan 11, 2025
Merged

chore(deps): Bump github.com/stretchr/testify to v1.10.0 #1114

merged 1 commit into from
Jan 11, 2025

Conversation

aalekseevx
Copy link
Contributor

Updating testify introduces stricter rules for using require.NotSame. The implementation now enforces passing pointers, making certain checks that were previously always valid unnecessary. These redundant checks have been removed from the codebase.`

@aalekseevx aalekseevx requested a review from a team as a code owner January 9, 2025 12:44
@aalekseevx aalekseevx force-pushed the update-testify branch 2 times, most recently from e01e70d to 6c5ce18 Compare January 9, 2025 13:17
@duglin
Copy link
Contributor

duglin commented Jan 9, 2025

@aalekseevx
can you run "go mod tidy" in observability/opencensus/v2 and binding/format/protobuf/v2.
I think that'll fix the current errors we're seeing.

After that I'm seeing a few errors from the tests that I think are related to your changes.

@aalekseevx
Copy link
Contributor Author

@duglin, thank you for the quick reply! I'll investigate the issues

@duglin
Copy link
Contributor

duglin commented Jan 9, 2025

Actually, it might be more dirs. Try: hack/go-mod-tidy.sh

=== RUN   TestEvent_Clone
    event_test.go:533: 
                Error Trace:    /root/go/src/github.com/cloudevents/sdk-go/v2/event/event_test.go:533
                Error:          Both arguments must be pointers
                Test:           TestEvent_Clone
    event_test.go:539: 
                Error Trace:    /root/go/src/github.com/cloudevents/sdk-go/v2/event/event_test.go:539
                Error:          Both arguments must be pointers
                Test:           TestEvent_Clone

@aalekseevx aalekseevx force-pushed the update-testify branch 2 times, most recently from fc9125b to 649b5f0 Compare January 9, 2025 13:56
@aalekseevx
Copy link
Contributor Author

@duglin Thank you for your help! Could you please review the new fixes and let me know if they work for you?

duglin
duglin previously approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@duglin duglin left a comment

Choose a reason for hiding this comment

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

LGTM

ping @embano1 for the merge

duglin
duglin reviewed Jan 9, 2025
View reviewed changes
v2/types/value_test.go
}

func TestCloneTime(t *testing.T) {
original := time.Now()
cloned := types.Clone(original).(types.Timestamp)

require.Equal(t, original, cloned.Time)
require.NotSame(t, original, cloned.Time)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the original... how can it be equal and not equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it was intended to check whether the values are equal and their addresses differ.

Copy link
Contributor

Choose a reason for hiding this comment

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

that really hurts my head :-)
Sure reads like a bug in "require" to me.
So, in this case did you remove it because "NotSame" fails or because it's redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah "same" vs "equal". I missed that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Removed because it seems redundant for me to check that &original и &cloned.Time are 2 different addresses

@aalekseevx
chore(deps): Bump github.com/stretchr/testify to v1.10.0
df16e77 
Signed-off-by: Aleksandr Alekseev <alekseev-dev@yandex-team.ru>
@aalekseevx aalekseevx requested a review from duglin January 10, 2025 09:25
@aalekseevx
Copy link
Contributor Author

@duglin, @embano1 are there any blockers for merge?

@embano1
Copy link
Member

embano1 commented Jan 11, 2025

Fantastic, thx"

@embano1 embano1 enabled auto-merge January 11, 2025 15:18
embano1
embano1 approved these changes Jan 11, 2025
View reviewed changes
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

LGTM

@embano1 embano1 merged commit 7e6729c into cloudevents:main Jan 11, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@embano1 embano1 embano1 approved these changes

@duglin duglin Awaiting requested review from duglin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aalekseevx @duglin @embano1