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(linters): Fix findings found by testifylint: go-require for zabbix #15727

Merged
merged 3 commits into from
Aug 22, 2024
Merged

chore(linters): Fix findings found by testifylint: go-require for zabbix #15727

merged 3 commits into from
Aug 22, 2024

Conversation

zak-pawel
Copy link
Collaborator

Summary

This is only the first part of a larger effort to address the findings identified by testifylint: go-require: #15535
Once all the findings have been addressed, testifylint: go-require can be enabled in .golangci.yml.

In this PR, I'm focusing on just one test file: zabbix_test.go, to start a discussion on whether this approach to fixing go-require issues works for us.

Here are the findings that this PR addresses:

plugins/outputs/zabbix/zabbix_test.go:478:17  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:489:6   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:490:6   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:492:6   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:578:14  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:579:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:582:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:583:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:586:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:587:3   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:589:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:592:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:593:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:596:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:597:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:600:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:601:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:605:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:606:3   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:608:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:611:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:612:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:616:13  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:617:3   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:619:3   testifylint  go-require: compareData contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:693:7   testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function
plugins/outputs/zabbix/zabbix_test.go:696:14  testifylint  go-require: listenForZabbixMetric contains assertions that must only be used in the goroutine running the test function

Checklist

  • No AI generated code was used in this PR

@telegraf-tiger telegraf-tiger bot added the chore label Aug 10, 2024
srebhan
srebhan reviewed Aug 12, 2024
View reviewed changes
plugins/outputs/zabbix/zabbix_test.go Outdated
Comment on lines 485 to 490
req, err := listenForZabbixMetric(t, listener, len(test.zabbixMetrics) == 0)
if err != nil {
innerErrs <- err
} else {
success <- req
}
Copy link
Member

Choose a reason for hiding this comment

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

How about encapsulating the listener stuff in a zabbixMockServer struct and record the received messages and errors there instead of utilizing channels to pass around data. We then perform the test, wait for the required data (with timeout) and then check the recorded internal data (and errors). This also allows to stick with require instead of reintroducing assert... What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Utilized zabbixMockServer and simplfied things.

@telegraf-tiger
Copy link
Contributor

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

@powersj powersj requested a review from srebhan August 16, 2024 13:51
@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 16, 2024
srebhan
srebhan approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good thanks @zak-pawel!

@srebhan srebhan merged commit e94f0c5 into influxdata:master Aug 22, 2024
27 checks passed
@github-actions github-actions bot added this to the v1.32.0 milestone Aug 22, 2024
@ccoVeille ccoVeille mentioned this pull request Sep 1, 2024
Open
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024
@zak-pawel @asaharn
chore(linters): Fix findings found by testifylint: go-require for…
0a2c9f5 
… `zabbix` (influxdata#15727)
@zak-pawel zak-pawel mentioned this pull request Nov 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srebhan srebhan srebhan approved these changes

@powersj powersj powersj approved these changes

Assignees

@srebhan srebhan

Labels
chore linter ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
v1.32.0
Development

Successfully merging this pull request may close these issues.
3 participants
@zak-pawel @powersj @srebhan