-
Notifications
You must be signed in to change notification settings - Fork 345
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
Add vet to tests #126
Add vet to tests #126
Conversation
9c12c32
to
21d16d2
Compare
Fixes #124 |
@@ -145,7 +145,7 @@ func TestLoadAllPlugins(t *testing.T) { | |||
} | |||
|
|||
if len(dsplugin.GetPodSpec().Containers) != 2 { | |||
t.Fatalf("JobPlugin should have 1 container, got 2", len(jobplugin.GetPodSpec().Containers)) | |||
t.Fatalf("JobPlugin should have 2 containers, got %d", len(jobplugin.GetPodSpec().Containers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 container, got %d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then why's it != 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait now I need to dblcheck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, ignore this comment.
Makefile
Outdated
# Vendor this someday | ||
INSTALL_GOLINT = go get -u github.com/golang/lint/golint | ||
GOLINT_FLAGS ?= -set_exit_status | ||
VET = go vet $(TEST_PKGS) && $(INSTALL_GOLINT) && golint $(GOLINT_FLAGS) $(TEST_PKGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably break this into 2 var..
VET
LINT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also add a gfmt check but now were entering into the whole we should break up the target b/c we're chaining too many things.
Makefile
Outdated
|
||
container: test | ||
container: test vet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is vet needed here?
test: cbuild vet
Adds `go vet` and `golint` to the `make test` task. Also makes several small, mostly cosmetic changes to bring code into compliance with sed style-checking tools. Signed-off-by: liz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Add vet to tests Signed-off-by: Jesse Hamilton [email protected]
Add vet to tests Signed-off-by: Jesse Hamilton [email protected] Signed-off-by: Jesse Hamilton [email protected]
Our tests should vet, lint, and otherwise verify the hygiene of our code before we allow it into master