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.

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

Run go vet and tests in all subpackages in CI #107

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Nov 8, 2022

go vet and go test without any package arguments runs only
on the go files in the repo root directory. It should run on all
non-vendored packages.

Fixes following go vet failure:

  • ./scylla_test.go:180:4: call to (*T).Fatal from a non-test goroutine

@zimnx zimnx requested a review from tnozicka November 8, 2022 09:29
@zimnx zimnx force-pushed the mz/fix-ci-tests-vet branch 2 times, most recently from 106363c to 2de9047 Compare November 8, 2022 16:14
scylla_test.go Outdated
@@ -167,6 +169,9 @@ func TestScyllaRandomConnPIcker(t *testing.T) {
})

t.Run("async access of max iterations", func(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

On a busy system timeout as low as this could flake. Is there a particular issue you've encountered that forced you to start addressing this? Would collecting the errors be enough to fix the vet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not just the vet, I noticed an issue and I fixed it, it's better to flake than hang infinitely. I increased the timeout to 5s which should be big enough even for busy system.

@tnozicka
Copy link
Member

nit: the first commit name misses : after UPSTREAM

`go vet` and `go test` without any package arguments runs only
on the go files in the repo root directory. It should run on all
non-vendored packages.
Fixes following go vet failure:
* ./scylla_test.go:180:4: call to (*T).Fatal from a non-test goroutine
@zimnx
Copy link
Collaborator Author

zimnx commented Nov 14, 2022

nit: the first commit name misses : after UPSTREAM

good catch, thanks. Fixed

@zimnx zimnx requested a review from tnozicka November 14, 2022 09:09
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/lgtm

@zimnx zimnx merged commit 2a9c55a into scylladb:master Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants