feat: add unit tests for ferrogw-cli command behavior - #12
Closed
VedantMadane wants to merge 3 commits into
Closed
Conversation
This PR adds dedicated unit tests for the ferrogw-cli tool by: 1. Refactoring cmd/ferrogw-cli/main.go to extract command dispatch into a testable execute function. 2. Adding cmd/ferrogw-cli/main_test.go to validate behavior for help, version, plugins, and unknown commands.
There was a problem hiding this comment.
Pull request overview
This PR makes the ferrogw-cli entry point testable by extracting command dispatch into a function that returns an exit code, and adds unit tests to prevent regressions in user-facing CLI behavior.
Changes:
- Refactor
cmd/ferrogw-cli/main.goto route throughexecute(args, stdout, stderr) intand return exit codes instead of callingos.Exitthroughout. - Add
cmd/ferrogw-cli/main_test.gotable-driven tests for core command dispatch paths (usage/help, version, plugins, unknown command).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/ferrogw-cli/main.go | Refactors CLI dispatch to a testable execute(...) int function with injected writers and explicit exit codes. |
| cmd/ferrogw-cli/main_test.go | Adds unit tests validating exit codes and stdout/stderr output patterns for several commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expectedStdout: "Usage:", | ||
| expectedStderr: "", | ||
| }, | ||
| { |
There was a problem hiding this comment.
Tests cover help and -h, but the issue/acceptance criteria also calls out --help. Add a test case for ferrogw-cli --help to ensure that flag remains supported and prints usage with exit code 0.
Suggested change
| { | |
| { | |
| name: "--help flag", | |
| args: []string{"ferrogw-cli", "--help"}, | |
| expectedExit: 0, | |
| expectedStdout: "Usage:", | |
| expectedStderr: "", | |
| }, | |
| { |
Comment on lines
+48
to
+53
| name: "plugins command", | ||
| args: []string{"ferrogw-cli", "plugins"}, | ||
| expectedExit: 0, | ||
| expectedStdout: "Registered plugins:", | ||
| expectedStderr: "", | ||
| }, |
There was a problem hiding this comment.
The plugins test currently only asserts the header text. To better validate “lists registered plugins”, also assert that at least one known built-in plugin name (e.g. response-cache, request-logger, etc.) appears in stdout.
MitulShah1
requested changes
Mar 2, 2026
MitulShah1
left a comment
MitulShah1
left a comment
Contributor
There was a problem hiding this comment.
Thanks just need to fix minor changes
Made-with: Cursor
Made-with: Cursor
Contributor
|
Hey Thanks for contribution but i think this issue is not anymore valid. i've added few new issues please have a look. |
MitulShah1
pushed a commit
that referenced
this pull request
Jun 15, 2026
…cording correctness (#199) * fix(gateway): wire streaming circuit breaker at stream completion (#136) Initialize circuit breakers via ensureCircuitBreakersLocked from New, ReloadConfig, getStrategy, and RouteStream so streaming-only traffic gets breaker protection without calling getStrategy. Stop recording breaker success when CompleteStream opens; defer success/failure to streamwrap.Meter via CircuitBreakerOutcome, skipping client disconnects and rate limits. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix(gateway): do not trip stream circuit breaker on client deadline PR #7 defers streaming circuit-breaker outcomes to stream completion via streamwrap.Meter. Client context deadlines were counted as provider failures, opening the breaker after enough timed-out streams even when the provider was healthy. Skip context.DeadlineExceeded alongside context.Canceled in recordStreamCircuitBreakerOutcome, matching fallback retry semantics. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix(streamwrap): preserve provider errors when client cancels mid-stream When a client disconnects during stream forwarding, do not overwrite an already-captured provider error with ctx.Err(). While draining the upstream channel after cancel, also adopt error chunks so circuit breaker outcomes still record genuine provider failures. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix: skip non-streaming targets before circuit-breaker wrap in RouteStream When a target has circuit_breaker configured, cbProvider implements CompleteStream even if the underlying provider does not. Check the underlying provider's StreamProvider support before wrapping so fallback/conditional/load-balance routing can advance to the next streaming-capable target. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix: address stream circuit-breaker review feedback - Do not record startup stream failures for client cancellation/deadlines - Skip open circuit-breaker targets during streaming fallback selection - Preserve ErrCircuitOpen when the only compatible target is open Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix(gateway): do not trip non-stream circuit breaker on client deadline Apply shouldRecordCircuitBreakerFailure to cbProvider.Complete so context.Canceled and context.DeadlineExceeded are excluded alongside rate limits, matching the streaming path fixed in PR #12. Add TestGateway_Route_ClientDeadlineDoesNotTripCircuit. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> * fix: count provider-side timeouts as circuit breaker failures Only suppress context.Canceled and context.DeadlineExceeded when the request context is already done (ctx.Err() != nil). Provider SDK or HTTP client timeouts that surface as DeadlineExceeded while the caller context is still active now trip the circuit breaker as intended. Adds unit and integration tests distinguishing client cancellation from provider/upstream timeouts for both Route and RouteStream paths. Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com> --------- Co-authored-by: Rachit Gandhi <Rachit-Gandhi@users.noreply.github.com>
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
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.
Fixes #11. This PR refactors the CLI entry point to be testable and adds comprehensive unit tests for command dispatch.