CLI Inegration Test #648
CLI Inegration Test #648
Conversation
|
Warning Rate limit exceeded@piyumaldk has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a YAML-configured CLI integration test framework: Godog suite, test configuration loader, infrastructure manager (build/start/wait/teardown), per-test state and reporter, step/assertion implementations, feature files, test resources, Makefile/script updates, and supporting unit-test additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Suite as Godog Suite
participant Reporter as Test Reporter
participant Infra as InfrastructureManager
participant CLI as CLI Binary
participant Docker as Docker Compose
Runner->>Suite: invoke TestFeatures()
Suite->>Reporter: NewTestReporter & Setup
Suite->>Infra: SetupInfrastructure(required infra)
Infra->>Reporter: LogPhase1(build_cli / build_images)
Infra->>Docker: build gateway images / docker compose up
Infra->>CLI: build CLI binary
Infra->>Infra: waitForGatewayHealth / waitForMCPServer
Suite->>Suite: InitializeScenario() / StartTest
Suite->>CLI: ExecuteCLI(args) via TestState
CLI-->>Suite: stdout / stderr / exit code
Suite->>Reporter: EndTest(result) (write per-test log)
Suite->>Infra: Teardown() -> docker compose down
Suite->>Reporter: PrintSummary()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @cli/it/config.go:
- Around line 138-154: GetRequiredInfrastructure builds the result slice from a
map which yields nondeterministic order; change
TestConfig.GetRequiredInfrastructure to return a deterministic order by
collecting unique InfrastructureID values from GetEnabledTests (from
test.Requires) then either sort the result slice (e.g., by string or numeric
value of InfrastructureID) or reorder it according to a predefined dependency
sequence (ensuring gateway images come before gateway stack); update the
function to perform that deterministic ordering before returning the slice.
In @cli/it/features/gateway/api.feature:
- Around line 34-43: The scenario named GW-API-002 ("List APIs when empty") is
misleading because it only checks command success and the API deployed by
GW-API-001 ("List APIs") will still exist; either rename GW-API-002 to "List
APIs" to reflect that it only asserts success, or change GW-API-002 to assert an
empty list by running the same "ap gateway api list" invocation and adding a
check that the output contains no APIs (and ensure the test removes the resource
applied from resources/gateway/sample-api.yaml before running GW-API-002, e.g.,
add a teardown step to delete that API or run a cleanup command prior to the
empty-list assertion).
In @cli/it/reporter.go:
- Around line 349-354: GetResults currently returns the internal slice r.results
directly, allowing callers to mutate it and cause data races; fix by returning a
copy made while holding the lock (e.g., allocate a new slice and copy r.results
into it) so callers get an independent slice; update the GetResults method on
TestReporter (which uses r.mu and r.results) to create and return that copied
slice before releasing the lock.
- Around line 200-203: The call to os.WriteFile in reporter.go ignores its
returned error; capture the error (e.g., err := os.WriteFile(...)) and, if
non-nil, emit a non-fatal warning (using the package logger, log.Printf, or
fmt.Fprintf(os.Stderr, ...)) so logging failures are visible without failing
tests; update the os.WriteFile call site to check err and log a descriptive
warning including the error value.
In @cli/it/steps/cli_steps.go:
- Around line 255-268: The code uses nonexistent filepath.HasPrefix causing a
compile error; replace the check filepath.HasPrefix(filePath,
"resources/gateway/") with strings.HasPrefix(filePath, "resources/gateway/") and
add the strings package to the imports, leaving the rest of the logic (filename
:= filepath.Base(filePath); absPath = resources.GetResourcePath(filename); and
the final s.state.ExecuteCLI("gateway", "apply", "-f", absPath")) unchanged.
- Around line 162-173: The BuildGatewayWithManifest function currently swallows
errors from filepath.Abs(manifestPath) and silently falls back to
resources.GetPolicyManifestPath(), which can mask test misconfiguration; change
the behavior to return the absPath error instead of overwriting it—i.e., in
BuildGatewayWithManifest, when filepath.Abs returns a non-nil err, return that
error (or wrap it with context) immediately so s.state.ExecuteCLI is not invoked
with an unintended manifest path; update any callers/tests that expected the old
fallback if necessary and keep references to filepath.Abs,
resources.GetPolicyManifestPath, BuildGatewayWithManifest, and
s.state.ExecuteCLI to locate the change.
- Around line 240-249: The ResetConfiguration function currently builds a
hardcoded config path using os.Getenv("HOME"); replace this with a
cross-platform approach: call os.UserHomeDir() to get the user's home directory
(handle and return any error), construct the config directory and file paths
using filepath.Join (use configDir := filepath.Join(home, ".wso2ap") and
configFile := filepath.Join(configDir, "config.yaml")), ensure the config
directory exists (os.MkdirAll with proper permissions) before writing, and then
write the emptyConfig to configFile, returning any write errors from
ResetConfiguration.
In @cli/it/suite_test.go:
- Around line 62-67: The call to filepath.Abs("logs") ignores its error; update
the suite_test.go setup so you capture both return values (logsDir, err :=
filepath.Abs("logs")) and check err immediately, calling t.Fatalf (or t.Fatal)
with the error if non-nil before creating NewTestReporter and calling
testReporter.Setup(); this ensures failures constructing the absolute path are
not silently ignored.
In @cli/it/test-config.yaml:
- Around line 31-36: The compose_file path from the config is ignored and a
hardcoded path is used via filepath.Abs at setup.go, so update the startup logic
to read the ComposeFile (and startup_timeout, health_check_interval) from the
loaded config, resolve that ComposeFile path relative to the config file's
directory (not CWD) and pass the resolved path plus the timeout/interval values
into InfrastructureManager (or the constructor NewInfrastructureManager / the
function that currently calls filepath.Abs). Replace the hardcoded
../../gateway/it/docker-compose.test.yaml usage with the config-derived,
resolved path and ensure InfrastructureManager fields or params (ComposeFile,
StartupTimeout, HealthCheckInterval) are populated from the config.
In @cli/src/internal/gateway/resources.go:
- Around line 28-31: GetResourceHandler currently matches resource kinds using
exact string constants (e.g., ResourceKindMCP = "Mcp"), which breaks manifests
using different casing; update GetResourceHandler to perform case-insensitive
matching by normalizing the incoming kind (e.g., strings.ToLower or
strings.EqualFold) before comparing against normalized constants (or normalize
keys in any resource-kind map), and apply the same normalization approach to the
other resource-kind comparisons/lookup in the file (the places where
ResourceKindRestAPI/ResourceKindMCP are mapped or checked) so manifests like
"mcp" or "MCP" route correctly.
In @cli/src/scripts/run-tests-with-summary.sh:
- Around line 5-7: The cd to "$SCRIPT_DIR/.." may fail and the script would
continue in the wrong directory; after computing SCRIPT_DIR (the SCRIPT_DIR
variable) ensure the directory change succeeds by checking the cd exit status
and exiting with a non-zero code if it fails (e.g., test the result of the cd
"$SCRIPT_DIR/.." command and call exit 1 or use a conditional || exit), so
subsequent commands run only when the working directory change succeeded.
In @cli/src/test/unit/cmd_naming_convention_test.go:
- Around line 115-121: The loop over foundCommands can panic when cmd.Use is
empty because strings.Fields(cmd.Use)[0] assumes at least one field; change the
logic in the loop that builds cmdName (used in the block iterating
foundCommands) to safely handle empty or whitespace-only cmd.Use by first
calling fields := strings.Fields(cmd.Use) and skipping or using a fallback
(e.g., continue the loop or set cmdName = "") when len(fields) == 0, then
proceed to append and compare using cmdName as before.
🧹 Nitpick comments (23)
cli/src/test/unit/flags_test.go (1)
67-67: Consider removing hardcoded filename and line number.Hardcoding
"flags_test.go"and67in the log statement is error-prone—the line number will become stale as the code evolves. Go'st.Logfalready includes location information in test output. If explicit location is required, consider usingruntime.Caller()instead.♻️ Simplified logging without hardcoded values
- t.Logf("%s:%d: ✓ All %d flag values are unique", "flags_test.go", 67, len(allFlagValues)) + t.Logf("✓ All %d flag values are unique", len(allFlagValues))cli/src/scripts/run-tests-with-summary.sh (1)
19-22: Consider adding warnings when log formatting fails.The
|| trueoperators on Lines 20 and 22 prevent the script from failing if log formatting encounters errors (e.g., disk space, permissions). While this keeps the script resilient for non-critical formatting, it silently hides potential issues.💡 Optional improvement to add warnings
# Improve readability by inserting a visual divider before each test run marker -awk 'BEGIN{first=1} /^=== RUN/ { if(!first) print ""; if(!first) print "--------------------------------------------------"; print ""; print; first=0; next } {print}' test/unit/logs/test-results.log > test/unit/logs/test-results.formatted.log || true +awk 'BEGIN{first=1} /^=== RUN/ { if(!first) print ""; if(!first) print "--------------------------------------------------"; print ""; print; first=0; next } {print}' test/unit/logs/test-results.log > test/unit/logs/test-results.formatted.log || echo "⚠ Warning: Failed to format test logs" # Replace original log with formatted version -mv test/unit/logs/test-results.formatted.log test/unit/logs/test-results.log || true +mv test/unit/logs/test-results.formatted.log test/unit/logs/test-results.log || echo "⚠ Warning: Failed to replace log with formatted version"cli/it/README.md (1)
40-49: Add language specification to fenced code block.The directory structure code block is missing a language identifier, which helps with consistent rendering and accessibility.
📝 Suggested fix
-``` +```text cli/it/ ├── test-config.yaml # Enable/disable tests ├── features/ # Gherkin feature files │ └── gateway/ ├── steps/ # Step definitions ├── resources/ # Test resources │ └── gateway/ └── logs/ # Test logs (git-ignored)</details> </blockquote></details> <details> <summary>cli/src/test/unit/cmd_naming_convention_test.go (1)</summary><blockquote> `186-188`: **Backtick (raw) string literals are not handled.** The code uses `strings.Trim(lit.Value, "\"")` which only handles double-quoted strings. Go also supports raw string literals with backticks (e.g., `` `command` ``), which would not be trimmed correctly. <details> <summary>♻️ Proposed fix</summary> ```diff if lit, ok := vs.Values[i].(*ast.BasicLit); ok && lit.Kind == token.STRING { - constMap[name.Name] = strings.Trim(lit.Value, "\"") + constMap[name.Name] = strings.Trim(lit.Value, "\"`") }Apply the same fix at line 233:
if val, ok := kv.Value.(*ast.BasicLit); ok && val.Kind == token.STRING { - useValue := strings.Trim(val.Value, "\"") + useValue := strings.Trim(val.Value, "\"`")cli/it/features/gateway/build.feature (1)
34-35: Hardcoded image tag may become stale.The
gateway-builder:0.3.0-SNAPSHOTimage tag is hardcoded in the feature file. Consider parameterizing this value through test configuration or environment variables to avoid updating multiple feature files when versions change.Also applies to: 39-41
cli/it/features/gateway/apply.feature (1)
24-60: Stabilize negative assertions (stderr vs stdout + OS-specific file errors) to reduce IT flakiness.
- Line 52:
"no such file"may differ by OS/runtime ("no such file or directory", Windows wording, etc.). If your steps can assert stderr, prefer that; otherwise consider matching a more stable substring (e.g.,"no such file"OR"cannot find"), or implementing a dedicated step for “file not found”.- If scenarios share CLI config/state across runs, consider an explicit “reset CLI configuration” step in
Background:(or in the scenarios) to avoid cross-scenario interference.cli/it/features/gateway/management.feature (1)
31-78: Add a couple assertions to avoid “always green” scenarios (duplicate overwrite + empty list).
- Duplicate gateway case (Line 52-59): if overwrite semantics matter, assert an observable outcome (e.g.,
gateway listcontains exactly one entry for that name, or output contains “updated/overwritten” if available).- “List gateways when empty” (Line 72-78): consider asserting either “No gateways” or an empty table header to ensure the command didn’t silently change output/format in an incompatible way.
- “invalid server URL accepted” (Line 47-49): if this is intentionally unsupported behavior, fine; otherwise consider flipping this to a failing case once URL validation is introduced to avoid locking in the current lax behavior.
Also applies to: 52-59
cli/it/INTEGRATION-TESTS.md (1)
1-691: Consider adding language specifiers to fenced code blocks.The documentation is comprehensive and well-structured. To improve markdown rendering and accessibility, consider adding language specifiers to fenced code blocks at lines 9, 43, 92, 395, 420, 517, and 571 (e.g.,
text,plaintext, orconsolefor example outputs).Example fix for line 9
-``` +```text ┌─────────────────────────────────────────────────────────────────────────────┐ │ CLI Integration Test Suite │cli/it/Makefile (1)
60-60: Document Docker Compose V2 requirement.Line 60 uses
docker compose(space, not hyphen), which requires Docker Compose V2. The prerequisites section in INTEGRATION-TESTS.md mentions Docker but doesn't specify the version requirement.cli/it/state.go (4)
80-118: Consider logging errors from temp directory cleanup.Line 87 silently ignores errors from
os.RemoveAll. While this won't break functionality (a new temp directory is created regardless), suppressed errors could indicate permissions issues or locked files that merit investigation.Suggested improvement
// Clean up previous temp directory if s.TempDir != "" { - os.RemoveAll(s.TempDir) + if err := os.RemoveAll(s.TempDir); err != nil { + // Log but don't fail - we'll create a new temp dir anyway + fmt.Fprintf(os.Stderr, "Warning: failed to remove temp directory %s: %v\n", s.TempDir, err) + } }
120-132: Consider logging errors from cleanup operations.Line 129 silently ignores errors from
os.RemoveAll, which could leave temp directories behind on cleanup failures. While not critical, this affects resource cleanup reliability.Suggested improvement
if s.TempDir != "" { - os.RemoveAll(s.TempDir) + if err := os.RemoveAll(s.TempDir); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to cleanup temp directory %s: %v\n", s.TempDir, err) + } s.TempDir = "" }
166-172: Consider using distinct exit codes for different error types.Lines 166-172 set
LastExitCodeto -1 for non-exit errors (e.g., context timeout, spawn failure), conflating different failure modes. This could make debugging harder when distinguishing between command failures and infrastructure issues.Alternative approach
Consider using distinct exit codes or storing the error separately:
+ s.LastError = err if exitErr, ok := err.(*exec.ExitError); ok { s.LastExitCode = exitErr.ExitCode() - } else if err != nil { - s.LastExitCode = -1 + } else if ctx.Err() == context.DeadlineExceeded { + s.LastExitCode = 124 // Standard timeout exit code + } else if err != nil { + s.LastExitCode = 127 // Command not executable/found } else { s.LastExitCode = 0 }
145-146: Consider making the CLI execution timeout configurable.The 30-second timeout is hardcoded. Some CLI operations (e.g., building images, applying large configurations) might legitimately require longer execution times.
Suggested improvement
Add a configurable timeout to TestState:
type TestState struct { + // CLI execution state + CLITimeout time.Duration // Default to 30s if zero CLIBinaryPath stringThen in ExecuteCLI:
+ timeout := s.CLITimeout + if timeout == 0 { + timeout = 30 * time.Second + } - ctx, cancel := context.WithTimeout(s.ctx, 30*time.Second) + ctx, cancel := context.WithTimeout(s.ctx, timeout)cli/it/setup.go (2)
34-49: Duplicate constants withresources/values.go.
DefaultStartupTimeoutandHealthCheckIntervalare defined here withtime.Durationtypes (120s, 5s), whileresources/values.godefines them as plain integers (120, 5). This duplication can lead to inconsistencies if updated independently.Consider consolidating these constants in one location, or explicitly importing from
resourcesand converting totime.Durationwhere needed.
176-184: Long-running build with no timeout or progress streaming.The
make build-coveragecommand can take several minutes but has no explicit timeout. The output is only captured after completion, so there's no progress indication for long builds. Consider:
- Adding a build-specific timeout
- Streaming output for visibility during long builds
cli/it/resources/values.go (2)
117-122:runtime.Callerreturn values not validated.The function ignores the
okboolean return value fromruntime.Caller(0). While this rarely fails, defensive coding would handle the error case.♻️ Proposed fix
func GetResourcePath(filename string) string { - _, currentFile, _, _ := runtime.Caller(0) + _, currentFile, _, ok := runtime.Caller(0) + if !ok { + // Fallback to relative path if runtime.Caller fails + return filepath.Join("resources", "gateway", filename) + } resourcesDir := filepath.Dir(currentFile) return filepath.Join(resourcesDir, "gateway", filename) }
45-55: Consider usingtime.Durationfor timeout constants.Timeout constants are defined as plain integers requiring callers to multiply by
time.Second. Usingtime.Durationdirectly (like insetup.go) would be type-safe and self-documenting.cli/it/suite_test.go (3)
163-170: Test ID extraction uses first tag only.The logic takes the first tag starting with
@as the test ID. If scenarios have multiple tags, subsequent ones are ignored. This is acceptable if documented, but consider adding a specific prefix convention (e.g.,@TEST-) for clarity.
355-358: Empty step implementation relies on implicit isolation.The
noGatewayIsConfiguredstep is a no-op, relying on isolation provided elsewhere. Consider adding an explicit verification that the gateway list is empty to catch isolation failures.♻️ Proposed fix
func noGatewayIsConfigured() error { - // This is handled by the isolated config directory - return nil + // Verify no gateways exist in current config + if err := cliSteps.RunGatewayList(); err != nil { + return err + } + // Check that output indicates no gateways + output := testState.GetStdout() + if strings.Contains(output, "NAME") && !strings.Contains(output, "No gateways") { + return fmt.Errorf("expected no gateways configured, but found: %s", output) + } + return nil }
380-382: Hardcoded URL should use constant.The MCP endpoint URL is hardcoded. Use
resources.MCPServerMCPEndpointfor consistency with other endpoint references.♻️ Proposed fix
+import "github.com/wso2/api-platform/cli/it/resources" + func iGenerateMCPConfigToOutput(output string) error { - return cliSteps.GenerateMCPConfig("http://localhost:3001/mcp", output) + return cliSteps.GenerateMCPConfig(resources.MCPServerMCPEndpoint, output) }cli/it/config.go (1)
91-136: Consider extracting test group iteration.Both
GetEnabledTestsandGetAllTestsiterate over the same test groups. A helper method could reduce duplication.cli/it/steps/assert_steps.go (2)
246-254: Method name suggests regex but uses substring match.
OutputShouldMatchPatternimplies regex matching but usesstrings.Contains. Consider renaming toOutputShouldContainPatternor implementing actual regex support.♻️ Option 1: Rename for clarity
-// OutputShouldMatchPattern asserts output matches a pattern -func (s *AssertSteps) OutputShouldMatchPattern(pattern string) error { +// OutputShouldContainText asserts output contains the text +func (s *AssertSteps) OutputShouldContainText(text string) error { output := s.state.GetCombinedOutput() - // Simple pattern matching - could be enhanced with regex - if !strings.Contains(output, pattern) { - return fmt.Errorf("expected output to match pattern %q, got:\n%s", pattern, output) + if !strings.Contains(output, text) { + return fmt.Errorf("expected output to contain %q, got:\n%s", text, output) } return nil }♻️ Option 2: Implement regex support
+import "regexp" + // OutputShouldMatchPattern asserts output matches a pattern func (s *AssertSteps) OutputShouldMatchPattern(pattern string) error { output := s.state.GetCombinedOutput() - // Simple pattern matching - could be enhanced with regex - if !strings.Contains(output, pattern) { + re, err := regexp.Compile(pattern) + if err != nil { + return fmt.Errorf("invalid regex pattern %q: %w", pattern, err) + } + if !re.MatchString(output) { return fmt.Errorf("expected output to match pattern %q, got:\n%s", pattern, output) } return nil }
191-209: Heuristic-based validation may be fragile.The MCP config generation check relies on keyword matching ("success", "generated", "created") or file extension presence. This could produce false positives/negatives if CLI output format changes. Consider validating against expected output patterns or checking file existence.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cli/it/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (29)
cli/it/INTEGRATION-TESTS.mdcli/it/Makefilecli/it/README.mdcli/it/config.gocli/it/features/gateway/api.featurecli/it/features/gateway/apply.featurecli/it/features/gateway/build.featurecli/it/features/gateway/management.featurecli/it/features/gateway/mcp.featurecli/it/go.modcli/it/reporter.gocli/it/resources/gateway/invalid.yamlcli/it/resources/gateway/policy-manifest.yamlcli/it/resources/gateway/sample-api.yamlcli/it/resources/gateway/sample-mcp-config.yamlcli/it/resources/values.gocli/it/setup.gocli/it/state.gocli/it/steps/assert_steps.gocli/it/steps/cli_steps.gocli/it/suite_test.gocli/it/test-config.yamlcli/src/Makefilecli/src/internal/gateway/resources.gocli/src/scripts/run-tests-with-summary.shcli/src/test/TESTING.mdcli/src/test/test-config.yamlcli/src/test/unit/cmd_naming_convention_test.gocli/src/test/unit/flags_test.go
💤 Files with no reviewable changes (2)
- cli/src/test/test-config.yaml
- cli/src/test/TESTING.md
🧰 Additional context used
🧬 Code graph analysis (6)
cli/it/setup.go (3)
cli/it/config.go (5)
InfrastructureID(63-63)InfraCLI(67-67)InfraGatewayImages(69-69)InfraGateway(71-71)InfraMCPServer(73-73)cli/it/reporter.go (1)
TestReporter(59-64)cli/it/resources/values.go (2)
DefaultStartupTimeout(48-48)HealthCheckInterval(51-51)
cli/it/state.go (2)
cli/it/steps/cli_steps.go (1)
TestState(32-41)gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)
cli/it/steps/cli_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/resources/values.go (6)
GetResourcePath(118-122)GatewayControllerURL(30-30)GetSampleAPIPath(125-127)TestAPIName(75-75)TestAPIVersion(78-78)GetPolicyManifestPath(135-137)
cli/it/config.go (2)
cli/it/setup.go (1)
HealthCheckInterval(39-39)cli/it/resources/values.go (1)
HealthCheckInterval(51-51)
cli/it/steps/assert_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/steps/cli_steps.go (1)
TestState(32-41)
cli/it/reporter.go (3)
gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)cli/it/state.go (1)
TestState(33-69)cli/it/steps/cli_steps.go (1)
TestState(32-41)
🪛 checkmake (0.2.2)
cli/it/Makefile
[warning] 26-26: Target body for "help" exceeds allowed length of 5 (12).
(maxbodylength)
[warning] 19-19: Missing required phony target "all"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
cli/it/INTEGRATION-TESTS.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
395-395: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
420-420: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
517-517: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
571-571: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
cli/it/README.md
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
cli/src/scripts/run-tests-with-summary.sh
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 YAMLlint (1.37.1)
cli/it/resources/gateway/invalid.yaml
[error] 22-22: syntax error: expected ',' or ']', but got ':'
(syntax)
🔇 Additional comments (27)
cli/src/test/unit/flags_test.go (3)
19-19: LGTM: Package reorganization aligns with test separation.The package rename from
teststounitclearly distinguishes unit tests from the new integration test framework being introduced in this PR.
69-70: LGTM: Helpful comment about test runner behavior.The comment clarifies that the divider is handled externally, preventing duplication. This is good documentation for future maintainers.
32-32: Import path resolves correctly.The relative path
../../utils/flags.gofromcli/src/test/unit/flags_test.gosuccessfully resolves tocli/src/utils/flags.go. No issues detected.cli/src/Makefile (1)
105-105: LGTM! Test path reorganization aligns with integration test framework.The change from
./test/basic/...to./test/unit/...correctly separates unit tests from integration tests, supporting the new CLI integration test infrastructure introduced in this PR.cli/src/scripts/run-tests-with-summary.sh (1)
16-16: LGTM! Test scope correctly narrowed to unit tests.The test path change to
./test/unit/...is consistent with the Makefile update and properly separates unit tests from the new integration test framework.cli/it/go.mod (2)
3-3: Go version 1.25.1 may not exist yet.The
go 1.25.1directive specifies a version that may not be released. This could cause build failures. Verify this is the intended version, or consider using a currently available stable version (e.g., 1.22.x or 1.23.x).
5-8: Dependencies are appropriate for BDD testing.The direct dependencies (
godogfor BDD-style tests andyaml.v3for configuration parsing) are well-suited for the integration test framework.cli/src/test/unit/cmd_naming_convention_test.go (1)
42-161: Well-structured test with clear violation reporting.The test logic is sound: it walks the command directory, parses Go files for Cobra command definitions, and validates naming conventions. The detailed violation messages with convention rules will help developers fix issues quickly.
cli/it/resources/gateway/policy-manifest.yaml (1)
19-24: Clarify if emptyfilePathis intentional.The
filePath: ""field is empty. If this is intentional for testing edge cases (e.g., testing validation of missing file paths), consider adding a comment to clarify. Otherwise, provide a valid path or placeholder.cli/it/resources/gateway/invalid.yaml (1)
19-23: Intentionally invalid YAML - LGTM.This file correctly implements invalid YAML syntax for negative test cases. The static analysis warning is expected and can be safely ignored since the purpose is to trigger parsing errors during testing.
cli/it/features/gateway/api.feature (1)
24-28: Background setup is well-designed.The background appropriately establishes prerequisites (CLI, gateway, configuration) that apply to all scenarios, reducing duplication and improving maintainability.
cli/it/features/gateway/build.feature (1)
28-41: Only failure scenarios are tested.The build feature only tests invalid/missing manifest cases. Consider adding a success scenario with a valid manifest to ensure the happy path works, unless intentionally omitted due to build time/resource constraints.
Is there a reason successful build scenarios are excluded? If builds are expensive, consider documenting this decision or adding a
@slowtag for optional execution.cli/it/README.md (1)
7-7: No changes needed. Go 1.25.1 is a valid version and appropriate for documentation as of January 2026.Likely an incorrect or invalid review comment.
cli/it/resources/gateway/sample-mcp-config.yaml (1)
19-33: Looks consistent with the updated MCP kind casing.
kind: McpmatchesResourceKindMCP = "Mcp", and the manifest is minimal/clear for integration testing.cli/it/resources/gateway/sample-api.yaml (1)
19-39: LGTM! Well-structured test resource.The RestApi manifest is well-defined for CLI integration testing. The structure follows standard Kubernetes CRD patterns, operations cover typical CRUD scenarios, and the upstream backend reference aligns with the Docker Compose test stack.
cli/it/features/gateway/mcp.feature (1)
19-113: LGTM! Well-designed BDD scenarios.The MCP management scenarios provide excellent coverage of both positive and negative test cases. The Given-When-Then structure is clear, scenario tags align with the documented test matrix, and resource paths are consistent with the project structure.
cli/it/Makefile (1)
41-43: Verify that output truncation doesn't hide test failures.Line 43 pipes test output through awk to exit after printing "Logs directory:", which truncates any output appearing afterward. If test failures or important diagnostics occur after this marker, they won't be visible in the console.
Consider removing the awk filter or adjusting the pattern to ensure critical failure information is always displayed:
Alternative approach
test: check-docker @mkdir -p logs - @go test -v -timeout 30m ./... 2>&1 | awk '/^Logs directory:/{print;exit}1' + @go test -v -timeout 30m ./...Or if you want to preserve some output control:
- @go test -v -timeout 30m ./... 2>&1 | awk '/^Logs directory:/{print;exit}1' + @go test -v -timeout 30m ./... 2>&1 | tee /dev/tty | awk '/^Logs directory:/{print;exit}1'cli/it/setup.go (3)
92-110: Implicit dependency handling may miss requirements.The
InfraGatewaycase ensuresInfraGatewayImagesis built, andInfraMCPServerensuresInfraGatewayis started. However, theInfraCLIdependency isn't automatically included. If a test requires the gateway but not explicitly the CLI, tests may fail.Consider documenting the expected dependency chain or making dependencies explicit in the config.
318-327: LGTM!Using
exec.Commandinstead ofexec.CommandContextin Teardown is the correct approach - it ensures cleanup proceeds even if the context was cancelled.
342-360: LGTM!Pre-flight port availability check is a good practice for integration tests. The TOCTOU window is acceptable for this use case.
cli/it/resources/values.go (1)
96-115: LGTM!Error message constants are well-defined and useful for consistent error validation in tests.
cli/it/suite_test.go (1)
267-289: LGTM!Infrastructure step implementations correctly delegate to the infrastructure manager for health checks and availability verification.
cli/it/reporter.go (1)
273-347: LGTM!The
PrintSummarymethod provides a well-formatted, colorized summary with proper mutex protection. The table formatting handles name truncation appropriately.cli/it/steps/cli_steps.go (1)
103-122: LGTM!
EnsureGatewayExistsproperly handles the "already exists" case as a success condition, which is the expected idempotent behavior for setup steps.cli/it/steps/assert_steps.go (2)
98-124: Assertions execute CLI commands, modifying test state.
GatewayShouldExistandGatewayShouldNotExistrungateway list, overwritingLastCommand,LastStdout, etc. This side effect may cause issues if subsequent assertions rely on the original command's output. Consider documenting this behavior or preserving state before/after.
38-96: LGTM!Basic assertion methods are well-implemented with clear error messages that include actual vs expected values and relevant output for debugging.
cli/it/config.go (1)
166-174: No changes needed.The project requires Go 1.25.1 (per
cli/it/config.go's module configuration in go.mod), which is well after Go 1.22 when the loop variable semantics were fixed. In Go 1.22+, the loop variable is allocated per iteration, makingreturn &testsafe and correct. The proposed copy workaround is unnecessary for this codebase.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cli/it/setup.go:
- Around line 379-397: CheckPortsAvailable is dead code; either delete the
CheckPortsAvailable function or call it from InitializeTestSuite so the test
suite performs the pre-flight port check and fails early; if choosing to call
it, invoke CheckPortsAvailable() at the start of InitializeTestSuite and
return/fatal the error (e.g., t.Fatalf or log+os.Exit) so test initialization
stops when ports are in use, mirroring the existing pre-flight pattern used
elsewhere.
- Around line 280-287: The code sets m.startedServices[InfraMCPServer] = true
even when waitForMCPServer() returns an error; change startGatewayStack() so the
MCP service flag is only set on success (i.e., after waitForMCPServer() returns
nil) and propagate the failure instead of silently continuing: either return the
error from startGatewayStack() when waitForMCPServer() fails (so callers can
retry or fail fast) or, if you must continue, do not set
m.startedServices[InfraMCPServer] and document that callers should retry
waitForMCPServer(); modify the block around waitForMCPServer(), the error
handling branch, and where m.startedServices[InfraMCPServer] is assigned to
ensure the flag reflects actual readiness.
In @cli/it/steps/cli_steps.go:
- Around line 239-260: The ResetConfiguration method in CLISteps currently
writes to the real user home (~/.wso2ap) instead of the test-isolated directory;
change it to use s.state.ConfigDir (which the test harness sets via
AP_CONFIG_DIR) when building configDir/configFile so the method writes the empty
config into the isolated test config directory (use s.state.ConfigDir as the
base instead of os.UserHomeDir()), keep the same file content (emptyConfig) and
error handling around os.MkdirAll and os.WriteFile for configDir/configFile.
🧹 Nitpick comments (3)
cli/it/Makefile (1)
19-19: Addtest-allto.PHONYdeclaration.The
test-alltarget is missing from the.PHONYlist, which could cause issues if a file namedtest-allexists.Proposed fix
-.PHONY: test clean deps check-docker build-images help +.PHONY: test test-all clean deps check-docker build-images helpcli/it/suite_test.go (1)
191-195: Consider removing redundantLogTestcall.The
LogTestmethod is a no-op (perreporter.golines 270-273), andEndTestalready handles result recording. This call could be removed for clarity.Proposed removal
testReporter.EndTest(testState, passed, errorMsg) - // Log the result - if testState.TestID != "" { - testReporter.LogTest(testState.TestID, testState.TestName, passed, - testReporter.generateLogFileName(testState.TestID, testState.TestName)) - } - testState.Cleanup()cli/it/steps/cli_steps.go (1)
262-280: Minor redundancy in path handling.The condition at line 266-267 assigns
filePathtoabsPathwhen it's already absolute, butabsPathwas already initialized tofilePathat line 265.Simplified logic
func (s *CLISteps) ApplyResourceFile(filePath string) error { // Convert relative resource paths to absolute paths - absPath := filePath - if filepath.IsAbs(filePath) { - absPath = filePath - } else if strings.HasPrefix(filePath, "resources/gateway/") { + var absPath string + if strings.HasPrefix(filePath, "resources/gateway/") { // Extract the filename from the relative path filename := filepath.Base(filePath) absPath = resources.GetResourcePath(filename) + } else if !filepath.IsAbs(filePath) { + var err error + absPath, err = filepath.Abs(filePath) + if err != nil { + return fmt.Errorf("failed to resolve file path: %w", err) + } } else { - var err error - absPath, err = filepath.Abs(filePath) - if err != nil { - return fmt.Errorf("failed to resolve file path: %w", err) - } + absPath = filePath } return s.state.ExecuteCLI("gateway", "apply", "-f", absPath) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/it/Makefilecli/it/config.gocli/it/features/gateway/api.featurecli/it/reporter.gocli/it/setup.gocli/it/steps/cli_steps.gocli/it/suite_test.gocli/src/scripts/run-tests-with-summary.shcli/src/test/unit/cmd_naming_convention_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/it/config.go
- cli/src/scripts/run-tests-with-summary.sh
- cli/it/features/gateway/api.feature
🧰 Additional context used
🧬 Code graph analysis (3)
cli/it/setup.go (4)
cli/it/config.go (6)
InfrastructureID(64-64)TestConfig(29-32)InfraCLI(68-68)InfraGatewayImages(70-70)InfraGateway(72-72)InfraMCPServer(74-74)cli/it/reporter.go (1)
TestReporter(59-64)cli/it/resources/values.go (2)
DefaultStartupTimeout(48-48)HealthCheckInterval(51-51)sdk/gateway/policy/v1alpha/context.go (1)
Body(4-16)
cli/it/suite_test.go (6)
cli/it/setup.go (2)
InfrastructureManager(52-62)CheckDockerAvailable(371-377)cli/it/reporter.go (2)
TestReporter(59-64)NewTestReporter(67-72)cli/it/steps/cli_steps.go (3)
TestState(32-41)CLISteps(44-46)NewCLISteps(49-51)cli/it/state.go (2)
TestState(33-69)NewTestState(72-78)cli/it/config.go (2)
TestConfig(29-32)LoadTestConfig(78-90)cli/it/steps/assert_steps.go (2)
AssertSteps(29-31)NewAssertSteps(34-36)
cli/it/reporter.go (3)
gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)cli/it/steps/cli_steps.go (1)
TestState(32-41)cli/it/state.go (1)
TestState(33-69)
🪛 checkmake (0.2.2)
cli/it/Makefile
[warning] 26-26: Target body for "help" exceeds allowed length of 5 (12).
(maxbodylength)
[warning] 41-41: Target body for "test" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 19-19: Missing required phony target "all"
(minphony)
🔇 Additional comments (20)
cli/it/Makefile (2)
41-48: LGTM!The test target correctly identifies packages with tests and handles the case where no test packages are found. The 30-minute timeout is reasonable for integration tests.
63-68: LGTM!The clean target properly handles Docker cleanup with error suppression for idempotent execution.
cli/src/test/unit/cmd_naming_convention_test.go (2)
42-166: LGTM!The test comprehensively validates the Cobra command naming convention by:
- Walking the cmd directory and filtering appropriately
- Parsing Go files to extract
cobra.Commanddefinitions- Comparing file paths against command hierarchies
- Providing detailed violation reports with actionable guidance
The error messages are well-crafted with clear examples.
174-266: LGTM!The AST parsing logic correctly handles:
- Both pointer and value composite literals for
cobra.Command- String literals and const-based identifiers for the
Usefield- Deduplication to avoid processing the same literal twice
The fallback behavior for unresolved identifiers is reasonable for this validation use case.
cli/it/suite_test.go (3)
56-106: LGTM!The test entry point correctly:
- Loads configuration and initializes the reporter
- Sets up the Godog test suite with appropriate options
- Prints a summary and fails the test on non-zero exit codes
The global state pattern is acceptable for test orchestration.
109-150: LGTM!The test suite lifecycle is well-structured:
- Pre-flight Docker check prevents cryptic failures
- Infrastructure setup is configuration-driven
- Teardown gracefully handles errors as warnings
212-421: LGTM!The step definitions are well-organized by category (infrastructure, CLI, assertions, gateway, API, MCP, build) with clean delegation to the step handler classes.
cli/it/setup.go (5)
34-62: LGTM!The constants define reasonable timeouts for integration tests, and the
InfrastructureManagerstruct includes proper synchronization with a mutex.
64-108: LGTM!The constructor properly handles path resolution relative to the config file and gracefully degrades to defaults when configuration parsing fails.
110-154: LGTM!The setup method correctly handles infrastructure dependencies (gateway images before gateway stack, MCP server with gateway) and tracks started services to avoid redundant initialization.
156-196: LGTM!The CLI build process includes proper verification: checking binary existence and running a version command to ensure the binary is functional.
348-368: LGTM!The teardown method properly cleans up Docker resources and cancels the context. Using
exec.Commandinstead ofCommandContextfor cleanup ensures teardown completes even if the parent context is cancelled.cli/it/steps/cli_steps.go (3)
31-51: LGTM!The
TestStateinterface provides a clean abstraction for CLI execution and state management, enabling testable step definitions.
69-83: LGTM!The
RunWithArgsmethod correctly resolves resource paths for test resources. The pattern matching forresources/gateway/is sufficient for current test scenarios.
85-101: LGTM!The gateway add logic correctly handles authentication modes and properly checks both the error return and exit code before updating state.
cli/it/reporter.go (5)
43-72: LGTM!The
TestResultstruct captures comprehensive test metadata, andTestReporterincludes proper mutex synchronization for concurrent access.
82-129: LGTM!The test lifecycle methods properly synchronize access and handle edge cases like a nil
currentTest. The log file is written before appending results, ensuring data consistency.
147-205: LGTM!The log file generation uses appropriate sanitization for filenames and produces a well-structured, human-readable log format. Write failures are logged as warnings without failing tests.
275-349: LGTM!The summary output is well-formatted with:
- A results table with proper column alignment
- Color-coded status indicators
- A summary box with counts and duration
- Clear indication of the logs directory
351-372: LGTM!
GetResultscorrectly returns a copy of the results slice to prevent external modification of internal state.HasFailuresprovides an efficient check without exposing the full results.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cli/it/setup.go:
- Around line 131-141: When handling the InfraGateway case, after successfully
calling buildGatewayImages() you must mark the dependency as started to avoid
redundant builds; specifically, set m.startedServices[InfraGatewayImages] = true
immediately after the build succeeds (before calling startGatewayStack()), so
that subsequent iterations checking m.startedServices see the images as already
built; keep the rest of the flow (error returns and startGatewayStack() call)
unchanged.
In @cli/it/state.go:
- Around line 80-118: Reset() must create a fresh cancellable context so
subsequent ExecuteCLI() calls don't see a canceled s.ctx; update Reset() to call
context.WithCancel(context.Background()) and assign both s.ctx and s.cancel
(replacing any previous values), ensuring the new context is used by
ExecuteCLI(), and if desired call any existing s.cancel before overwriting to
avoid leaks.
🧹 Nitpick comments (6)
cli/it/suite_test.go (4)
56-64: Config path resolution assumes fixed working directory.The hardcoded relative path
"test-config.yaml"assumes tests are always run from thecli/itdirectory. If tests are invoked from a different directory, this will fail.Consider using a path relative to the test file location or allowing override via environment variable.
Suggested improvement
func TestFeatures(t *testing.T) { // Load configuration - configPath := "test-config.yaml" + configPath := os.Getenv("CLI_IT_CONFIG") + if configPath == "" { + // Default to path relative to this test file + _, thisFile, _, _ := runtime.Caller(0) + configPath = filepath.Join(filepath.Dir(thisFile), "test-config.yaml") + } var err error testConfig, err = LoadTestConfig(configPath)
176-183: Tag extraction uses only the first tag as TestID.The loop breaks after the first tag, meaning if a scenario has multiple tags (e.g.,
@TC001 @smoke @gateway), onlyTC001will be used as the TestID. If the first tag is a category tag like@smoke, the actual test ID might be missed.Consider filtering for tags matching a specific pattern (e.g.,
@TC\d+).Suggested pattern-based filtering
+import "regexp" + +var testIDPattern = regexp.MustCompile(`^@(TC\d+|[A-Z]+-\d+)$`) + // Extract test ID from tags if available for _, tag := range sc.Tags { - if len(tag.Name) > 1 && tag.Name[0] == '@' { + if testIDPattern.MatchString(tag.Name) { testState.SetTestInfo(tag.Name[1:], sc.Name) testReporter.StartTest(tag.Name[1:], sc.Name) break } }
393-395: Hardcoded MCP server URL should use a constant.The URL
"http://localhost:3001/mcp"is hardcoded here but should reference theMCPServerPortconstant or a resource constant for consistency and maintainability.Suggested fix
func iGenerateMCPConfigToOutput(output string) error { - return cliSteps.GenerateMCPConfig("http://localhost:3001/mcp", output) + return cliSteps.GenerateMCPConfig(fmt.Sprintf("http://localhost:%s/mcp", MCPServerPort), output) }
34-53: Global mutable state pattern is not suitable for concurrent scenarios, but concurrent execution is not currently enabled.The code currently uses global variable reassignment (
testState = NewTestState()in the Before hook), which is safe for sequential execution. However, the current Godog configuration does not enable parallel execution—noConcurrencyoption is set ingodog.Options, so Godog defaults to sequential mode.If concurrent execution is planned in the future, refactor to use Godog's context propagation pattern or allocate per-scenario state objects to avoid race conditions. For now, this is not a blocking issue.
cli/it/state.go (1)
134-175: Mutex held during potentially long CLI execution may cause contention.The write lock is held for the entire duration of command execution (up to 30 seconds). This blocks all other state access including read operations. While this ensures consistency, it may cause issues if any concurrent access is needed.
Consider releasing the lock after setting up the command and reacquiring it to store results, or document that
ExecuteCLIis expected to be called sequentially.cli/it/steps/cli_steps.go (1)
264-282: Minor: Redundant assignment when path is already absolute.Lines 267-269 assign
absPath = filePathwhenfilepath.IsAbs(filePath)is true, butabsPathis already initialized tofilePathon line 267. This is redundant but harmless.Simplified version
func (s *CLISteps) ApplyResourceFile(filePath string) error { - // Convert relative resource paths to absolute paths - absPath := filePath - if filepath.IsAbs(filePath) { - absPath = filePath - } else if strings.HasPrefix(filePath, "resources/gateway/") { + var absPath string + if strings.HasPrefix(filePath, "resources/gateway/") { // Extract the filename from the relative path filename := filepath.Base(filePath) absPath = resources.GetResourcePath(filename) - } else { - var err error - absPath, err = filepath.Abs(filePath) + } else if !filepath.IsAbs(filePath) { + var err error + absPath, err = filepath.Abs(filePath) if err != nil { return fmt.Errorf("failed to resolve file path: %w", err) } + } else { + absPath = filePath } return s.state.ExecuteCLI("gateway", "apply", "-f", absPath) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/it/setup.gocli/it/state.gocli/it/steps/cli_steps.gocli/it/suite_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
cli/it/suite_test.go (6)
cli/it/setup.go (1)
InfrastructureManager(52-62)cli/it/reporter.go (2)
TestReporter(59-64)NewTestReporter(67-72)cli/it/state.go (2)
TestState(33-69)NewTestState(72-78)cli/it/steps/cli_steps.go (3)
TestState(32-42)CLISteps(45-47)NewCLISteps(50-52)cli/it/config.go (2)
TestConfig(29-32)LoadTestConfig(78-90)cli/it/steps/assert_steps.go (2)
AssertSteps(29-31)NewAssertSteps(34-36)
cli/it/steps/cli_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/resources/values.go (5)
GetResourcePath(118-122)GatewayControllerURL(30-30)GetSampleAPIPath(125-127)TestAPIName(75-75)TestAPIVersion(78-78)
cli/it/state.go (2)
cli/it/steps/cli_steps.go (1)
TestState(32-42)gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)
🔇 Additional comments (15)
cli/it/suite_test.go (1)
108-156: LGTM!The suite initialization properly handles infrastructure setup with pre-flight checks and cleanup on teardown. The use of
log.Fatalffor setup failures is appropriate for failing fast, and the warning-only approach for teardown errors is reasonable.cli/it/setup.go (6)
64-108: LGTM!The
NewInfrastructureManagerconstructor properly handles configuration parsing with sensible defaults and appropriate warning logging for invalid values. The compose file path resolution handles both relative and absolute paths correctly.
164-204: LGTM!The CLI build process properly uses context for cancellation, verifies the binary exists and runs, and provides detailed logging. The relative path assumption is acceptable for this test infrastructure.
235-298: LGTM!The gateway stack startup is well-structured with proper cleanup of previous containers, health checks for both gateway controller and MCP server, and appropriate error handling. The
--waitflag on docker compose helps ensure containers are ready.
356-376: LGTM!The teardown correctly uses
exec.Command(notexec.CommandContext) since the manager's context may already be cancelled. The silent error handling during cleanup is appropriate, and cancelling the context after cleanup is the correct order.
315-349: LGTM!Both health check functions properly handle resource cleanup (closing response bodies and connections), use configurable timeouts and intervals, and have appropriate error messages. The TCP-only check for MCP is simpler but sufficient for connectivity verification.
387-405: LGTM!The port availability check is a reasonable pre-flight validation. While there's an inherent TOCTOU (time-of-check-time-of-use) race between checking and starting services, this is acceptable for early failure detection and improving the developer experience.
cli/it/state.go (3)
71-78: LGTM!The constructor properly initializes a cancellable context for the test state lifecycle.
120-132: LGTM!The cleanup properly records timing, cancels the context, and removes temporary resources.
177-259: LGTM!All accessor methods properly use read/write locks for thread-safe access. The
GetDurationcorrectly handles the case whereEndTimeis not yet set.cli/it/steps/cli_steps.go (5)
31-42: LGTM!The
TestStateinterface properly defines the contract for state access from the steps package, enabling loose coupling between steps and the concrete state implementation.
86-102: LGTM!The gateway add command construction properly handles authentication options. The hardcoded test credentials are acceptable for integration test infrastructure.
104-123: LGTM!Both methods properly handle the idempotent case where the gateway already exists. The difference in auth handling (basic vs. none) is appropriate for their respective use cases as documented.
Also applies to: 220-238
240-262: LGTM!The configuration reset properly uses the isolated test config directory and writes a valid empty configuration. Error handling is thorough with descriptive error messages.
70-84: The path resolution approach is consistent with the resource directory structure, which is flat. TheGetResourcePathfunction is designed to accept filenames only (not paths with subdirectories), and the resources incli/it/resources/gateway/are organized without subdirectories. The use offilepath.Baseto extract the filename aligns with this design.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @cli/it/setup.go:
- Around line 237-300: The code uses a global ComposeProjectName constant which
can collide in parallel runs; add a composeProjectName string field to
InfrastructureManager, default it in the manager constructor/startup to a unique
value (e.g., fmt.Sprintf("cli-it-%d", os.Getpid()) or append a random suffix)
and replace uses of the global ComposeProjectName in startGatewayStack
(exec.Command args), stopGatewayStack, and Teardown so they all reference
m.composeProjectName; ensure the default is set before any call to
startGatewayStack/stopGatewayStack and that tests can still override the field
for deterministic runs.
- Around line 166-206: The buildCLI method uses brittle relative lookups like
filepath.Abs("../src") (and similar uses later for "../../gateway") which break
if tests run with a different CWD; change buildCLI to resolve source and gateway
paths relative to a stable anchor instead: use a base dir provided on the
manager (e.g., a cfgPath/baseDir field initialized in NewInfrastructureManager)
or compute the directory of the current file with runtime.Caller and join from
there, then use that resolved path for cliSrcDir, m.cliBinaryPath, and the
gateway path verification to avoid depending on the process CWD.
- Around line 389-407: The CheckPortsAvailable function drops the original
listen error which loses useful details; update the error return in the loop to
wrap the underlying err (e.g., use fmt.Errorf with %w) and provide a clearer
message (e.g., "failed to listen on port %s: %w") so permission issues vs in-use
errors are preserved; keep the existing net.Listen and ln.Close logic and only
change the fmt.Errorf construction in CheckPortsAvailable to include the wrapped
err.
- Around line 110-164: In SetupInfrastructure, the InfraMCPServer case can call
startGatewayStack() but never sets m.startedServices[InfraGateway]=true, which
allows a subsequent InfraGateway iteration to rebuild the stack; update the
InfraMCPServer branch in the SetupInfrastructure method so that after a
successful call to m.startGatewayStack() you also set
m.startedServices[InfraGateway] = true (only on nil error), and leave the
existing waitForMCPServer() path unchanged so that the gateway-start flag
accurately reflects the running stack.
- Around line 270-276: The code invokes "docker compose ... --wait" in
cli/it/setup.go (around the exec.CommandContext call using m.composeFile and
ComposeProjectName) which requires Docker Compose V2 or later; update the
project documentation to state this minimum requirement (e.g., add a clear note
to README.md and any setup/CI docs such as docs/SETUP.md or the README section
for integration tests) specifying "Docker Compose v2.0+ (supports --wait)" and
include a short troubleshooting note on how to check/upgrade the Compose
version; optionally also add a one-line comment above the exec.CommandContext
call referencing the minimum version.
In @cli/it/state.go:
- Around line 142-183: The ExecuteCLI function currently swallows non-ExitError
failures; change the final error handling so that when cmd.Run() returns a
non-*exec.ExitError (e.g., spawn or context timeout errors) you record
LastStdout/LastStderr/LastExitCode as you already do and then return that error
instead of nil; keep the existing behavior of extracting and setting ExitCode
when the error is an *exec.ExitError (using exitErr.ExitCode()) but ensure only
exit errors do not cause ExecuteCLI to return a non-nil error. Ensure you
reference ExecuteCLI, LastStdout, LastStderr, LastExitCode and the err / exitErr
variables when making the change.
🧹 Nitpick comments (3)
cli/it/setup.go (1)
111-114: Avoid holdingm.muwhile runningmake/docker(long critical section).
Line 112-113 holds the mutex across slow external processes; this blocks teardown and any concurrent setup calls for the full duration.Suggested direction: only lock to read/update
m.startedServices, but runbuildCLI/buildGatewayImages/startGatewayStackoutside the lock (with a second lock to commit state), or switch to per-infrasync.Once/singleflight if you need de-dupe.Also applies to: 120-160
cli/it/state.go (2)
32-69: Exported fields undermine themu-based “thread-safe state” story.
Because fields likeLastStdout,GatewayServer, etc. are exported (Line 35+), callers can read/write them without locks, reintroducing data races.Make fields unexported and force access via methods (or clearly document that direct field access is unsupported and not concurrency-safe).
143-180: Consider releasing the lock while the process runs.
Line 144-145 holds the write lock for the full command duration (up to 30s). This blocksGetStdout()/GetExitCode()and can make reporting/debugging harder.Pattern: snapshot immutable inputs under lock (
CLIBinaryPath,WorkingDir,ConfigDir), run the process without the lock, then reacquire lock to storeLastStdout/LastExitCode.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/it/setup.gocli/it/state.go
🧰 Additional context used
🧬 Code graph analysis (2)
cli/it/state.go (2)
cli/it/steps/cli_steps.go (1)
TestState(32-42)gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)
cli/it/setup.go (3)
cli/it/config.go (1)
InfrastructureID(64-64)cli/it/reporter.go (1)
TestReporter(59-64)cli/it/resources/values.go (2)
DefaultStartupTimeout(48-48)HealthCheckInterval(51-51)
🔇 Additional comments (1)
cli/it/state.go (1)
80-126: Reset/Cleanup look solid; context is refreshed per scenario.
Canceling the prior context onReset()(Line 105-109) prevents leaked goroutines and ensures subsequentExecuteCLIcalls have a live context.Also applies to: 128-140
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/it/steps/cli_steps.go:
- Around line 256-260: The config file is created with permissive 0644 mode in
the block that writes emptyConfig to configFile; change the file permissions to
0600 to match backupAndClearUserConfig and enforce owner-only access. Update the
os.WriteFile call that writes emptyConfig (variable names: emptyConfig,
configFile) to use 0600 instead of 0644 so permission handling is consistent
across the tests.
🧹 Nitpick comments (5)
cli/it/README.md (1)
54-63: Add language specifier to fenced code block.The code block showing the test structure lacks a language specifier, which helps with syntax highlighting and satisfies markdown linting rules.
📝 Suggested fix
-``` +```text cli/it/ ├── test-config.yaml # Enable/disable tests ├── features/ # Gherkin feature filescli/it/suite_test.go (1)
250-257: Tag extraction assumes test ID is the first tag.The loop breaks after finding the first
@-prefixed tag. If scenarios have multiple tags (e.g.,@wip @GW-MANAGE-001), this may extract the wrong tag as the test ID. Consider filtering for tags matching the expected test ID pattern (e.g.,GW-,API-,MCP-).♻️ Suggested pattern-based extraction
for _, tag := range sc.Tags { - if len(tag.Name) > 1 && tag.Name[0] == '@' { - testState.SetTestInfo(tag.Name[1:], sc.Name) - testReporter.StartTest(tag.Name[1:], sc.Name) - break + tagName := strings.TrimPrefix(tag.Name, "@") + // Match expected test ID patterns + if strings.HasPrefix(tagName, "GW-") || + strings.HasPrefix(tagName, "API-") || + strings.HasPrefix(tagName, "MCP-") { + testState.SetTestInfo(tagName, sc.Name) + testReporter.StartTest(tagName, sc.Name) + break } }cli/it/setup.go (1)
368-376: Teardown command lacks timeout protection.Unlike other commands that use
exec.CommandContext, the teardowndocker compose downuses plainexec.Command. If Docker becomes unresponsive, this could hang indefinitely.♻️ Add timeout context for teardown
// Stop compose stack using native docker compose if m.composeFile != "" { - cmd := exec.Command("docker", "compose", + teardownCtx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cmd := exec.CommandContext(teardownCtx, "docker", "compose", "-f", m.composeFile, "-p", m.composeProjectName, "down", "-v", "--remove-orphans", )cli/it/steps/cli_steps.go (2)
91-94: Hardcoded test credentials in source code.The basic auth credentials (
admin/admin) are hardcoded here. While acceptable for integration tests, consider extracting these to the test configuration or resource files for easier maintenance and to avoid credential sprawl in source code.
229-237: Misleading comment: gateway is not actually updated.The comment states "If gateway already exists, update it" but the code doesn't perform any update operation—it just sets the state and returns. Consider updating the comment to reflect the actual behavior.
📝 Suggested comment fix
- // If gateway already exists, update it + // If gateway already exists, that's acceptable - just update state if s.state.GetExitCode() != 0 {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/it/README.mdcli/it/setup.gocli/it/state.gocli/it/steps/cli_steps.gocli/it/suite_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T08:31:38.227Z
Learnt from: VirajSalaka
Repo: wso2/api-platform PR: 642
File: gateway/gateway-controller/cmd/controller/main.go:272-290
Timestamp: 2026-01-09T08:31:38.227Z
Learning: In gateway/gateway-controller/cmd/controller/main.go, the EventListener.Start() call must be synchronous (blocking) to ensure the EventListener is fully initialized before the REST API server starts. This blocking behavior is intentional to maintain proper startup ordering in multi-replica mode.
Applied to files:
cli/it/setup.go
🧬 Code graph analysis (3)
cli/it/suite_test.go (6)
cli/it/setup.go (3)
InfrastructureManager(49-60)CheckDockerAvailable(383-389)CheckPortsAvailable(392-409)cli/it/reporter.go (1)
TestReporter(59-64)cli/it/state.go (2)
TestState(33-69)NewTestState(72-78)cli/it/steps/cli_steps.go (3)
TestState(32-42)CLISteps(45-47)NewCLISteps(50-52)cli/it/config.go (2)
TestConfig(29-32)LoadTestConfig(78-90)cli/it/steps/assert_steps.go (2)
AssertSteps(29-31)NewAssertSteps(34-36)
cli/it/state.go (2)
cli/it/steps/cli_steps.go (1)
TestState(32-42)gateway/gateway-builder/pkg/errors/errors.go (1)
ExitCode(27-27)
cli/it/steps/cli_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/resources/values.go (3)
GetResourcePath(118-122)GatewayControllerURL(30-30)GetSampleAPIPath(125-127)
🪛 markdownlint-cli2 (0.18.1)
cli/it/README.md
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
cli/it/suite_test.go (3)
34-53: Global mutable state limits parallel test execution.These global variables (
infraManager,testReporter,testState, etc.) create shared mutable state that prevents running scenarios in parallel. This is acceptable for sequential BDD tests, but worth noting if parallelism is desired in the future.
168-201: LGTM! Config backup implementation is well-structured.The backup/restore logic properly handles the case where no config exists, uses secure file permissions (0600), and ensures cleanup in AfterSuite even on failures.
56-106: LGTM! Test suite entry point is well-structured.The
TestFeaturesfunction properly loads configuration, sets up reporting, configures the Godog test suite, and handles the exit code appropriately.cli/it/setup.go (3)
62-109: LGTM! Infrastructure manager initialization is robust.The constructor properly handles:
- Unique project naming with PID + timestamp to avoid collisions
- Graceful fallback for config resolution failures
- Duration parsing with sensible defaults
111-166: Mutex held during long-running infrastructure setup.The mutex is held for the entire
SetupInfrastructureoperation, which includes building CLI, building Docker images, and starting the compose stack. While this prevents concurrent access, it could block for several minutes. This is likely intentional for sequential setup, but consider documenting this behavior.
319-353: LGTM! Health check implementations are robust.Both
waitForGatewayHealthandwaitForMCPServerproperly:
- Use timeouts on individual checks
- Close resources (response body, connection) correctly
- Implement deadline-based polling with configurable intervals
cli/it/state.go (4)
71-78: LGTM! Constructor properly initializes cancellable context.The
NewTestStateconstructor correctly creates a context with cancellation support for managing CLI execution lifecycle.
80-126: LGTM! Reset method properly handles context lifecycle.The method correctly:
- Cancels the previous context before creating a new one (prevents resource leaks)
- Creates an isolated temp directory with config subdirectory
- Resets all state fields for a clean test environment
189-225: LGTM! Thread-safe accessors with appropriate lock usage.All getters correctly use
RLockfor concurrent read access, and setters useLockfor exclusive write access. Mutex usage is consistent throughout.
156-163: No action needed: AP_CONFIG_DIR is not used by the CLI.The CLI's configuration loading (in
cli/src/internal/config/config.go) usesos.UserHomeDir()directly without checking for anAP_CONFIG_DIRenvironment variable. Therefore, not setting this variable inExecuteCLIis not a bug—the CLI doesn't expect it.Likely an incorrect or invalid review comment.
cli/it/steps/cli_steps.go (1)
54-68: LGTM! Command parsing handles "ap" prefix gracefully.The
RunCommandfunction correctly strips the "ap" prefix if present, allowing feature files to use natural command syntax likeI run "ap gateway list".cli/it/README.md (1)
7-7: > Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/it/steps/cli_steps.go:
- Around line 104-123: The code mixes two error-reporting mechanisms in
EnsureGatewayExists (function EnsureGatewayExists) by checking both the returned
error from RunGatewayAdd and s.state.GetExitCode()/GetCombinedOutput;
standardize on returning errors from RunGatewayAdd: update RunGatewayAdd to
return a non-nil error when the CLI exits non-zero (including the combined
output in the error message), then simplify EnsureGatewayExists to only handle
the returned error (remove the s.state.GetExitCode() check and subsequent
GetCombinedOutput() logic) and, on success, call s.state.SetGatewayInfo(name,
server).
🧹 Nitpick comments (4)
cli/it/steps/cli_steps.go (4)
86-102: Consider extracting hardcoded test credentials and simplifying success checks.Two observations:
Lines 92-94: The hardcoded credentials
"admin"/"admin"should be extracted as constants or configurable values for better maintainability and to make test assumptions explicit.Lines 98-100: The success check pattern
err == nil && s.state.GetExitCode() == 0is redundant. IfExecuteCLIreturns an error, checking the exit code is unnecessary. Consider a consistent pattern: either return errors fromExecuteCLIthat encapsulate exit code failures, or check only the exit code.♻️ Suggested improvements
Extract credentials as constants at package level:
const ( defaultTestUsername = "admin" defaultTestPassword = "admin" )Simplify the success check (assuming ExecuteCLI returns error for non-zero exit):
- err := s.state.ExecuteCLI(args...) - if err == nil && s.state.GetExitCode() == 0 { + if err := s.state.ExecuteCLI(args...); err == nil { s.state.SetGatewayInfo(name, server) } return err
163-173: Consider making Docker build parameters configurable.The Docker registry (
localhost:5000) and image tag (test-v1.0.0) are hardcoded. If different test scenarios require different registries or tags, consider making these configurable via the TestState or as method parameters.
240-262: Consider extracting configuration constants.The CLI config directory name (
.wso2ap), config filename (config.yaml), and empty config template are hardcoded. Extracting these as package-level or shared constants would improve maintainability and ensure consistency across the test suite.Example constant extraction
const ( cliConfigDirName = ".wso2ap" configFileName = "config.yaml" emptyConfigTemplate = "# WSO2 API Platform CLI Configuration\ngateways: []\n" )
264-282: Simplify path resolution logic.Lines 268-269 contain redundant logic: when
filepath.IsAbs(filePath)is true, the code setsabsPath = filePath, butabsPathis already initialized tofilePathat line 267.♻️ Simplified version
func (s *CLISteps) ApplyResourceFile(filePath string) error { - // Convert relative resource paths to absolute paths - absPath := filePath - if filepath.IsAbs(filePath) { - absPath = filePath - } else if strings.HasPrefix(filePath, "resources/gateway/") { + var absPath string + var err error + + if strings.HasPrefix(filePath, "resources/gateway/") { // Extract the filename from the relative path filename := filepath.Base(filePath) absPath = resources.GetResourcePath(filename) + } else if filepath.IsAbs(filePath) { + absPath = filePath } else { - var err error absPath, err = filepath.Abs(filePath) if err != nil { return fmt.Errorf("failed to resolve file path: %w", err) } } return s.state.ExecuteCLI("gateway", "apply", "-f", absPath) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/it/steps/cli_steps.go
🧰 Additional context used
🧬 Code graph analysis (1)
cli/it/steps/cli_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/resources/values.go (3)
GetResourcePath(118-122)GatewayControllerURL(30-30)GetSampleAPIPath(125-127)
🔇 Additional comments (2)
cli/it/steps/cli_steps.go (2)
19-42: LGTM!The package structure and TestState interface are well-designed. The interface provides a clear contract for CLI execution and state management.
70-84: The path resolution design is intentional and appropriate for the current architecture.GetResourcePath()is designed to accept only filenames, and all resource files incli/it/resources/gateway/are flat with no subdirectories. The use offilepath.Base()is consistent with this design. No nested resource paths are used anywhere in the codebase, so this is not an actual issue.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/it/steps/cli_steps.go:
- Around line 167-176: The BuildGatewayWithManifest function hardcodes the
Docker registry and image tag; change it to use configurable values (e.g., from
cli/it/resources/values.go or the test config loader) instead of
"localhost:5000" and "test-v1.0.0". Update BuildGatewayWithManifest to accept or
read TestDockerRegistry and TestImageTag (or load from the YAML config) and pass
those variables into s.state.ExecuteCLI, and add the constants or config keys
(TestDockerRegistry, TestImageTag) to the resources/values.go or the test config
reader so CI and different environments can override them.
🧹 Nitpick comments (6)
cli/it/steps/cli_steps.go (6)
71-84: Consider extracting path resolution logic to reduce duplication.The path resolution logic here (lines 75-81) is duplicated in
ApplyResourceFile(lines 273-276). Extract this into a helper method to improve maintainability.♻️ Proposed refactor
Add a helper method:
// resolveResourcePath resolves resource paths to absolute paths func (s *CLISteps) resolveResourcePath(path string) string { if strings.HasPrefix(path, "resources/gateway/") { filename := filepath.Base(path) return resources.GetResourcePath(filename) } return path }Then simplify both methods:
func (s *CLISteps) RunWithArgs(args string) error { parts := strings.Fields(args) - // Resolve resource paths in arguments for i, part := range parts { - if strings.HasPrefix(part, "resources/gateway/") { - // Extract filename and get absolute path - filename := filepath.Base(part) - parts[i] = resources.GetResourcePath(filename) - } + parts[i] = s.resolveResourcePath(part) } return s.state.ExecuteCLI(parts...) }
92-93: Consider extracting test credentials to constants.The hardcoded credentials "admin"/"admin" appear in the code. While acceptable for test code, extracting these to named constants in the resources package would improve maintainability and make the test configuration more explicit.
♻️ Suggested improvement
In
cli/it/resources/values.go, add:const ( TestUsername = "admin" TestPassword = "admin" )Then update this code:
- args = append(args, "--username", "admin", "--password", "admin", "--no-interactive") + args = append(args, "--username", resources.TestUsername, "--password", resources.TestPassword, "--no-interactive")
110-122: Simplify error handling logic.The error handling flow checks
err == nil && exitCode == 0, thenerr != nil, then checks exitCode again. This can be streamlined for better readability.♻️ Proposed simplification
- if err == nil && s.state.GetExitCode() == 0 { + if err != nil { + return err + } + + if s.state.GetExitCode() == 0 { s.state.SetGatewayInfo(name, server) return nil } - if err != nil { - return err - } - - // Command returned non-zero exit code; allow duplicate gateway error + // Allow duplicate gateway error if !strings.Contains(s.state.GetCombinedOutput(), "already exists") { return fmt.Errorf("failed to ensure gateway exists: %s", s.state.GetCombinedOutput()) }
232-237: Simplify error handling (similar to EnsureGatewayExists).This error handling pattern is nearly identical to lines 115-122. The logic can be streamlined for consistency and clarity.
♻️ Proposed simplification
- // If gateway already exists, update it - if s.state.GetExitCode() != 0 { - if !strings.Contains(s.state.GetCombinedOutput(), "already exists") { - return fmt.Errorf("failed to create gateway: %s", s.state.GetCombinedOutput()) - } + // Allow duplicate gateway error + if s.state.GetExitCode() != 0 && !strings.Contains(s.state.GetCombinedOutput(), "already exists") { + return fmt.Errorf("failed to create gateway: %s", s.state.GetCombinedOutput()) }
260-260: Extract hardcoded config template to a constant.The empty config structure is hardcoded as a string literal. Extracting it to a package constant improves maintainability.
♻️ Suggested improvement
At the package level:
const emptyConfigTemplate = "# WSO2 API Platform CLI Configuration\ngateways: []\n"Then use it:
- emptyConfig := "# WSO2 API Platform CLI Configuration\ngateways: []\n" + emptyConfig := emptyConfigTemplate if err := os.WriteFile(configFile, []byte(emptyConfig), 0600); err != nil {
268-285: Remove redundant conditional check.Lines 271-272 check
if filepath.IsAbs(filePath)and assignabsPath = filePath, butabsPathis already initialized tofilePathon line 270. This check is redundant.♻️ Proposed fix
func (s *CLISteps) ApplyResourceFile(filePath string) error { - // Convert relative resource paths to absolute paths - absPath := filePath - if filepath.IsAbs(filePath) { - absPath = filePath - } else if strings.HasPrefix(filePath, "resources/gateway/") { + var absPath string + if strings.HasPrefix(filePath, "resources/gateway/") { // Extract the filename from the relative path filename := filepath.Base(filePath) absPath = resources.GetResourcePath(filename) + } else if filepath.IsAbs(filePath) { + absPath = filePath } else { var err error absPath, err = filepath.Abs(filePath)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/it/steps/cli_steps.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
cli/it/steps/cli_steps.go
🧬 Code graph analysis (1)
cli/it/steps/cli_steps.go (2)
cli/it/state.go (1)
TestState(33-69)cli/it/resources/values.go (5)
GetResourcePath(118-122)GatewayControllerURL(30-30)GetSampleAPIPath(125-127)TestAPIName(75-75)TestAPIVersion(78-78)
|
1 participant
Purpose
$Subject
Summary by CodeRabbit
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.