test(reporting/exporters/mongo): add mongo integration test with test… #6237
Conversation
WalkthroughA new integration test for MongoDB exporter functionality was added, involving the orchestration of a MongoDB container, data export, and verification of persistence. The test is registered under a new "exporters" group in the integration test runner. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant MongoDBContainer
participant MongoExporter
participant MongoDBClient
TestRunner->>MongoDBContainer: Start MongoDB container
MongoDBContainer-->>TestRunner: Provide connection string
TestRunner->>MongoExporter: Export sample ResultEvent
MongoExporter->>MongoDBContainer: Insert document
TestRunner->>MongoDBClient: Connect and query for inserted document
MongoDBClient-->>TestRunner: Return document
TestRunner->>MongoDBContainer: Terminate container
TestRunner->>MongoExporter: Close exporter
TestRunner->>MongoDBClient: Close client
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/reporting/exporters/mongo/mongo_test.go (2)
17-29: Consider adding context timeout and better error handling.While the test correctly sets up a MongoDB container, it would benefit from a timeout context and improved error handling for container startup failures.
- ctx := context.Background() + // Add timeout to prevent hanging tests + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() mongodbContainer, err := mongodb.Run(ctx, "mongo:6") defer func() { if err := testcontainers.TerminateContainer(mongodbContainer); err != nil { log.Printf("failed to terminate container: %s", err) } }() if err != nil { - log.Printf("failed to start container: %s", err) - return + t.Fatalf("failed to start container: %s", err) }
20-20: Consider parameterizing MongoDB version.The MongoDB version is hardcoded to "mongo:6". Consider making this a constant or test parameter to make it easier to test with different versions.
+const ( + dbName = "test" + mongoDBVersion = "mongo:6" +) func TestMongo(t *testing.T) { ctx := context.Background() - mongodbContainer, err := mongodb.Run(ctx, "mongo:6") + mongodbContainer, err := mongodb.Run(ctx, mongoDBVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(7 hunks)pkg/reporting/exporters/mongo/mongo_test.go(1 hunks)
🔇 Additional comments (15)
go.mod (10)
121-122: LGTM! Dependency version upgrade.The upgrade of the
dario.cat/mergopackage from v1.0.0 to v1.0.1 is appropriate. This is a minor version update that likely contains bug fixes.
155-157: LGTM! New containerd-related dependencies.These containerd-related dependencies are required to support the testcontainers functionality being introduced for MongoDB integration testing.
158-158: LGTM! Docker ecosystem dependencies.These Docker-related dependencies are necessary for the testcontainers framework to manage Docker containers for the integration tests.
Also applies to: 162-162, 165-166, 167-167
169-170: LGTM! HTTP instrumentation dependencies.The addition of HTTP instrumentation libraries (
felixge/fgprofandfelixge/httpsnoop) will support profiling and monitoring in the testing environment.
179-180: LGTM! Logging dependencies.These logging libraries (
go-logr/lograndgo-logr/stdr) provide structured logging capabilities which are beneficial for test output and debugging.
212-212: LGTM! Additional container management dependencies.These dependencies provide additional container management capabilities needed for the testcontainer functionality.
Also applies to: 217-221, 225-225
245-245: LGTM! System monitoring dependency.Upgrading to the latest
gopsutilversion provides better system monitoring capabilities which may be helpful for resource tracking during tests.
250-251: LGTM! Core testcontainers dependencies.These are the primary dependencies for the integration tests being implemented. The main testcontainers-go module along with its MongoDB-specific module are essential for the MongoDB integration testing implemented in this PR.
278-282: LGTM! OpenTelemetry dependencies.The addition of OpenTelemetry dependencies will enable better observability in the testing environment, potentially helping with diagnosing test failures or performance issues.
365-365: LGTM! Windows-specific dependency upgrade.Upgrading the
Microsoft/go-winiopackage helps with Windows compatibility if tests need to run on Windows environments.pkg/reporting/exporters/mongo/mongo_test.go (5)
1-12: LGTM! Proper package structure and imports.The file is correctly set up with appropriate imports for the MongoDB integration test, including the necessary testcontainers libraries.
13-16: LGTM! Database name constant.Good practice to define the database name as a constant for reuse throughout the test.
30-37: LGTM! Proper container connection setup.The code correctly obtains the connection string from the container and appends the database name.
38-43: LGTM! Clear exporter configuration.The MongoDB exporter is configured with appropriate test parameters, including a batch size of 1 which is good for testing purposes.
44-53: LGTM! Proper exporter initialization and cleanup.The test correctly initializes the exporter and ensures it's properly closed after the test completes.
loresuso
force-pushed
the
testcontainer
branch
4 times, most recently
from
7b9adfe to
dcfbd56
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/reporting/exporters/mongo/mongo_test.go (1)
69-85: 🛠️ Refactor suggestionImprove verification of exported data.
The current verification only checks if a document can be found but doesn't verify that all the exported fields were correctly saved. The original
resvariable is also being overwritten, which isn't ideal for verification.// Verify that the result was written to the database clientOptions := mongooptions.Client().ApplyURI(connString) client, err := mongo.Connect(ctx, clientOptions) if err != nil { t.Fatalf("Error creating MongoDB client: %s", err) } defer func() { if err := client.Disconnect(ctx); err != nil { t.Fatalf("failed to disconnect from MongoDB: %s", err) } }() collection := client.Database(dbName).Collection(opts.CollectionName) - err = collection.FindOne(ctx, map[string]interface{}{"request": res.Request}).Decode(&res) + var retrievedResult output.ResultEvent + err = collection.FindOne(ctx, map[string]interface{}{"request": res.Request}).Decode(&retrievedResult) if err != nil { t.Fatalf("failed to find document in MongoDB: %s", err) } + + // Verify that the retrieved document matches the expected values + if retrievedResult.Request != res.Request || retrievedResult.Response != res.Response { + t.Fatalf("exported data doesn't match expected values: got %v, want Request=%s, Response=%s", + retrievedResult, res.Request, res.Response) + }
🧹 Nitpick comments (1)
pkg/reporting/exporters/mongo/mongo_test.go (1)
59-62: Create a more comprehensive test case.The current test case only populates the Request and Response fields. Consider creating a more comprehensive test case by populating additional fields from the ResultEvent struct to better test the exporter's capabilities.
res := &output.ResultEvent{ Request: "test request", Response: "test response", + TemplateID: "test-template", + Type: "http", + Host: "example.com", + Matched: "test match", + ExtractedResults: []string{"test extraction"}, + Timestamp: time.Now(), + MatcherStatus: true, }
🛑 Comments failed to post (3)
pkg/reporting/exporters/mongo/mongo_test.go (3)
39-39: 💡 Verification agent
🧩 Analysis chain
Check MongoDB connection string format.
When appending the database name to the connection string, you might need a separator character before the database name.
🏁 Script executed:
#!/bin/bash # Check the typical format of MongoDB connection strings echo "Checking MongoDB Go driver documentation..." curl -s https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo | grep -A 5 "mongodb://" || echo "No clear example found" echo "Checking MongoDB connection string format..." echo "Typical format is: mongodb://host:port/database"Length of output: 4445
Ensure a “/” separator when appending the database name to the MongoDB URI
The standard MongoDB URI format is:
mongodb://host:port/databaseIn
• pkg/reporting/exporters/mongo/mongo_test.go:39
you’re doing:connString = connString + dbNamewhich will yield e.g.
"mongodb://localhost:27017myDb"Instead, include a slash before the database name:
- connString = connString + dbName + connString = connString + "/" + dbNameOr, for clarity:
connString = fmt.Sprintf("%s/%s", connString, dbName)🤖 Prompt for AI Agents
In pkg/reporting/exporters/mongo/mongo_test.go at line 39, the code appends the database name directly to the connection string without a separating slash, resulting in an invalid MongoDB URI. Fix this by adding a "/" separator before the database name when concatenating, for example by using string formatting to combine the connection string and database name with a slash in between.
20-33: 🛠️ Refactor suggestion
Improve error handling for container startup failure.
Currently, if container creation fails, the code logs the error and returns instead of failing the test. Also, the deferred termination function would try to terminate a nil container, potentially causing a panic.
Apply this diff for better error handling:
// Start a MongoDB container mongodbContainer, err := mongodb.Run(ctx, "mongo:8") + if err != nil { + t.Fatalf("failed to start container: %s", err) + } defer func() { if err := testcontainers.TerminateContainer(mongodbContainer); err != nil { log.Printf("failed to terminate container: %s", err) } }() - if err != nil { - log.Printf("failed to start container: %s", err) - return - }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func TestMongo(t *testing.T) { ctx := context.Background() // Start a MongoDB container mongodbContainer, err := mongodb.Run(ctx, "mongo:8") if err != nil { t.Fatalf("failed to start container: %s", err) } defer func() { if err := testcontainers.TerminateContainer(mongodbContainer); err != nil { log.Printf("failed to terminate container: %s", err) } }() // …rest of test… }🤖 Prompt for AI Agents
In pkg/reporting/exporters/mongo/mongo_test.go around lines 20 to 33, improve error handling by checking if the container creation returned an error before deferring the termination. If there is an error starting the container, call t.Fatalf to fail the test immediately instead of just logging and returning. Also, only defer the termination function if the container was successfully created to avoid panics from terminating a nil container.
24-24: 💡 Verification agent
🧩 Analysis chain
Verify MongoDB version compatibility.
The test uses "mongo:8" which may not be a valid MongoDB version. The latest MongoDB version is 7.0 as of May 2023, and container tags usually follow MongoDB versioning conventions.
🌐 Web query:
What is the latest MongoDB version available as a Docker container?💡 Result:
Based on the search results, the latest MongoDB version available as a Docker container depends on which image repository you're using.
From the official MongoDB Docker Hub repository, MongoDB 7.0.20 appears to be the latest version available as of May 2025. This is indicated by a recent update to the 7.0 branch that was made approximately three weeks ago[6].
For the Bitnami packaged version of MongoDB, version 7.0.14 is mentioned in an example from September 2024[8]. However, since this information is from 2024, there may be newer versions available now.
The official Docker Hub repository for MongoDB offers different versions, including the latest builds that can be pulled using the command:
docker pull mongo:latestTo determine the exact MongoDB version inside a container, you can run:
docker run --rm mongo:latest mongod --versionThis command will display the specific MongoDB version installed in the container[8].
MongoDB Docker containers come in two main editions:
- Community Edition: Ideal for non-commercial use or smaller setups
- Enterprise Edition: Includes additional features like encryption, auditing, and LDAP integration for larger applications or businesses[7]
If you need the very latest version for your specific use case, it's recommended to check the Docker Hub tags page for the most current information, as new versions are regularly released.
Citations:
- 1: https://hub.docker.com/_/mongo
- 2: https://hub.docker.com/r/mongodb/mongodb-community-server
- 3: https://www.mongodb.com/compatibility/docker
- 4: https://hub.docker.com/layers/mongodb/atlas/v1.42.2-2025-05-06/images/sha256-3fdb73a5fb78656f5cbd3f51471b5d21c2e7679187c9f670248b1ae906983368
- 5: https://www.upgrad.com/blog/how-to-use-mongodb-docker/
- 6: https://github.com/docker-library/mongo
- 7: https://www.ionos.com/digitalguide/server/configuration/mongodb-docker-container/
- 8: https://www.mongodb.com/community/forums/t/what-is-the-method-to-determine-the-mongodb-version-within-a-particular-docker-image/297107
- 9: https://hub.docker.com/_/mongo/tags
- 10: https://hub.docker.com/r/bitnami/mongodb
Use a valid MongoDB Docker image tag
The test in pkg/reporting/exporters/mongo/mongo_test.go (line 24) currently tries to pull
mongo:8, which isn’t published. The latest official major release is 7.x (for example,7.0.20). Please update the tag to a valid one—e.g.:- mongodbContainer, err := mongodb.Run(ctx, "mongo:8") + mongodbContainer, err := mongodb.Run(ctx, "mongo:7")or pin to a specific patch release:
- mongodbContainer, err := mongodb.Run(ctx, "mongo:8") + mongodbContainer, err := mongodb.Run(ctx, "mongo:7.0.20")This ensures the container can be pulled successfully.
🤖 Prompt for AI Agents
In pkg/reporting/exporters/mongo/mongo_test.go at line 24, the Docker image tag "mongo:8" is invalid because MongoDB version 8 does not exist yet. Update the tag to a valid MongoDB version such as "mongo:7.0.20" or another current 7.x release to ensure the container can be pulled and run successfully during tests.
|
cc @dogancanbakir what do you think? I believe we are missing integration tests for all our exporters. It would be nice to cover these tests |
|
@loresuso Thanks for the PR! 100%. What do you think about mocking MongoDB instead of setting up a container to run actual MongoDB? If you still believe we should proceed with this, we can write integration tests instead. We've a simple convention for it, in this dir https://github.com/projectdiscovery/nuclei/tree/dev/cmd/integration-test. Let me know what you think! We can collab on it! |
|
Thanks for the comment @dogancanbakir ! Yes, my original idea would be to implement integration tests for all the exporters. The container felt the most useful way to test the integration, so that we know we are really writing to a mongo instance. I will move this file to the integration tests directory then! :) |
|
Hey @dogancanbakir! so it looks like I need to put my logic inside an |
|
Ok so now it should be in the expected format! Trying to find the best way to run it |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
cmd/integration-test/mongo.go (5)
18-21: Consider using a more stable MongoDB version tagThe MongoDB image version is set to "mongo:8", which might become outdated or unsupported over time. Consider using a more stable tag like "latest" or a specific minor version that's known to be stable.
const ( dbName = "test" - dbImage = "mongo:8" + dbImage = "mongo:latest" // or a specific stable version like "mongo:6.0" )
88-92: Enhance result verificationThe current verification only checks if the document exists by request field. Consider verifying all fields of the exported result to ensure data integrity.
collection := client.Database(dbName).Collection(opts.CollectionName) err = collection.FindOne(ctx, map[string]interface{}{"request": res.Request}).Decode(&res) if err != nil { return fmt.Errorf("failed to find document in MongoDB: %s", err) } + // Verify that the returned document matches our original test data + if res.Request != "test request" || res.Response != "test response" { + return fmt.Errorf("retrieved document does not match expected values: %+v", res) + }
48-52: Consider using a unique collection name for test isolationUsing a hardcoded "test" collection name may lead to conflicts with other tests or existing data. Consider using a unique/random name.
opts := mongo.Options{ ConnectionString: connString, - CollectionName: "test", + CollectionName: fmt.Sprintf("test_%d", time.Now().UnixNano()), // Generate unique collection name BatchSize: 1, // Ensure we write the result immediately }
93-95: Add cleanup to remove test collectionThe test should clean up after itself by dropping the collection to avoid leaving test data in the database.
+ // Clean up: drop the test collection + if err := collection.Drop(ctx); err != nil { + log.Printf("warning: failed to drop test collection: %s", err) + } return nil
65-73: Enhance test coverage with additional test casesThe current test only covers the basic export functionality. Consider adding test cases for edge cases, such as empty results or results with special characters.
You could extend the test to include multiple test cases with different types of data to ensure robust handling of various inputs:
testCases := []struct { name string result *output.ResultEvent }{ { name: "basic case", result: &output.ResultEvent{ Request: "test request", Response: "test response", }, }, { name: "with special characters", result: &output.ResultEvent{ Request: "test request with $pecial ch@racters", Response: "test response with \"quotes\" and other special characters", }, }, // Add more test cases as needed } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Export the result err = exporter.Export(tc.result) if err != nil { return fmt.Errorf("failed to export result event to MongoDB: %s", err) } // Verify the export // ... (verification code) }) }
|
@loresuso Yes! You're in the right direction. To run these tests, we need to add them to the To run the tests, go to the |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
cmd/integration-test/mongo.go (1)
1-96:⚠️ Potential issueRemove redundant file due to code duplication
This file appears to duplicate the implementation that already exists in
cmd/integration-test/exporters.go. The static analysis shows multiple redeclarations ofdbName,dbImage,mongoExporterstruct, and theExecutemethod.Having duplicate implementations creates maintenance challenges and confusion about which file is actually used in the test framework.
Since
exporters.goalready contains the proper test case registration withexportersTestCases, this file should be removed completely. The integration test framework references the test cases throughexportersTestCasesinexporters.go, making this file redundant.🧰 Tools
🪛 golangci-lint (1.64.8)
19-19: dbName redeclared in this block
(typecheck)
20-20: dbImage redeclared in this block
(typecheck)
23-23: mongoExporter redeclared in this block
(typecheck)
25-25: method mongoExporter.Execute already declared at cmd/integration-test/exporters.go:29:25
(typecheck)
♻️ Duplicate comments (3)
cmd/integration-test/mongo.go (3)
45-45: Fix incorrect use of fmt.Println with format string
fmt.Printlndoesn't process format directives like%s. You should usefmt.Printfinstead.- fmt.Println("MongoDB connection string: %s", connString) + fmt.Printf("MongoDB connection string: %s\n", connString)
58-63: Improve error handling in defer functionThe current defer function returns an error, but return values from defer functions are ignored. Consider logging the error instead.
- defer func() error { + defer func() { if err := exporter.Close(); err != nil { - return fmt.Errorf("failed to close exporter: %s", err) + log.Printf("failed to close exporter: %s", err) } - return nil }()
81-86: Fix ineffective error return in defer functionSimilar to the previous issue, this defer function returns an error which is ignored. The error should be logged instead.
- defer func() error { + defer func() { if err := client.Disconnect(ctx); err != nil { - return fmt.Errorf("failed to disconnect from MongoDB: %s", err) + log.Printf("failed to disconnect from MongoDB: %s", err) } - return nil }()
🧹 Nitpick comments (1)
cmd/integration-test/exporters.go (1)
68-76: Enhance test with more comprehensive ResultEvent dataThe current test only populates Request and Response fields. Consider adding more fields to the ResultEvent to test a more realistic scenario, such as TemplateID, Host, and Timestamp.
res := &output.ResultEvent{ Request: "test request", Response: "test response", + TemplateID: "test-template-id", + Host: "test-host", + Timestamp: time.Now(), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/integration-test/exporters.go(1 hunks)cmd/integration-test/integration-test.go(1 hunks)cmd/integration-test/mongo.go(1 hunks)go.mod(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/integration-test/mongo.go (1)
pkg/output/output.go (1)
ResultEvent(146-221)
🪛 golangci-lint (1.64.8)
cmd/integration-test/exporters.go
19-19: other declaration of dbName
(typecheck)
20-20: other declaration of dbImage
(typecheck)
27-27: other declaration of mongoExporter
(typecheck)
cmd/integration-test/mongo.go
19-19: dbName redeclared in this block
(typecheck)
20-20: dbImage redeclared in this block
(typecheck)
23-23: mongoExporter redeclared in this block
(typecheck)
25-25: method mongoExporter.Execute already declared at cmd/integration-test/exporters.go:29:25
(typecheck)
🔇 Additional comments (2)
cmd/integration-test/integration-test.go (1)
59-59: LGTM: Successfully integrates the MongoDB exporter testsThe change correctly adds the "exporters" category to the protocolTests map, allowing the new MongoDB exporter integration tests to run within the existing framework.
cmd/integration-test/exporters.go (1)
34-42: LGTM: Proper MongoDB container setup with error handlingThe container setup is well-implemented with appropriate error handling and cleanup in the defer function.
loresuso
force-pushed
the
testcontainer
branch
3 times, most recently
from
2ea0d88 to
1a0b72c
Compare
|
Ok, I think we are good! tested locally and it works again |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/integration-test/exporters.go (1)
44-48: 🛠️ Refactor suggestionValidate the connection string format before appending the database name
The code currently appends the database name directly to the connection string without checking if the connection string already ends with a slash. This could lead to malformed URIs.
connString, err := mongodbContainer.ConnectionString(ctx) if err != nil { return fmt.Errorf("failed to get connection string for MongoDB container: %s", err) } +// Ensure the connection string ends with a slash before appending database name +if !strings.HasSuffix(connString, "/") { + connString += "/" +} connString = connString + dbNameDon't forget to add
"strings"to the import statement.
🧹 Nitpick comments (3)
cmd/integration-test/exporters.go (3)
30-31: Consider using a shorter timeout for the test contextA 10-minute timeout seems excessive for this integration test. A shorter timeout (e.g., 1-2 minutes) would be more appropriate and would fail faster if there are issues.
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
67-70: Consider using more realistic test dataThe current test data is very minimal. Consider using more comprehensive test data that better represents real-world usage, including more fields from the ResultEvent struct.
res := &output.ResultEvent{ Request: "test request", Response: "test response", + // Add more fields to better represent real-world data + TemplateID: "test-template", + MatcherName: "test-matcher", + Type: "http", + Host: "example.com", + Timestamp: time.Now().UTC(), }
51-55: Consider making test constants configurableThe collection name and batch size are hardcoded. Consider making these configurable constants at the top of the file alongside the other constants.
const ( dbName = "test" dbImage = "mongo:8" + collectionName = "test" + batchSize = 1 // Ensure we write the result immediately ) // ... opts := mongo.Options{ ConnectionString: connString, - CollectionName: "test", - BatchSize: 1, // Ensure we write the result immediately + CollectionName: collectionName, + BatchSize: batchSize, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumintegration_tests/protocols/code/pre-condition.yamlis excluded by!**/*.yamlintegration_tests/protocols/code/py-env-var.yamlis excluded by!**/*.yamlintegration_tests/protocols/code/py-file.yamlis excluded by!**/*.yamlintegration_tests/protocols/code/py-interactsh.yamlis excluded by!**/*.yamlintegration_tests/protocols/code/py-snippet.yamlis excluded by!**/*.yamlintegration_tests/workflow/code-template-1.yamlis excluded by!**/*.yamlintegration_tests/workflow/code-template-2.yamlis excluded by!**/*.yaml
📒 Files selected for processing (3)
cmd/integration-test/exporters.go(1 hunks)cmd/integration-test/integration-test.go(1 hunks)go.mod(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/integration-test/integration-test.go
- go.mod
🔇 Additional comments (3)
cmd/integration-test/exporters.go (3)
96-98: Good implementation of result validationThe explicit validation of the exported result against the expected result is well implemented. This ensures that the data is correctly stored and retrieved from MongoDB.
23-25: Test case registration looks goodThe test case is properly registered with a descriptive path "exporters/mongo" which clearly indicates its purpose.
3-16: Well-organized importsThe imports are well-organized and include all necessary packages for the MongoDB integration test.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/integration-test/exporters.go (1)
44-49: 🛠️ Refactor suggestionAdd validation for the connection string format
The connection string should be validated before appending the database name to ensure it's properly formatted.
connString, err := mongodbContainer.ConnectionString(ctx) if err != nil { return fmt.Errorf("failed to get connection string for MongoDB container: %s", err) } +// Ensure the connection string ends with a slash before appending database name +if !strings.HasSuffix(connString, "/") { + connString += "/" +} connString = connString + dbNameDon't forget to add
"strings"to the import statement.
🧹 Nitpick comments (2)
cmd/integration-test/exporters.go (2)
51-55: Consider extracting test values as constantsFor better maintainability, consider extracting the collection name and batch size as constants.
const ( dbName = "test" dbImage = "mongo:8" + collectionName = "test" + batchSize = 1 ) // ... opts := mongo.Options{ ConnectionString: connString, - CollectionName: "test", - BatchSize: 1, // Ensure we write the result immediately + CollectionName: collectionName, + BatchSize: batchSize, // Ensure we write the result immediately }
29-31: Consider a shorter timeout for integration testsThe 10-minute timeout is quite long for this type of test. While it's good to have a generous timeout to prevent flaky tests, consider if a shorter duration (e.g., 2-5 minutes) would be sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/integration-test/exporters.go(1 hunks)cmd/integration-test/integration-test.go(1 hunks)go.mod(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/integration-test/integration-test.go
- go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/integration-test/exporters.go (3)
cmd/integration-test/integration-test.go (1)
TestCaseInfo(19-23)pkg/testutils/integration.go (1)
TestCase(247-250)pkg/output/output.go (1)
ResultEvent(146-221)
🔇 Additional comments (5)
cmd/integration-test/exporters.go (5)
23-25: The test case path is now properly descriptive.Good job setting a descriptive path for the MongoDB exporter test. This follows the best practices discussed in previous reviews.
33-42: LGTM! Good error handling for container startup.The container setup and error handling look good. The deferred cleanup function for terminating the container is properly implemented.
67-75: LGTM! Test data creation and export.The test case creates appropriate test data and exports it to MongoDB with proper error handling.
96-98: Good validation of the exported document.The verification logic correctly checks that both request and response fields match the expected values.
89-94: LGTM! Good MongoDB query implementation.The query to verify the exported document is well implemented, using the appropriate MongoDB driver functions and error handling.
|
Mmm interesting that it is failing on windows and macos. Wondering if there is Docker installed in the runner by default |
…container-go module Signed-off-by: Lorenzo Susini <[email protected]>
|
Added the installation of docker in macos and windows |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/integration-test/exporters.go (1)
44-49: Add validation for the connection stringCurrently, there's no validation that the connection string is properly formatted before appending the database name. Add a check to ensure the connection string ends with a slash before appending the database name.
connString, err := mongodbContainer.ConnectionString(ctx) if err != nil { return fmt.Errorf("failed to get connection string for MongoDB container: %s", err) } +// Ensure the connection string ends with a slash before appending database name +if !strings.HasSuffix(connString, "/") { + connString += "/" +} connString = connString + dbNameDon't forget to add
"strings"to the import statement.
🧹 Nitpick comments (5)
cmd/integration-test/exporters.go (5)
34-39: Consider adding container startup configuration optionsThe current container setup doesn't specify startup timeout or resource constraints. Consider adding configuration options for more robust test behavior.
// Start a MongoDB container -mongodbContainer, err := mongocontainer.Run(ctx, dbImage) +mongodbContainer, err := mongocontainer.RunWithOptions(ctx, mongocontainer.RunOptions{ + ContainerRequest: testcontainers.ContainerRequest{ + Image: dbImage, + ExposedPorts: []string{"27017/tcp"}, + WaitingFor: wait.ForListeningPort("27017/tcp").WithStartupTimeout(30 * time.Second), + }, +}) defer func() { if err := testcontainers.TerminateContainer(mongodbContainer); err != nil { log.Printf("failed to terminate container: %s", err) } }You'll need to add
"github.com/testcontainers/testcontainers-go/wait"to your imports.
67-75: Add test for batch export functionalityThe current test only verifies exporting a single document. Since the MongoDB exporter supports batch exports (as indicated by the BatchSize option), consider adding a test for batch exports with multiple documents.
res := &output.ResultEvent{ Request: "test request", Response: "test response", } err = exporter.Export(res) if err != nil { return fmt.Errorf("failed to export result event to MongoDB: %s", err) } +// Test batch export with multiple documents +res2 := &output.ResultEvent{ + Request: "test request 2", + Response: "test response 2", +} + +err = exporter.Export(res2) +if err != nil { + return fmt.Errorf("failed to export second result event to MongoDB: %s", err) +}
61-65: Improve error handling in defer functionCurrently, errors in the defer function for closing the exporter are only printed. Consider propagating these errors or at least using the same error formatting pattern as other parts of the code.
defer func() { if err := exporter.Close(); err != nil { - fmt.Printf("failed to close exporter: %s\n", err) + log.Printf("failed to close exporter: %s", err) } }()
83-87: Apply consistent error handling patternUse the same error logging pattern as other defer functions for consistency.
defer func() { if err := client.Disconnect(ctx); err != nil { - fmt.Printf("failed to disconnect from MongoDB: %s\n", err) + log.Printf("failed to disconnect from MongoDB: %s", err) } }()
29-101: Consider adding negative test casesThe current test only covers the happy path. Consider adding test cases for error scenarios, such as:
- Invalid connection string
- Connection timeout
- Authentication failure
- MongoDB server shutdown during operation
This would provide more comprehensive test coverage for the MongoDB exporter functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
.github/workflows/tests.yamlis excluded by!**/*.yamlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
cmd/integration-test/exporters.go(1 hunks)cmd/integration-test/integration-test.go(1 hunks)go.mod(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/integration-test/integration-test.go
- go.mod
🔇 Additional comments (2)
cmd/integration-test/exporters.go (2)
1-25: LGTM! The integration test for MongoDB exporter is well set up.The test case structure follows the project's conventions with proper Path naming and TestCase implementation.
96-98: Good validation of exported resultsThe explicit validation of the exported document is a good practice, ensuring that the data was correctly stored and retrieved from MongoDB.
…container-go module
Proposed changes
Proposing to implement integration testing with testcontainers for all export methods
Checklist
Summary by CodeRabbit