Closed SaiedKazemi closed 2 years ago
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
caller.go | 3 | 4 | 75.0% | ||
hopannotation/hopannotation.go | 2 | 3 | 66.67% | ||
<!-- | Total: | 34 | 36 | 94.44% | --> |
Totals | |
---|---|
Change from base Build 398: | 13.8% |
Covered Lines: | 515 |
Relevant Lines: | 533 |
caller.go, line 42 at r1 (raw file):
I still find it peculiar to name string variables `errFoo` when they are not Error types.
Because these are error messages, they are prefixed by "err". I don't think all variable names that start with "err" should only be of type "error".
Anyway, I changed them to type error to avoid any potential confusion.
caller_test.go, line 74 at r1 (raw file):
When I see sleeps in tests, I worry that it's only a matter of time before a race is successful. I ran `while go test -v -count=1 ./ ; do sleep .1 ; done` until it failed with the error below. ``` caller_test.go:74: eventsocket server goroutine still running --- FAIL: TestMainFunc (2.98s) ``` Is there a way to make the server setup and shutdown deterministic? (doesn't have to be this PR, but I imagine this is something you care about -- and maybe it's not practically possible -- but we also don't want the test to fail unless there's a real problem -- so perhaps increasing the constant delay (which doesn't feel satisfying, but might get the job done).
I agree that it's not desirable to have sleeps but in this particular case I don't think we have much choice because Serve() does not return unless its context is cancelled.
The test cancels the context after one second and we now wait two seconds for Serve() to return. This should prevent the error that you reported earlier.
Thanks a lot for your review @stephen-soltesz. Because I made some minor changes based on your review comments, could you please take another look?
ipcache/staticcheck.conf, line 1 at r2 (raw file):
What uses this file?
As part of making sure my changes are good, I run staticcheck
. This particular file in the ipcache
directory is meant to prevent staticcheck
from complaining that nil is not a valid context value.
$ cd ipcache
$ mv staticcheck.conf staticcheck.conf.KEEP
$ staticcheck .
ipcache_test.go:42:27: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (SA1012)
$ sed -n '40,45p' ipcachetest.go
func TestNew(t *testing.T) {
// nolint:staticcheck
if , err := ipcache.New(nil, nil, ipcache.Config{}); err == nil {
t.Error("FetchTrace() = nil, want error")
}
if _, err := ipcache.New(context.TODO(), &fakeTracer{}, ipcache.Config{}); err == nil {
$ mv staticcheck.conf.KEEP staticcheck.conf
$ staticcheck .
$ echo $?
0
Thanks again. Please see my explanation of staticcheck.conf
in the other comment.
ipcache/staticcheck.conf, line 1 at r2 (raw file):
Should running staticcheck be added to the standard CI steps for traceroute-caller? That way we can guarantee it's always executed.
Two things come to mind with respect to adding staticcheck
only to traceroute-caller
standard CI steps:
It's better to have a consistent set of checks across all our Go repos as opposed to running some checks against one repo and other checks against another repo.
Installing staticcheck
for repos running a version of Go earlier than 1.17 is a bit tricky. Beginning with Go 1.17, it can be installed simply with go install honnef.co/go/tools/cmd/staticcheck@latest
. traceroute-caller
is currently using Go 1.16 and I am not sure what versions of Go other repos use.
That said, I am in favor or having more code checks and tests in our standard CI steps for all our repos and using the same version of Go.
This commit adds package unit testing to traceroute-caller after its recent refactor. Code coverage is as follows:
Only 8 lines from the entire traceroute source code where it's almost impossible to force a failure are not covered.
Changes were tested locally:
This change is![Reviewable](https://reviewable.io/review_button.svg)