Add GO_TESTFLAGS variable to append to test flags#352
Conversation
| if isGolang { | ||
| build.addDefinition("GO_BUILDFLAGS =%s", cfg.Variable("GO_BUILDFLAGS", defaultBuildFlags)) | ||
| build.addDefinition("GO_LDFLAGS =%s", cfg.Variable("GO_LDFLAGS", strings.TrimSpace(defaultLdFlags))) | ||
| build.addDefinition("GO_TESTFLAGS :=%s $(GO_TESTFLAGS)", cfg.Variable("GO_TESTFLAGS", "")) |
There was a problem hiding this comment.
It's using slightly different approach here for the assignment to accommodate opportunities to both set and append to the variable.
Given the following configuration:
variables:
GO_TESTFLAGS: '-short'
verbatim: |
foo:
go test $(GO_TESTFLAGS) pkg.example/fooA normal target call would render:
❯ make foo
go test -short pkg.example/fooIn order to append test flags one would do:
❯ GO_TESTFLAGS="-v -count=1" make foo
go test -short -v -count=1 pkg.example/fooIn order to override test flags one would do:
❯ make foo GO_TESTFLAGS=
go test pkg.example/foo(note, the new target foo is merely for representation purpose - the introduced variable is used in the main test command target).
If the proposed behavior is fine, I'd add a paragraph to describe it in README. It may be reasonable to also add the same for other variables - though this would backward incompatible with previous behavior, so I'm open for your suggestions.
There was a problem hiding this comment.
I checked in my workgroup; we don't see a strong reason for why this should not be allowed. (Maybe this will blow up somewhere because it does break backwards compatibility, but we're willing to risk that.) Please apply this scheme to all variables: and document accordingly.
There was a problem hiding this comment.
stanislav-zaprudskiy
requested review from
Nuckal777,
SuperSandro2000,
VoigtS,
majewsky and
wagnerd3
as code owners
| if isGolang { | ||
| build.addDefinition("GO_BUILDFLAGS =%s", cfg.Variable("GO_BUILDFLAGS", defaultBuildFlags)) | ||
| build.addDefinition("GO_LDFLAGS =%s", cfg.Variable("GO_LDFLAGS", strings.TrimSpace(defaultLdFlags))) | ||
| build.addDefinition("GO_TESTFLAGS :=%s $(GO_TESTFLAGS)", cfg.Variable("GO_TESTFLAGS", "")) |
There was a problem hiding this comment.
I checked in my workgroup; we don't see a strong reason for why this should not be allowed. (Maybe this will blow up somewhere because it does break backwards compatibility, but we're willing to risk that.) Please apply this scheme to all variables: and document accordingly.
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
The override mechanism proposed by @stanislav-zaprudskiy in #352 is problematic because --warn-undefined-variables makes noise about it: ```shellSession $ make Makefile:67: warning: undefined variable 'GO_BUILDFLAGS' Makefile:68: warning: undefined variable 'GO_LDFLAGS' Makefile:69: warning: undefined variable 'GO_TESTFLAGS' Makefile:70: warning: undefined variable 'GO_TESTENV' Makefile:71: warning: undefined variable 'GO_BUILDENV' env go build -mod vendor -ldflags '-s -w ... ``` However, through much experimentation, I discovered that the += operator behaves in the intended way, as documented in the new generated comment.
The override mechanism proposed by @stanislav-zaprudskiy in #352 is problematic because --warn-undefined-variables makes noise about it: ```shellSession $ make Makefile:67: warning: undefined variable 'GO_BUILDFLAGS' Makefile:68: warning: undefined variable 'GO_LDFLAGS' Makefile:69: warning: undefined variable 'GO_TESTFLAGS' Makefile:70: warning: undefined variable 'GO_TESTENV' Makefile:71: warning: undefined variable 'GO_BUILDENV' env go build -mod vendor -ldflags '-s -w ... ``` However, through much experimentation, I discovered that the += operator behaves in the intended way, as documented in the new generated comment.
The override mechanism proposed by @stanislav-zaprudskiy in #352 is problematic because --warn-undefined-variables makes noise about it: ```shellSession $ make Makefile:67: warning: undefined variable 'GO_BUILDFLAGS' Makefile:68: warning: undefined variable 'GO_LDFLAGS' Makefile:69: warning: undefined variable 'GO_TESTFLAGS' Makefile:70: warning: undefined variable 'GO_TESTENV' Makefile:71: warning: undefined variable 'GO_BUILDENV' env go build -mod vendor -ldflags '-s -w ... ``` However, through much experimentation, I discovered that the += operator behaves in the intended way, as documented in the new generated comment.
4 participants
The PR adds a new variable
GO_TESTFLAGSfor Makefile to manipulate additional flags to append to the test command. For example:would append the respective arguments to the test command after all other provided test flags (provided by
go-makefile-maker).This could be useful when one needs to alter default
go-makefile-makerbehavior, for example:-vand so on.
The appended variable should work for both
go testandginkgo- where it appends just before packages specification:See also #352 (comment) thread for additional considerations around the behavior.