Improve coverage for gateway builder and policy engine#945
Open
IsuruGunarathne wants to merge 28 commits intowso2:mainfrom
Open
Improve coverage for gateway builder and policy engine#945IsuruGunarathne wants to merge 28 commits intowso2:mainfrom
IsuruGunarathne wants to merge 28 commits intowso2:mainfrom
Conversation
IsuruGunarathne
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
Contributor
WalkthroughThis PR introduces comprehensive test coverage across gateway-builder and policy-engine by adding 35+ new test files and creating shared testutils infrastructure with helpers for file I/O, Go module configuration, policy definitions, and mock policies. Existing tests are refactored to use the new helpers, reducing setup boilerplate and standardizing test patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@gateway/gateway-builder/go.mod`:
- Around line 5-15: The indirect dependency github.com/pmezard/go-difflib is
unmaintained—replace it with an actively maintained diff library (e.g.,
github.com/google/go-cmp) by updating the go.mod require list to include
github.com/google/go-cmp and removing github.com/pmezard/go-difflib, then run go
mod tidy; update any test files or utility code that import or call
pmezard/go-difflib (search for imports or calls to "difflib" or methods used
from github.com/pmezard/go-difflib) to use the corresponding google/go-cmp APIs
(e.g., cmp.Diff) or an equivalent maintained API so tests compile and behavior
is preserved.
In `@gateway/gateway-builder/internal/compilation/compiler_test.go`:
- Around line 286-298: The lookupPath function misnames the second return from
os.LookupEnv as err even though LookupEnv returns (string, bool); rename that
variable to a boolean like ok or found and update the conditional from !err to
!ok to make the intent clear (in function lookupPath, change the os.LookupEnv
return variable and its check), leaving the rest of the logic (iterating
filepath.SplitList and os.Stat) unchanged.
In `@gateway/gateway-builder/internal/discovery/discovery_test.go`:
- Around line 745-767: The test
TestValidateDirectoryStructure_UnreadableDirectory uses os.Chmod(testDir, 0000)
to make the directory unreadable but can produce false failures in privileged
environments; after applying os.Chmod, explicitly verify that the directory is
indeed unreadable (e.g., attempt to open or read a file/path under testDir) and
if the check still succeeds, call t.Skip to avoid a false failure; ensure you
reference the existing test identifiers
(TestValidateDirectoryStructure_UnreadableDirectory, testDir,
ValidateDirectoryStructure, os.Chmod) when adding the post-chmod readability
guard and the skip path, and keep the defer to restore permissions for cleanup.
In `@gateway/gateway-builder/internal/docker/client_test.go`:
- Around line 94-127: The custom Unix-only helpers lookupPath and splitPath
should be removed and replaced with Go's cross-platform exec.LookPath; update
any calls such as lookupPath("docker") in tests (e.g.,
TestCheckDockerAvailable_Success and TestExecuteDockerCommand_Version) to call
exec.LookPath("docker") instead, add the necessary import for "os/exec" (and
remove unused imports if any), and delete the lookupPath and splitPath functions
entirely so path lookup is handled correctly on all platforms (including .exe on
Windows).
- Around line 64-79: The test TestCheckDockerAvailable_Success currently uses a
weak assertion on errors from CheckDockerAvailable(); update it to explicitly
fail on unexpected errors: keep the lookupPath("docker") skip, call
CheckDockerAvailable(), and then if err != nil assert that the error text
contains expected, allowed substrings (e.g., "daemon" or a known permission
message) and call t.Fatalf (or t.FailNow) for any other error so unexpected
failures don't pass silently; reference the TestCheckDockerAvailable_Success
test and the CheckDockerAvailable function and add the "strings" import to
perform substring checks.
In `@gateway/gateway-builder/internal/packaging/packager_test.go`:
- Around line 305-334: The test TestGenerateDockerfile_BuildTimestampIsRecent
allows missing or unparsable timestamps to pass; update it so it fails when the
"Build timestamp: " line is missing or time.Parse on timestampStr returns an
error. After reading BUILD.md in the test, assert that timestampIdx and
timestampEnd are not -1, assert that timestampStr is non-empty, and
require.NoError(t, err) for the time.Parse result stored in parsedTime; then
keep the existing assertions that parsedTime is between beforeTime and
afterTime. Ensure references: TestGenerateDockerfile_BuildTimestampIsRecent,
GenerateDockerfile, BUILD.md, timestampStr, parsedTime.
- Around line 168-197: The BUILD.md template used by generateBuildInstructions
is prefixing p.Version with an extra literal "v" (producing "vv1.0.0"); remove
the hardcoded "v" in the template so it renders p.Version directly (use
{{.Version}} instead of v{{.Version}}) and then update the test expectations in
TestGenerateBuildInstructions_Success to assert "1. ratelimit v1.0.0" and "2.
jwt-auth v0.1.0" (and remove assertions that expect "vv...") so the test matches
the corrected single-v output.
In `@gateway/gateway-builder/internal/testutils/gomod.go`:
- Around line 41-83: The helper functions WriteGoModWithVersion,
WriteGoModWithRequire, and WriteGoModWithReplace assume the target dir exists;
create the parent directories before writing the go.mod (e.g., call
os.MkdirAll(dir, 0755) or reuse your existing WriteFile helper) so nested dirs
won't cause os.WriteFile to fail, then proceed to write the file and keep the
existing require.NoError checks; apply the same change in WritePolicyEngineGoMod
if it delegates to WriteGoMod or directly writes files.
- Around line 31-32: Update the DefaultGoVersion constant in the testutils gomod
generator by changing DefaultGoVersion from "1.23" to "1.25.1" so generated
go.mod files match the gateway-builder toolchain; edit the const
DefaultGoVersion declaration to use "1.25.1" and then rebuild the gateway
artifacts (run the gateway build-local target, e.g., cd gateway && make
build-local) to propagate the change.
In `@gateway/gateway-builder/internal/testutils/policy.go`:
- Around line 42-65: The tests generate invalid Go package names when policy
names contain hyphens because CreateValidPolicyDir (and
CreatePolicyWithDefinition) use fmt.Sprintf("package %s", name) directly; fix by
sanitizing the package name before writing the source (e.g., replace hyphens
with underscores or otherwise normalize to a valid identifier) and use that
sanitized value in the package declaration (update the package creation logic in
CreateValidPolicyDir and similarly in CreatePolicyWithDefinition or accept an
explicit packageName parameter and use it when writing the "package ..." line).
In `@gateway/gateway-builder/pkg/fsutil/path_validation_test.go`:
- Around line 182-247: The permission-based tests
(TestValidatePathExists_PermissionDenied, TestCopyFile_SourceUnreadable,
TestCopyFile_DestinationReadOnly) should guard against platforms/users that
ignore chmod by probing actual access after changing permissions and skipping if
access is still allowed; after calling os.Chmod(...) to make a file/dir
unreadable or write-protected, attempt a minimal access probe (e.g., os.Open or
os.Stat for the file you expect to be restricted, or try creating a file in the
directory for destination tests) and if the probe succeeds, call t.Skip with an
explanatory message so the subsequent call to ValidatePathExists or CopyFile is
only executed when the OS enforces the permission change.
In `@gateway/policy-engine/internal/metrics/server_test.go`:
- Around line 87-98: The test in server_test.go currently uses "if err == nil"
after http.Get calls which silently skips assertions when the request fails;
update the two http.Get calls (the metrics and health requests) to fail the test
when err != nil (e.g., use t.Fatalf/t.Helper or require.NoError) so connection
errors don't mask failures, ensure resp is non-nil before checking
resp.StatusCode and always close resp.Body (use defer resp.Body.Close()), and
optionally add a short retry/backoff loop around http.Get to handle slow CI
startup before asserting http.StatusOK.
🧹 Nitpick comments (11)
gateway/gateway-builder/internal/docker/client_test.go (2)
28-38: Uset.Setenvinstead ofos.Setenvfor safer environment manipulation.The
os.Setenv/deferpattern is not safe for parallel tests and can cause race conditions. Go 1.17+ providest.Setenvwhich automatically restores the original value and marks the test as incompatible witht.Parallel().♻️ Proposed fix
func TestCheckDockerAvailable_DockerNotInPath(t *testing.T) { - // Save original PATH and set to empty - originalPath := os.Getenv("PATH") - os.Setenv("PATH", "/nonexistent") - defer os.Setenv("PATH", originalPath) + // Set PATH to nonexistent directory (automatically restored after test) + t.Setenv("PATH", "/nonexistent") err := CheckDockerAvailable() assert.Error(t, err) assert.Contains(t, err.Error(), "docker command not found in PATH") }
40-50: Same issue: uset.Setenvfor environment variable manipulation.Apply the same fix as above for consistency and test safety.
♻️ Proposed fix
func TestExecuteDockerCommand_DockerNotInPath(t *testing.T) { - // Save original PATH and set to empty - originalPath := os.Getenv("PATH") - os.Setenv("PATH", "/nonexistent") - defer os.Setenv("PATH", originalPath) + // Set PATH to nonexistent directory (automatically restored after test) + t.Setenv("PATH", "/nonexistent") err := ExecuteDockerCommand("version") assert.Error(t, err) assert.Contains(t, err.Error(), "docker version failed") }gateway/policy-engine/internal/admin/server_test.go (3)
44-48: Bind the test listeners to loopback to avoid wide exposure warnings.Using
"127.0.0.1:0"keeps tests local and prevents lints about binding to all interfaces.♻️ Suggested change
- listener, err := net.Listen("tcp", ":0") + listener, err := net.Listen("tcp", "127.0.0.1:0")Also applies to: 123-127
52-70: NewServer setup test looks good.As per coding guidelines: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.
76-120: Consider replacing the fixed sleep with readiness polling.The fixed 100ms wait can be flaky on slower CI nodes; a short retry loop around the /config_dump probe will be more stable.
gateway/gateway-builder/internal/testutils/manifest.go (1)
58-82: Optional: guard against invalid policy entries.Consider asserting that exactly one of
FilePathorGoModuleis set to avoid generating ambiguous manifests in tests.gateway/gateway-builder/internal/testutils/gomod.go (1)
19-38: Reminder: rebuild gateway images after these gateway-component changes.Run
cd gateway && make build-localto keep local images in sync with updated gateway-builder code.
As per coding guidelines: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-builder/internal/discovery/discovery_test.go (1)
728-786: Consider making gomodule-resolution tests hermetic.
resolveModuleInfoshells out togo mod download -json, which can introduce flaky CI behavior in offline or restricted-network environments. While the tests intentionally use non-existent module names to verify error handling, making them hermetic by injecting a mock resolver or running them behind an integration tag would improve CI reliability and reduce external dependencies.gateway/gateway-builder/internal/manifest/manifest_test.go (1)
518-539: Inconsistent indentation in test function.The code in
TestManifest_ToJSON_Successlacks proper indentation, making it inconsistent with the rest of the file. While this doesn't affect functionality, it reduces readability.🔧 Suggested fix for indentation
func TestManifest_ToJSON_Success(t *testing.T) { -manifest := &Manifest{ -BuildTimestamp: "2025-01-01T00:00:00Z", -BuilderVersion: "v2.0.0", -OutputDir: "/output/dir", -Policies: []PolicyInfo{ -{Name: "test", Version: "v1.0.0"}, -}, -} - -jsonStr, err := manifest.ToJSON() -require.NoError(t, err) -assert.Contains(t, jsonStr, "v2.0.0") -assert.Contains(t, jsonStr, "test") -assert.Contains(t, jsonStr, "v1.0.0") - -// Verify it's valid JSON -var parsed Manifest -err = json.Unmarshal([]byte(jsonStr), &parsed) -require.NoError(t, err) -assert.Equal(t, manifest.BuilderVersion, parsed.BuilderVersion) + manifest := &Manifest{ + BuildTimestamp: "2025-01-01T00:00:00Z", + BuilderVersion: "v2.0.0", + OutputDir: "/output/dir", + Policies: []PolicyInfo{ + {Name: "test", Version: "v1.0.0"}, + }, + } + + jsonStr, err := manifest.ToJSON() + require.NoError(t, err) + assert.Contains(t, jsonStr, "v2.0.0") + assert.Contains(t, jsonStr, "test") + assert.Contains(t, jsonStr, "v1.0.0") + + // Verify it's valid JSON + var parsed Manifest + err = json.Unmarshal([]byte(jsonStr), &parsed) + require.NoError(t, err) + assert.Equal(t, manifest.BuilderVersion, parsed.BuilderVersion) }gateway/gateway-builder/internal/policyengine/policyengine_test.go (1)
496-508: Tests with invalid remote modules may be slow due to network calls.These tests (
TestUpdateGoMod_RemotePolicy_InvalidModuleandTestUpdateGoMod_OnlyRemotePolicies_AllFail) use non-existent Go modules which will cause actualgo getnetwork calls that may timeout or be slow in CI environments.Consider adding a short timeout or using a test flag to skip these in quick test runs if they become problematic.
Also applies to: 570-582
gateway/policy-engine/internal/testutils/helpers.go (1)
21-45: Pointer helpers are clean and practical for tests.
They keep optional-field setup concise. Also, remember to rebuild gateway Docker images after gateway component changes.As per coding guidelines, when modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.
…handling directory permissions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-builder/internal/testutils/filesystem.go`:
- Around line 30-41: SanitizePackageName currently only replaces '-' and '.' but
must replace every character not in [A-Za-z0-9_]; update the SanitizePackageName
function to iterate or use a regex to replace any byte that is not a letter,
digit, or underscore with '_' (so result matches [A-Za-z0-9_]+), then ensure the
first rune is a letter or underscore by prefixing '_' if it starts with a digit,
and finally handle the empty-result edge case (return a safe default like "_" or
"pkg") so the function never returns an invalid or empty package name.
🧹 Nitpick comments (3)
gateway/policy-engine/internal/metrics/server_test.go (2)
19-32: Rebuild gateway images after gateway changes.Please run
cd gateway && make build-localbefore any gateway-level Docker-based validation.As per coding guidelines, “When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local.”
68-125: Add an HTTP client timeout to avoid potential hangs.
http.Getuses a client without timeouts; a stalled connection can hang the test. Consider using a shared client with a timeout for all requests in this test.♻️ Suggested change
- // Wait for server to be ready with retries + client := &http.Client{Timeout: 2 * time.Second} + // Wait for server to be ready with retries var resp *http.Response var err error for i := 0; i < 10; i++ { time.Sleep(50 * time.Millisecond) - resp, err = http.Get("http://localhost:9101/health") + resp, err = client.Get("http://localhost:9101/health") if err == nil { resp.Body.Close() break } } @@ - resp, err = http.Get("http://localhost:9101/metrics") + resp, err = client.Get("http://localhost:9101/metrics") require.NoError(t, err, "metrics endpoint should be reachable") assert.Equal(t, http.StatusOK, resp.StatusCode) resp.Body.Close() @@ - resp, err = http.Get("http://localhost:9101/health") + resp, err = client.Get("http://localhost:9101/health") require.NoError(t, err, "health endpoint should be reachable") assert.Equal(t, http.StatusOK, resp.StatusCode) resp.Body.Close()gateway/gateway-builder/internal/testutils/filesystem.go (1)
89-97: Sanitize packageName before writing the source file.Other helpers already sanitize package names; this one should align to avoid invalid package declarations.
♻️ Suggested change
func CreateGoSourceWithImport(t *testing.T, dir, packageName, importPath string) { t.Helper() - content := `package ` + packageName + ` + safeName := SanitizePackageName(packageName) + content := `package ` + safeName + ` import _ "` + importPath + `" ` - CreateSourceFile(t, dir, packageName+".go", content) + CreateSourceFile(t, dir, safeName+".go", content) }
…larity and error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@gateway/gateway-builder/internal/packaging/packager_test.go`:
- Around line 108-118: The test TestGenerateDockerfile_InvalidOutputDir uses a
hardcoded absolute path which is flaky; change it to create a temporary file
(os.CreateTemp / ioutil.TempFile) and pass that file's path as the outputDir to
force a deterministic “not a directory” error, then clean up the temp file;
apply the same fix to the other invalid-path test (the one around lines 219-229)
so both tests use a temp file path instead of a hardcoded directory path when
calling GenerateDockerfile.
🧹 Nitpick comments (1)
gateway/gateway-builder/internal/packaging/packager_test.go (1)
33-54: Reminder: rebuild gateway images after gateway component changes.Please rebuild with
cd gateway && make build-localbefore running any integration tests. As per coding guidelines: “When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.”
… and sanitizing package names
…dated request structure
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.
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit
Bug Fixes
Tests
Chores