fix(test): replace os.Args guard with cobra.ExactArgs(3) to prevent panic#391
Open
gyanranjanpanda wants to merge 1 commit into
Open
Conversation
gyanranjanpanda
requested review from
Harsh4902,
lbroudoux and
yada
as code owners
|
Welcome to the Microcks community! 💖 Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly. Hope you have a great time there! |
…anic The test command validated argument count using len(os.Args) < 4, which counts global flags like --microcksURL and --keycloakClientId as part of the argument vector. When a user invokes the command with global flags but omits the <runner> positional argument (the typical CI pattern from the README), os.Args has enough entries to pass the guard, but cobra's parsed args slice only has 2 elements. The subsequent access to args[2] causes a runtime panic with a stack trace instead of a clean error. Fix: - Use cobra.ExactArgs(3) on the command definition — idiomatic and handles count validation before Run is ever called. - Remove three now-dead HasPrefix guards that were unreachable since cobra strips flags before handing args to Run. - Remove a redundant var testResultID string pre-declaration that was immediately shadowed by :=. - Fix the runner error message which listed SOAP (not a valid value) instead of SOAP_HTTP. - Update Use string to show positional args in help output. Fixes microcks#390 Signed-off-by: gyanranjanpanda <sanupanda141@gmail.com>
gyanranjanpanda
force-pushed
the
fix/test-args-panic-with-global-flags
branch
from
2fb3dc8 to
2c553b9
Compare
Author
|
could @lbroudoux review this pr |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #390
The
testcommand was guarding positional argument count withlen(os.Args) < 4. This check includes global persistent flags (--microcksURL,--keycloakClientId,--keycloakClientSecret) in the count. When the command is invoked with global flags but the<runner>argument is omitted - the standard CI invocation pattern shown in the README -os.Argshas enough elements to pass the guard, but Cobra'sargsslice (flags stripped) has only 2 elements.args[2]then causes a runtime panic with a stack trace instead of a clean error message.Changes
cmd/test.go: Usecobra.ExactArgs(3)on the command definition (idiomatic Cobra). The framework validates arg count and prints usage beforeRunis invoked - no manual guard needed.cmd/test.go: UpdateUsefield to"test <apiName:apiVersion> <testEndpoint> <runner>"so--helpoutput shows the expected signature.cmd/test.go: Remove three now-deadstrings.HasPrefix(x, "-")guards - Cobra strips all flags before passingargstoRun, so none of those conditions can ever be true.cmd/test.go: Remove redundantvar testResultID stringpre-declaration immediately shadowed by:=.cmd/test.go: Fix runner validation error message: listedSOAP(not inrunnerChoices) instead ofSOAP_HTTP.cmd/test_test.go: Add regression test that simulates the CI invocation pattern (global flags + missing runner) and asserts exit code 1 with nopanic:in output.Before / After
Before (with global flags, missing
<runner>):After:
Testing
go test ./cmd/... -run TestTestCommandMissingRunnerWithGlobalFlagsDoesNotPanic -v