chore(linters): Fix findings found by testifylint: go-require forinstrumental and parsers/processors

[zak-pawel]

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 parser/processors tests and instrumental_test.go. While fixing the parser/processors tests is quite straightforward, it seems that the approach to testing the simulation of the TCP Server's response in a separate goroutine requires discussion – because there are quite a few such tests in Telegraf’s test code, and it would be great to have a consistent approach in future PRs.

Here are the findings that this PR addresses:

plugins/outputs/instrumental/instrumental_test.go:92:3        testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:94:3        testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:99:3        testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:100:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:102:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:103:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:105:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:108:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:109:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:111:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:112:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:115:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:117:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:122:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:123:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:125:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:126:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:128:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:131:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:132:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:135:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:136:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:139:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:140:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:143:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:144:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/outputs/instrumental/instrumental_test.go:147:3       testifylint  go-require: require must only be used in the goroutine running the test function
plugins/parsers/influx/influx_upstream/parser_test.go:1011:3  testifylint  go-require: require must only be used in the goroutine running the test function
plugins/parsers/influx/parser_test.go:978:3                   testifylint  go-require: require must only be used in the goroutine running the test function
plugins/processors/ifname/ifname_test.go:129:4                testifylint  go-require: require must only be used in the goroutine running the test function
plugins/processors/ifname/ifname_test.go:130:4                testifylint  go-require: require must only be used in the goroutine running the test function
plugins/processors/ifname/ifname_test.go:131:4                testifylint  go-require: require must only be used in the goroutine running the test function
plugins/processors/reverse_dns/rdnscache_test.go:55:3         testifylint  go-require: require must only be used in the goroutine running the test function
plugins/processors/reverse_dns/rdnscache_test.go:61:3         testifylint  go-require: require must only be used in the goroutine running the test function

Checklist

• No AI generated code was used in this PR

[zak-pawel]

I see the following options for handling multiple errors in the code simulating the TCP Server in a separate goroutine:

1. (presented in the first commit of this PR): Introducing buffered channels (unbuffered channels are not the best idea here because they block until a receiver receives the sent message) for transmitting potential errors and exiting (return) from the goroutine after each error. Since at least one error (which should be nil) is always sent, and thanks to the fact that the error receiver reads them in a deferred function, we can get rid of WaitGroup (synchronization will happen after sending at least one error).
   Cons: Non-trivial code, prone to mistakes, no information about which line in the goroutine caused the error, need to remember to implement various defers.
2. From point 1, channels can be removed, and WaitGroup can be restored (as it was before). Instead of sending errors to a channel, they can be logged using the t.Error(f) function, which marks the test as failed but doesn’t immediately stop it (so goroutine should be exited manually after every problem (return)).
   Pros: fewer defers, no channels which need to be handled carefully, information about the line where the problem occurred (in the log)
3. Similar to point 2, but using assert.XXX instead of t.Error(f).
   Pros: less boilerplate than in points 1 and 2 (fewer if statements – assert functions check the failure condition themselves).
   Cons: it's assert, which we've been phasing out of the test code over the past few years.

@srebhan @DStrand1
What do you think? Or maybe you have some other suggestions?

[srebhan]

@zak-pawel is there any reason to not collect errors in the TCPServer instance? We then can simply use require outside of the go-routine without channels and other fu...

[zak-pawel]

@srebhan
If I understand you correctly, you are proposing to wrap TCPServer in a structure that would store potential errors and handle them after the goroutine has finished, right?

I think I prefer option number 2 (t.Error(f)), where there is no need for channels, wrappers, and the exact line where the error occurred will be known.

[srebhan]

@zak-pawel exactly, if we do store the errors we can then also check the error messages, types etc. But option 2 also works for me, just make sure the test doesn't sit and wait for the test-timeout (10min) to be hit...

[zak-pawel]

@srebhan Please, check t.Error(f) version.

[telegraf-tiger]

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 change the Telegraf binary size

📦 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

[srebhan]

The code is ok even though I don't like the channel stuff in the influx parser... But it's better than breaking things. IMO we are still better off if we are implementing mock-server stuff for instrumental and check the errors/metrics outside of the go-routine.

Anyway, thanks for your work @zak-pawel!

[zak-pawel]

IMO we are still better off if we are implementing mock-server stuff for instrumental and check the errors/metrics outside of the go-routine.

@srebhan I would like to understand why you would prefer it that way.
The Go convention (and common sense 😉) suggest that if someone returns an error to us (or a situation occurs that requires the creation of this error), we check it and decide what to do next. Because in the vast majority of situations, it makes no sense to continue the execution when an error occurs.
So far the error was checked by require (which should not be run in goroutines), and in your proposal with the mock-server we would still have to do it with ifs (just like in my proposal).

The only difference is the place where we "mark" the test as failed. If we do it outside the goroutine, we will lose very valuable information about exactly which line the error occurred - we will have to guess it based on the content of the message...

[srebhan]

Well sometimes we want to test that an error occurs e.g. if providing bad configuration settings etc. So my take is, returning an error via the normal communication means would be the best option (e.g. return a HTTP error code on http connections).

Alternatively, implement a mockup server that can trace issues during communication (as I proposed), this way we can test expected error cases as well as comparing results on the server side to what we expect ends up on the server in the unit-test. This also adds the flexibility to adapt metrics in the test cases as input and expected output are in one place.

The least preferable option in my view is to check the result directly on the server side as it creates a rigid structure where the input needs to be known in the unit-test function as-well-as in the mocked server side. Furthermore, it will provide no means to stepwise test code and react on the unit-test side without a lot of synchronization-fu.

But just my 2 cent...