Skip to content

Fixed errcheck issues in server/channels/app/channel_test.go #29233

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
Merged
hanzei merged 4 commits into from
Nov 19, 2024
Merged

Fixed errcheck issues in server/channels/app/channel_test.go #29233

hanzei merged 4 commits into from
Nov 19, 2024

Conversation

@edlerd
Copy link
Contributor

@edlerd edlerd commented Nov 12, 2024

Summary

Fixed errcheck issues in server/channels/app/channel_test.go

Ticket Link

Fixes #28784

@mm-cloud-bot
Copy link

mm-cloud-bot commented Nov 12, 2024

@edlerd: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

I understand the commands that are listed here

@mattermost-build
Copy link
Contributor

mattermost-build commented Nov 12, 2024

Hello @edlerd,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@hanzei hanzei self-requested a review November 13, 2024 06:14
@hanzei hanzei added the 2: Dev Review Requires review by a developer label Nov 13, 2024
hanzei
hanzei requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you please take a look at the linter issues?

Error: channels/app/channel_test.go:1838:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/channel_test.go:2229:5: calling require.Nil on error, please use require.NoError instead
Error: channels/app/channel_test.go:2259:5: calling require.Nil on error, please use require.NoError instead
Error: channels/app/channel_test.go:2261:5: calling require.Nil on error, please use require.NoError instead
Error: channels/app/channel_test.go:2268:5: calling require.Nil on error, please use require.NoError instead
Error: channels/app/channel_test.go:2270:5: calling require.Nil on error, please use require.NoError instead

Thanks!

@hanzei
Copy link
Contributor

hanzei commented Nov 13, 2024

/release-note-none

@mm-cloud-bot mm-cloud-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed labels Nov 13, 2024
@edlerd edlerd force-pushed the fix-lint-channel-test branch from 28c4a6a to ff5816a Compare November 13, 2024 13:54
@edlerd edlerd requested a review from hanzei November 13, 2024 13:59
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024

Updated those issue and ensured make vet passes locally. Please have another look at this and 🤞 now it passes CI and review.

@edlerd edlerd force-pushed the fix-lint-channel-test branch from ff5816a to c3837df Compare November 13, 2024 17:17
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024 •
edited
Loading

Suspect the reuse of the err object in the anonymous deferred functions caused the panic in CI. Giving them a dedicated name and new object to avoid this.

hanzei
hanzei requested changes Nov 13, 2024
View reviewed changes
@edlerd edlerd force-pushed the fix-lint-channel-test branch from c3837df to 9e24d2a Compare November 13, 2024 20:08
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024

Finally this should be resolved and pass CI 🤞 at least locally I could reproduce, and it is passing now.

@edlerd edlerd force-pushed the fix-lint-channel-test branch 2 times, most recently from 045f578 to 7eb1fee Compare November 13, 2024 21:18
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024

Ensured all test cases and make vet passes locally.

@edlerd edlerd requested a review from hanzei November 13, 2024 21:20
hanzei
hanzei requested changes Nov 14, 2024
View reviewed changes
hanzei
hanzei reviewed Nov 14, 2024
View reviewed changes
@edlerd edlerd force-pushed the fix-lint-channel-test branch from 7eb1fee to 874be14 Compare November 14, 2024 09:17
@hanzei hanzei self-requested a review November 14, 2024 09:32
@edlerd
Fixed errcheck issues in server/channels/app/channel_test.go mattermo…
9c75b27 
…st#28784

Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd edlerd force-pushed the fix-lint-channel-test branch from 874be14 to 9c75b27 Compare November 14, 2024 09:48
hanzei
hanzei requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Amazing work @edlerd 🎉

@edlerd
Revert unrelelated rename of users to testUsers, remove extra newline
bfcca9a 
Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd edlerd requested a review from hanzei November 14, 2024 12:04
@edlerd
Fix TestGetChannelMembersTimezones code, add user to team first
fbf762b 
Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd
Copy link
Contributor Author

edlerd commented Nov 14, 2024

This is ready for another review.

@edlerd
Copy link
Contributor Author

edlerd commented Nov 14, 2024

@hanzei everything passes except enterprise tests, which I can't access. is it even related to the changes in here? Maybe this can merge as is?

@edlerd
Copy link
Contributor Author

edlerd commented Nov 19, 2024

@hanzei everything passing, can this be merged?

hanzei
hanzei approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

LGTM, great work 🎉

@hanzei hanzei requested review from a team and isacikgoz and removed request for a team November 19, 2024 09:22
isacikgoz
isacikgoz approved these changes Nov 19, 2024
View reviewed changes
Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks both @edlerd and @hanzei 🚀

@isacikgoz isacikgoz added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Nov 19, 2024
@hanzei hanzei modified the milestones: v10.3.0, v10.4.0 Nov 19, 2024
@hanzei hanzei merged commit 03a4462 into mattermost:master Nov 19, 2024
28 checks passed
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 25, 2024
isacikgoz pushed a commit that referenced this pull request Dec 6, 2024
@edlerd @hanzei @isacikgoz
Fixed errcheck issues in server/channels/app/channel_test.go (#29233)
b6dd9a8 
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isacikgoz isacikgoz isacikgoz approved these changes

@hanzei hanzei hanzei approved these changes

Assignees

No one assigned

Labels

4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Contributor Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.

Projects

None yet

Milestone

v10.4.0

Development

Successfully merging this pull request may close these issues.

Fix errcheck issues in server/channels/app/channel_test.go

6 participants

@edlerd @mm-cloud-bot @mattermost-build @hanzei @isacikgoz @amyblais