Changed e2e to use the API #142
Conversation
- Changed the e2e tests to use the API instead of pkg for interactions - Changed the e2e tests also to use sqlite instead of redis Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
WalkthroughThe pull request introduces modifications across several files, focusing on the handling of storage types and the refactoring of test setups. In Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
... and 2 files with indirect coverage changes 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (13)
pkg/storages/storage.go (2)
18-18: Fix incorrect function documentationThe function comment incorrectly mentions Redis instead of SQL.
-// SetupRedisTestDB initializes a new SQLStorage with the given DSN. +// SetupSQLTestDB initializes a new SQLStorage with the given DSN.
18-34: Consider adding storage interface validationThe addition of both setup functions aligns well with the transition from Redis to SQLite. To ensure consistency across storage implementations:
- Consider adding interface compliance checks in tests
- Document any behavioral differences between Redis and SQLite implementations that might affect e2e tests
Add this test to validate interface compliance:
var ( _ Storage = (*SQLStorage)(nil) _ Storage = (*RedisStorage)(nil) )🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-23: pkg/storages/storage.go#L22-L23
Added lines #L22 - L23 were not covered by testscmd/server/server.go (1)
Line range hint
74-89: Consider refactoring validation logic for better maintainabilityWhile the validation logic is thorough, consider breaking it down into separate validation functions for each storage type to improve maintainability and testability.
Here's a suggested refactor:
func (o *options) PersistentPreRunE(_ *cobra.Command, _ []string) error { + if err := o.validateStorageType(); err != nil { + return err + } + return o.validateStorageConfig() +} + +func (o *options) validateStorageType() error { if o.StorageType != redisStorageType && o.StorageType != sqliteStorageType { return fmt.Errorf("invalid storage-type %q: must be one of [redis, sqlite]", o.StorageType) } + return nil +} + +func (o *options) validateStorageConfig() error { if o.StorageType == sqliteStorageType && o.StoragePath == "" { if !o.UseInMemory { return fmt.Errorf("storage-path is required when using SQLite with file-based storage") } } - if o.StorageType == redisStorageType && o.StorageAddr == "" { return fmt.Errorf("storage-addr is required when using Redis (format: host:port)") } - return nil }pkg/storages/redis_storage_test.go (3)
Line range hint
184-211: Enhance error simulation test coverageThe error handling test at the end of
TestGetNodesByGlobis good, but it could be more comprehensive.Consider adding more error scenarios:
func TestGetNodesByGlob(t *testing.T) { r := SetupRedisTestDB("localhost:6379") // ... existing test code ... - // Simulate an error by closing the Redis client - r.Client.Close() - _, err = r.GetNodesByGlob("test_*") - assert.Error(t, err) + t.Run("connection errors", func(t *testing.T) { + // Simulate various error scenarios + r.Client.Close() + + // Test all Redis operations after connection close + _, err := r.GetNodesByGlob("test_*") + assert.Error(t, err) + + _, err = r.GetNode(1) + assert.Error(t, err) + + err = r.SaveNode(node1) + assert.Error(t, err) + }) }
Line range hint
1-211: Consider adding parallel test executionThe tests are well-structured but could benefit from parallel execution to reduce test runtime.
Add parallel test execution where appropriate:
func TestGenerateID(t *testing.T) { + t.Parallel() r := SetupRedisTestDB(getTestRedisAddr()) // ... rest of the test }Note: Ensure proper test isolation and cleanup when enabling parallel execution.
Redis connections need proper cleanup in tests
The verification reveals several issues with Redis test management:
- Each test creates a new Redis connection via
SetupRedisTestDBbut never closes it- While
FlushDBis called during setup, there's no consistent cleanup after tests- No test suite setup/teardown methods (
BeforeTest/AfterTest) to manage connectionsRecommendations:
- Add
defer r.Client.Close()after Redis setup in each test- Consider implementing test suite setup/teardown to manage shared Redis instance
- Add cleanup in a
TestMain(m *testing.M)to ensure connections are properly closed🔗 Analysis chain
Line range hint
1-211: Verify test isolation and Redis usage patternsLet's ensure these tests don't conflict with other tests and that Redis resources are properly managed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other test files using Redis and potential conflicts # Look for other Redis test files echo "Checking for other Redis test files..." rg -l "SetupRedisTestDB|setupTestRedis" --type go # Check for Redis cleanup patterns echo "Checking for Redis cleanup patterns..." rg -A 5 "func Test.*" --type go | rg "defer.*Clean|defer.*Close" # Look for parallel test execution echo "Checking for parallel test execution..." rg "t.Parallel" --type goLength of output: 543
Script:
#!/bin/bash # Let's check the SetupRedisTestDB implementation and Redis connection management # Check SetupRedisTestDB implementation echo "Checking SetupRedisTestDB implementation..." rg -A 10 "func SetupRedisTestDB" --type go # Check for Redis connection cleanup echo "Checking Redis connection cleanup..." ast-grep --pattern 'func SetupRedisTestDB($_) $_ { $$$ }' # Check for test cleanup/teardown patterns echo "Checking test cleanup patterns..." rg "func Test.*|AfterTest|BeforeTest|TearDown" pkg/storages/redis_storage_test.goLength of output: 1725
pkg/storages/sql_test.go (6)
Line range hint
14-85: Maintain consistent test naming conventionThe test function names have inconsistent prefixes:
TestSQLGenerateID_InMemoryvsTestGenerateID_FileBased. Consider renaming for consistency.Apply this change for consistency:
-func TestGenerateID_FileBased(t *testing.T) { +func TestSQLGenerateID_FileBased(t *testing.T) {
Line range hint
89-165: Consider adding error scenario testsThe node operation tests cover the happy path well, but consider adding tests for:
- Duplicate node names
- Non-existent nodes
- Invalid node IDs
- Concurrent node operations
Would you like me to help generate these additional test cases?
Line range hint
166-263: Optimize test setup and reduce duplicationConsider using test tables or subtests to reduce setup duplication across cache-related tests. This would make the tests more maintainable and easier to extend.
Example refactor:
func TestSQLCacheOperations(t *testing.T) { tests := []struct { name string fn func(*testing.T, *SQLStorage) }{ {"SaveCache", testSQLSaveCache}, {"SaveCaches", testSQLSaveCaches}, {"GetCaches", testSQLGetCaches}, {"RemoveAllCaches", testSQLRemoveAllCaches}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s, err := SetupSQLTestDB("file::memory:") if err != nil { t.Fatalf("Setup failed: %v", err) } tt.fn(t, s) }) } }
Line range hint
264-271: Test appears incompleteThe test only verifies that an error occurs but doesn't test the successful case of adding and retrieving data. Consider:
- Testing successful data addition
- Verifying data retrieval
- Testing data updates
- Testing invalid cases (null data, empty keys)
Would you like me to help implement a complete test suite for this functionality?
Line range hint
273-293: Enhance glob pattern test coverageConsider adding test cases for:
- Special characters in node names
- Multiple wildcards
- Case sensitivity
- Empty or invalid patterns
Example additional test cases:
// Test cases to add: nodes, err = s.GetNodesByGlob("*[0-9]*") // Numbers in name nodes, err = s.GetNodesByGlob("NODE*") // Case sensitivity nodes, err = s.GetNodesByGlob("no*e*") // Multiple wildcards nodes, err = s.GetNodesByGlob("") // Empty pattern
Line range hint
1-293: Consider creating a test helper packageThe test file has significant setup duplication. Consider:
- Creating a test helper package with common setup and teardown functions
- Implementing test fixtures for common test data
- Using a test suite structure (e.g., using
testify/suite) to share setup codeThis would improve:
- Test maintainability
- Setup consistency
- Test data management
- Error handling patterns
e2e/e2e_test.go (1)
74-75: Avoid reusing the variablereqfor different requestsThe variable
reqis reused for different requests (IngestSBOM,IngestVulnerability,Cache), which might reduce code readability. Consider using descriptive variable names for each request to enhance clarity.Apply this diff to rename variables for clarity:
- req := connect.NewRequest(&service.IngestSBOMRequest{ + sbomReq := connect.NewRequest(&service.IngestSBOMRequest{ Sbom: data, }) - _, err = s.IngestSBOM(context.Background(), req) + _, err = s.IngestSBOM(context.Background(), sbomReq) ... - req := connect.NewRequest(&service.IngestVulnerabilityRequest{ + vulnReq := connect.NewRequest(&service.IngestVulnerabilityRequest{ Vulnerability: data, }) - _, err = s.IngestVulnerability(context.Background(), req) + _, err = s.IngestVulnerability(context.Background(), vulnReq) ... - req := connect.NewRequest(&emptypb.Empty{}) + cacheReq := connect.NewRequest(&emptypb.Empty{}) - _, err = s.Cache(context.Background(), req) + _, err = s.Cache(context.Background(), cacheReq)Also applies to: 46-50, 64-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cmd/server/server.go(3 hunks)e2e/e2e_test.go(5 hunks)pkg/storages/redis_storage_test.go(12 hunks)pkg/storages/sql_test.go(15 hunks)pkg/storages/storage.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/server/server.go
[warning] 60-60: cmd/server/server.go#L60
Added line #L60 was not covered by tests
pkg/storages/storage.go
[warning] 22-23: pkg/storages/storage.go#L22-L23
Added lines #L22 - L23 were not covered by tests
🔇 Additional comments (5)
pkg/storages/storage.go (2)
3-8: LGTM: Import statements are correctly structured
The new imports are necessary for the added functionality and are properly organized.
19-25: Add test coverage for error handling
The function implementation looks good with proper error wrapping, but the error handling path needs test coverage.
Let's verify the current test coverage:
Would you like me to help generate test cases for the error handling scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-23: pkg/storages/storage.go#L22-L23
Added lines #L22 - L23 were not covered by tests
cmd/server/server.go (1)
43-44: Verify impact of changing default storage type
Changing the default storage type from Redis to SQLite could impact existing deployments. Consider:
- Adding a note in the documentation about this breaking change
- Clarifying in release notes that users need to explicitly set
--storage-type=redisif they want to continue using Redis
✅ Verification successful
No documentation update needed for storage type change
The change of default storage type from Redis to SQLite is safe and well-handled in the code:
- The code has proper validation that requires explicit configuration for both storage types:
- For Redis: Requires valid
storage-addrwhen Redis is selected - For SQLite: Requires valid
storage-pathwhen file-based storage is used
- For Redis: Requires valid
- The help text for the
--storage-typeflag already lists both options: "Type of storage to use (e.g., redis, sqlite)" - The error messages are clear and guide users on proper configuration for both storage types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any deployment configs or docs that might need updates
fd -e yaml -e md | xargs rg -l "storage-type|redis|sqlite"
Length of output: 60
Script:
#!/bin/bash
# Let's try a different approach to find documentation and configuration files
# First, check README and other common documentation files
find . -maxdepth 2 -type f -name "README*" -o -name "CHANGELOG*" -o -name "*.md" | xargs cat
# Then check for any deployment or configuration files
find . -type f -name "*.yaml" -o -name "*.yml" -o -name "docker-compose*" | xargs cat
# Also check for any environment or configuration examples
find . -type f -name "*.env*" -o -name "config*.json" | xargs cat
Length of output: 36466
Script:
#!/bin/bash
# Let's check for any existing Redis usage in the codebase
rg -l "redis|Redis" --type go
# Also check for any storage type related code
ast-grep --pattern 'var $_ = $STORAGE_TYPE' --lang go
Length of output: 23643
Script:
#!/bin/bash
# Let's check for any default storage type references and their usage
rg -A 2 -B 2 "sqliteStorageType|redisStorageType" --type go
# Also check for any storage type related configuration or documentation
rg -A 3 "storage-type|storage-addr" --type go
Length of output: 4864
e2e/e2e_test.go (2)
1-1: Package name change to e2e is appropriate
The package name has been changed to e2e, which accurately reflects the context of the end-to-end tests.
19-21: Confirm intentional removal of environment check for E2E tests
The environment check that conditionally skips the E2E tests has been commented out. This change will cause the E2E tests to run unconditionally. Ensure that this is intentional and that it won't impact test execution time or CI/CD pipelines.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
pkg/storages/storage.go (2)
19-25: Consider adding database cleanup and improving documentationA few suggestions to improve the function:
- Add database cleanup similar to Redis's FlushDB to ensure a clean test state
- Document the purpose of the
falseparameter inNewSQLStorage(dsn, false)Consider this improved implementation:
// SetupSQLTestDB initializes a new SQLStorage with the given DSN. +// The boolean parameter controls whether to enable foreign key constraints. func SetupSQLTestDB(dsn string) (*SQLStorage, error) { storage, err := NewSQLStorage(dsn, false) if err != nil { return nil, fmt.Errorf("failed to initialize SQLStorage: %w", err) } + // Clean up existing data + if err := storage.Clear(context.Background()); err != nil { + return nil, fmt.Errorf("failed to clear SQL database: %w", err) + } return storage, nil }
18-44: Consider moving test setup functions to a separate test fileThese setup functions are specifically for testing purposes. Consider moving them to a separate test file (e.g.,
storage_test.go) to maintain a clear separation between production and test code.e2e/e2e_test.go (3)
Line range hint
31-73: Improve error handling consistency and reduce code duplicationThe SBOM and vulnerability processing sections have similar patterns that could be refactored for better maintainability.
Consider applying this refactor to improve the code:
+func processFiles(t *testing.T, dir string, process func([]byte) (*connect.Response, error)) error { + files, err := os.ReadDir(dir) + if err != nil { + return err + } + + for _, file := range files { + if !strings.HasSuffix(file.Name(), ".json") { + continue + } + + data, err := os.ReadFile(filepath.Join(dir, file.Name())) + if err != nil { + return fmt.Errorf("failed to read file %s: %v", file.Name(), err) + } + + if _, err := process(data); err != nil { + return fmt.Errorf("failed to process file %s: %v", file.Name(), err) + } + } + return nil +} - // Process SBOM files - sbomFiles, err := os.ReadDir(sbomPath) - assert.NoError(t, err) - - for _, file := range sbomFiles { - if !strings.HasSuffix(file.Name(), ".json") { - continue - } - - data, err := os.ReadFile(filepath.Join(sbomPath, file.Name())) - assert.NoError(t, err) - - req := connect.NewRequest(&service.IngestSBOMRequest{ - Sbom: data, - }) - _, err = s.IngestSBOM(context.Background(), req) - assert.NoError(t, err) - } + err = processFiles(t, sbomPath, func(data []byte) (*connect.Response, error) { + req := connect.NewRequest(&service.IngestSBOMRequest{ + Sbom: data, + }) + return s.IngestSBOM(context.Background(), req) + }) + assert.NoError(t, err) - // Process vulnerability files - vulnFiles, err := os.ReadDir(vulnsPath) - assert.NoError(t, err) - - for _, file := range vulnFiles { - if !strings.HasSuffix(file.Name(), ".json") { - continue - } - - data, err := os.ReadFile(filepath.Join(vulnsPath, file.Name())) - assert.NoError(t, err) - - req := connect.NewRequest(&service.IngestVulnerabilityRequest{ - Vulnerability: data, - }) - _, err = s.IngestVulnerability(context.Background(), req) - if err != nil { - t.Fatalf("Failed to load vulnerabilities from file %s: %v", file.Name(), err) - } - } + err = processFiles(t, vulnsPath, func(data []byte) (*connect.Response, error) { + req := connect.NewRequest(&service.IngestVulnerabilityRequest{ + Vulnerability: data, + }) + return s.IngestVulnerability(context.Background(), req) + }) + assert.NoError(t, err)
Line range hint
77-173: Enhance test case documentationWhile the test cases are comprehensive, some test cases would benefit from better documentation explaining the expected behavior and test coverage.
Consider adding comments to explain complex test cases, particularly for:
- The complex nested expressions test case
- The query with vulnerability test case
- The test case with default node name
Example:
{ + // Tests that complex nested expressions with multiple operations + // are correctly parsed and executed name: "Complex nested expressions", script: "(dependents library pkg:github/actions/checkout@v3 and dependents library pkg:golang/gopkg.in/yaml.v3@v3.0.1) or dependents library pkg:golang/gopkg.in/yaml.v3@v3.0.1", want: 8, defaultNodeName: "", wantErr: false, },
174-209: Improve consistency in error messagesThe error messages between the query and leaderboard paths have different formats. Consider standardizing them for better readability and maintenance.
Apply this diff to standardize error messages:
- t.Errorf("CustomLeaderboard() got the first nodes output len of = %v, want output len of %v", nodes[0].Output, tt.want) + t.Errorf("CustomLeaderboard() got output length = %v, want %v", len(nodes[0].Output), tt.want) - t.Errorf("Query() got cardinality = %v, want cardinality %v", len(nodes), tt.want) + t.Errorf("Query() got length = %v, want %v", len(nodes), tt.want)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
e2e/e2e_test.go(5 hunks)pkg/storages/storage.go(1 hunks)
🔇 Additional comments (5)
pkg/storages/storage.go (1)
27-44: LGTM! Well-structured implementation
The Redis setup function is well-implemented with:
- Proper context handling
- Connection verification
- Database cleanup
- Comprehensive error handling
e2e/e2e_test.go (4)
1-15: LGTM: Package and imports align with PR objectives
The package name change to e2e and the addition of Connect RPC and API imports appropriately reflect the transition from direct pkg usage to API-based testing.
22-27: ** Potential test interference due to shared database file**
A past review comment already identified this issue. The fixed database path could lead to race conditions in parallel tests. Consider using a unique temporary file for each test instance.
74-76: LGTM: Caching setup is correct
The caching implementation using Empty request is appropriate and well-handled.
152-157: ** defaultNodeName is not utilized in CustomLeaderboardRequest**
A past review comment already identified this issue. The defaultNodeName field is still not being used in the request.
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
pkg/storages/redis_storage_test.go (2)
22-25: Consider adding test cleanupWhile the setup and test logic are correct, consider adding
t.Cleanup()to ensure the test data is removed after the test completes. This prevents test pollution in case tests are run in parallel.func TestSaveNode(t *testing.T) { r, err := SetupRedisTestDB(context.Background(), "localhost:6379") assert.NoError(t, err) + t.Cleanup(func() { + r.Client.Del(context.Background(), "node:1") + r.Client.Del(context.Background(), "name:test_node") + }) node := &graph.Node{ID: 1, Name: "test_node", Children: roaring.New(), Parents: roaring.New()}
Line range hint
36-228: Well-structured test suite with comprehensive error handlingThe test suite demonstrates good practices:
- Consistent setup pattern across all tests
- Proper error handling for Redis operations
- Good coverage of error cases (especially in TestGetNodesByGlob)
However, consider these improvements:
- Add parallel test execution support using
t.Parallel()- Implement test cleanup for all tests to prevent test pollution
- Consider using test tables for tests with multiple cases (e.g., TestGetNodesByGlob)
e2e/e2e_test.go (5)
35-47: Consider using temporary file for SQLite database to prevent test interferenceUsing a fixed database file
test_e2e.dbmight lead to conflicts if tests are run in parallel or if the file isn't cleaned up properly. Consider creating a unique temporary file for each test instance.Apply this diff to create a unique temporary database file:
-testDBPath := "test_e2e.db" +testDBFile, err := os.CreateTemp("", "test_e2e_*.db") +if err != nil { + t.Fatal(err) +} +testDBPath := testDBFile.Name() +defer os.Remove(testDBPath) // Clean up after the test
20-22: Reconsider the environment variable check for enabling E2E testsThe environment check for E2E tests is currently active, which means these tests will only run when the
e2eenvironment variable is set. If the E2E tests are essential and not overly time-consuming, you might consider running them by default to ensure regular testing.
223-254: Simplify the conditionals in the test executionThe current structure uses
if tt.queryOrLeaderboard == trueto determine which test to run. Sincett.queryOrLeaderboardis a boolean, you can simplify the conditionals for clarity.Apply this diff to simplify the conditionals:
-if tt.queryOrLeaderboard == true { +if tt.queryOrLeaderboard {-} else { +} else if !tt.queryOrLeaderboard {
124-218: Explicitly setwantErrin all test cases for claritySome test cases do not explicitly set the
wantErrfield, relying on the default zero value (false). For better clarity and readability, consider explicitly settingwantErrtofalsein all test cases.
84-97: Handle file read errors consistentlyIn the SBOM processing loop, errors during file reading are handled using
assert.NoError(t, err), whereas in the vulnerability processing loop,t.Fatalfis used. For consistency and better error reporting, consider using the same error handling method in both loops.Apply this diff to use
t.Fatalffor consistency:-assert.NoError(t, err) +if err != nil { + t.Fatalf("Failed to read SBOM file %s: %v", file.Name(), err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Makefile(1 hunks)e2e/e2e_test.go(1 hunks)pkg/storages/redis_storage_test.go(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🔇 Additional comments (4)
pkg/storages/redis_storage_test.go (2)
14-15: LGTM: Proper error handling added for Redis setup
The setup code correctly handles potential Redis connection errors and uses an appropriate context.
14-14: Verify Redis test environment setup
Given the PR's objective to move from Redis to SQLite, verify that:
- Redis is available in the test environment
- CI/CD pipeline has Redis service configured
Also applies to: 22-22, 36-36, 48-48, 64-64, 76-76, 88-88, 103-103, 123-123, 140-140, 157-157, 176-176, 197-197
✅ Verification successful
Redis test environment is properly configured
The hardcoded Redis address "localhost:6379" is appropriate as:
- Redis service is properly configured in docker-compose.yml with the correct port mapping (6379:6379)
- SetupRedisTestDB is consistently used across test files (e2e_test.go, storage.go, redis_storage_test.go)
- Docker Compose configuration ensures Redis availability for local development and testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Redis configuration in CI environment
# Check if .github/workflows exists
if [ -d ".github/workflows" ]; then
echo "Checking CI workflows for Redis service..."
rg -l "redis" .github/workflows/
fi
# Check for docker-compose configurations
if [ -f "docker-compose.yml" ]; then
echo "Checking docker-compose for Redis service..."
rg -l "redis" docker-compose.yml
fi
# Check for test setup documentation
if [ -f "README.md" ]; then
echo "Checking documentation for test setup instructions..."
rg -A 5 -i "test.*setup|redis.*test" README.md
fi
Length of output: 530
Script:
#!/bin/bash
# Let's check the content of docker-compose.yml for Redis configuration
echo "Checking docker-compose.yml content:"
cat docker-compose.yml
# Let's also check for Redis test setup in the test files
echo -e "\nChecking Redis test setup files:"
rg -l "SetupRedisTestDB"
# Look for Redis environment variables or configuration
echo -e "\nChecking for Redis configuration:"
rg -A 3 "REDIS_"
Length of output: 539
e2e/e2e_test.go (2)
19-260: Well-structured end-to-end tests with thorough coverage
The Test_E2E function is comprehensive, testing multiple storage backends and various query scenarios. The implementation demonstrates a good understanding of the testing requirements.
233-238:
Prevent possible index out of range panic in CustomLeaderboard test
When accessing nodes[0].Output, there's a risk of an index out of range panic if nodes is empty. Ensure that the length of nodes is checked before accessing elements.
Apply this diff to add the length check:
if (err != nil) != tt.wantErr {
t.Errorf("CustomLeaderboard() error = %v, wantErr %v", err, tt.wantErr)
return
}
+if len(nodes) == 0 {
+ t.Errorf("CustomLeaderboard() returned no queries, expected at least one")
+ return
+}
if !tt.wantErr && len(nodes[0].Output) != int(tt.want) {
t.Errorf("CustomLeaderboard() got the first nodes output len of = %v, want output len of %v", len(nodes[0].Output), tt.want)
}Likely invalid or redundant comment.
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
neilnaveen
force-pushed
the
neil/e2eforserver
branch
from
219539b
to
45c8dcc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
e2e/e2e_test.go (3)
81-82: Standardize error handling approachThe code uses both
assert.NoErrorandt.Fatalfor error handling. This inconsistency makes the code harder to maintain. Standardize on one approach.Consider using
assertconsistently throughout the test:- if err != nil { - t.Fatalf("Failed to load vulnerabilities from file %s: %v", file.Name(), err) - } + assert.NoErrorf(t, err, "Failed to load vulnerabilities from file %s", file.Name())Also applies to: 113-116
124-218: Improve test case organization and documentationThe test cases mix different types of queries and contain magic numbers. Consider:
- Grouping test cases by query type (dependencies, dependents, vulnerabilities)
- Adding constants for commonly used test values
- Adding comments explaining the expected values
Example refactor:
+ // Test case groups + const ( + expectedDependentsCount = 6 + expectedDependenciesCount = 1 + complexQueryResultCount = 950 // High count due to extensive dependency tree + ) + + // Group test cases by type + dependentTests := []struct{...}{...} + dependencyTests := []struct{...}{...} + vulnerabilityTests := []struct{...}{...} + invalidTests := []struct{...}{...}
241-254: Reduce code duplication in test assertionsThe error handling and assertion logic is duplicated between Query and CustomLeaderboard cases. Consider extracting common validation logic.
Example refactor:
+ // Helper function for validating response + validateResponse := func(got, want int, err, wantErr error) { + if (err != nil) != tt.wantErr { + t.Errorf("got error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && got != int(want) { + t.Errorf("got count = %v, want count %v", got, want) + } + } + if tt.queryOrLeaderboard { // ... CustomLeaderboard case ... - if (err != nil) != tt.wantErr { - t.Errorf("CustomLeaderboard() error = %v, wantErr %v", err, tt.wantErr) - return - } + validateResponse(len(nodes[0].Output), int(tt.want), err, tt.wantErr) } else { // ... Query case ... - if (err != nil) != tt.wantErr { - t.Errorf("Query() error = %v, wantErr %v", err, tt.wantErr) - return - } + validateResponse(len(nodes), int(tt.want), err, tt.wantErr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
e2e/e2e_test.go(1 hunks)pkg/storages/storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/storages/storage.go
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/storages/storage.go (1)
28-29: Improve function documentationThe function comment should document the context parameter and the TEST_REDIS_URL environment variable.
-// SetupRedisTestDB initializes a new RedisStorage with the given address. +// SetupRedisTestDB initializes a new RedisStorage using the TEST_REDIS_URL environment +// variable (defaults to "localhost:6379" if not set). It verifies the connection +// and flushes the database before returning. +// +// The provided context is used for connection verification and database operations.pkg/storages/redis_storage_test.go (1)
Line range hint
14-204: Consider introducing test helpers to reduce setup boilerplateWhile the current implementation is correct, there's significant setup boilerplate across all tests. Consider introducing a test helper function to encapsulate the common setup pattern:
func setupTestRedisWithContext(t *testing.T) *RedisStorage { r, err := SetupRedisTestDB(context.Background()) assert.NoError(t, err) return r }This would reduce duplication and make tests more concise while maintaining the same level of error checking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
e2e/e2e_test.go(1 hunks)pkg/storages/redis_storage_test.go(12 hunks)pkg/storages/storage.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/e2e_test.go
🔇 Additional comments (7)
pkg/storages/storage.go (3)
3-9: LGTM!
The imports are well-organized and all are utilized in the code.
19-26: LGTM!
The function is well-implemented with proper error handling and follows good practices.
28-51: Verify if Redis setup is still needed
Given that the PR objectives mention moving from Redis to SQLite, verify if this Redis setup function is still required or should be removed.
pkg/storages/redis_storage_test.go (4)
14-16: LGTM: Improved error handling in test setup
The new setup properly handles potential Redis connection errors and uses context appropriately.
23-24: LGTM: Consistent error handling pattern
The setup follows the established pattern and properly separates setup errors from test operation errors.
Also applies to: 26-26
49-50: LGTM: Well-structured error handling for multiple operations
The setup properly handles errors for both the initial connection and subsequent node operations.
Also applies to: 53-53
198-204: LGTM: Comprehensive test coverage
The test thoroughly covers multiple scenarios including:
- Successful pattern matching
- No-match cases
- Error handling for connection failures
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
test-e2etarget.