-
Notifications
You must be signed in to change notification settings - Fork 86
Makefile: add gofmt and govet to test #161
Conversation
|
ping @opencontainers/image-tools-maintainers |
Makefile
Outdated
| .PHONY: .gofmt .gotest | ||
|
|
||
| PACKAGES = $(shell go list ./... | grep -v /vendor/) | ||
| test: .gofmt .gotest |
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 would format be part of testing?
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.
I want to do fmt detection for the format, so I think it belongs to test, and I can also add gofmt commands in the makefile to make formatting adjustments.
Makefile
Outdated
| test: .gofmt .gotest | ||
|
|
||
| .gofmt: | ||
| OUT=$$(go fmt $(PACKAGES)); if test -n "$${OUT}"; then echo "$${OUT}" && exit 1; fi |
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.
Use gofmt -s -d. The output as you have it won't be very useful.
Makefile
Outdated
| test: .gofmt .govet .gotest | ||
|
|
||
| .gofmt: | ||
| OUT=$$(go fmt $(PACKAGES)); if test -n "$${OUT}"; then echo "$${OUT}" && exit 1; fi |
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.
This still isn't using gofmt -d -s. Please use the correct command.
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.
I tried the method you mentioned, but gofmt only supports absolute paths. My idea is to use pwd to get the absolute path of image-tools, but I don't know how to remove the vendor next .
Do you have any advice?
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.
how about the following?
FILES = $(shell find ./ -name *.go | grep -v vendor)
OUT=$$(gofmt -s -d $(FILES)); if test -n "$${OUT}"; then echo "$${OUT}" && exit 1; fi
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.
I think it's good, thanks.
Signed-off-by: zhouhao <[email protected]>
c49a241 to
c720509
Compare
|
ping @coolljt0725 @stevvooe |
|
reping @opencontainers/image-tools-maintainers |
|
Bump |
Makefile
Outdated
|
|
||
| UTDIRS = ./image/... | ||
| .gotest: | ||
| go test -v -race -cover $(UTDIRS) |
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 go test only cover UTDIRS = ./image/... ? the old way is to cover $(shell go list ./... | grep -v /vendor/)?
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.
? github.com/opencontainers/image-tools/cmd/oci-image-tool [no test files]
ok github.com/opencontainers/image-tools/image 1.120s coverage: 56.5% of statements
? github.com/opencontainers/image-tools/version [no test files]
Because only the image has the test file, this can improve efficiency.
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.
But I think it's better to keep the old way, because we can't make sure we will not add test file in other dir in future.
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.
Fine, updated.
Signed-off-by: zhouhao <[email protected]>
cddee97 to
3ace3ab
Compare
|
with this patch and I run Is this a expected output? |
Yes.The purpose is to detect all the |
|
IMO, we can just output the gofmt warning if there is and output nothing if there is no format warning |
|
I think it should be output, just like |
Signed-off-by: zhouhao [email protected]