feat: add testcontainers-go tests to DynamoDB and NATS #1574
Conversation
mdelapenya
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes refactor the testing setup for both DynamoDB and NATS. Each now uses a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant TF as Test Function
participant TS as newTestStore
participant TC as TestContainers
TF->>TS: Call newTestStore(t)
TS->>TC: Start container with DynamoDB/NATS image
TC-->>TS: Return container details and connection info
TS-->>TF: Return Storage instance and error
TF->>TF: Check error and defer cleanup
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
nats/testdata/tls/redis.crt (1)
1-27: Review of Redis Certificate FileThe TLS certificate appears to be correctly structured in PEM format. Since this certificate is intended for testing purposes (as evidenced by its placement in the
testdatadirectory), please ensure it is clearly documented as a non-production, dummy certificate. This documentation would help prevent any potential misuse in a production environment.nats/testdata/tls/ca.crt (1)
1-32: Review of CA Certificate FileThe CA certificate is correctly structured in PEM format. As this certificate is intended to serve in a testing context (e.g., to validate test certificate chains), ensure it is clearly marked as being used solely for testing. Including a brief note at the top of the file could prevent any accidental reliance on it in production deployments.
nats/nats_test.go (3)
21-26: Minor copy-paste comment fix recommended.
The comment mentions "mysqlImage" while referring tonatsImage. Update it to accurately reflect the NATS context.- // mysqlImage is the default image used for running MySQL in tests. + // natsImage is the default image used for running NATS in tests.
28-40: Check naming consistency of embedded certificate files.
You're embedding files namedredis.crtandredis.keyin a NATS-specific test environment. This might be confusing or misleading. If they're NATS server certs, consider renaming them to reflect their actual usage.
331-342: Effective performance benchmark for Set.
Ensuring the store is initialized once per benchmark iteration can improve reliability and consistency. The code logic is correct.To avoid potential interference among runs, consider scoping each benchmark iteration with fresh keys or resetting the store between runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-dynamodb.ymlis excluded by!**/*.yml.github/workflows/test-nats.ymlis excluded by!**/*.ymldynamodb/go.modis excluded by!**/*.moddynamodb/go.sumis excluded by!**/*.sum,!**/*.sumnats/go.modis excluded by!**/*.modnats/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (7)
dynamodb/dynamodb_test.go(8 hunks)nats/nats_test.go(9 hunks)nats/testdata/tls/ca.crt(1 hunks)nats/testdata/tls/client.crt(1 hunks)nats/testdata/tls/client.key(1 hunks)nats/testdata/tls/redis.crt(1 hunks)nats/testdata/tls/redis.key(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
nats/testdata/tls/redis.key
1-28: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (25)
nats/testdata/tls/client.crt (1)
1-27: Review of Client Certificate FileThe client certificate is properly formatted in PEM. As this file is meant to support secure communication in test scenarios, confirm that its usage is strictly limited to testing, and consider adding a header comment in the file to clearly indicate its test-only purpose.
nats/testdata/tls/client.key (1)
1-29:❓ Verification inconclusive
Critical: Private Key Exposure in Client Key
This file contains a private key in PEM format. Storing private keys in plaintext—even for testing—poses a potential security risk if they are ever misused. Please verify that this key is a dummy key intended solely for integration tests and is not used in any production scenario. It may be beneficial to add a header comment explicitly stating its test-only nature.
Below is a shell script that you can run to verify that the exposed key is used only in test code and to inspect its contents:
#!/bin/bash # 1. Search for references to 'nats/testdata/tls/client.key' outside of test directories. echo "Searching for references to 'nats/testdata/tls/client.key' outside test files..." rg "nats/testdata/tls/client.key" --glob "!.*/testdata/.*" -n . # 2. Display the first 30 lines of the key file to verify its contents. echo "Displaying the first 30 lines of nats/testdata/tls/client.key:" head -n 30 nats/testdata/tls/client.keyPlease execute this script and review the output. The first step checks that the key isn't referenced in production or non-test code, ensuring it’s isolated for integration tests only. The output from the second step should confirm that the file contents match the dummy key provided.
Once you have verified:
- That the key is used solely within test contexts.
- That the key itself is clearly a dummy/test key.
You can then address (or dismiss) the review comment accordingly.
If after running the script you confirm that the key is only used in tests and is dummy by nature, you may add a header comment to the file specifying its test-only purpose to make the intent explicit.
nats/testdata/tls/redis.key (1)
1-29:❓ Verification inconclusive
Critical: Private Key Exposure in Redis Key
Similar to the client key, this file contains a private key in PEM format. Confirm that it is a non-production, dummy key for testing purposes only. Storing a private key publicly, even for tests, can be flagged by static analysis tools (e.g., Gitleaks), so please ensure that its usage is safe and appropriately documented.
Action Required: Verify Dummy Private Key in Test Data
It appears that the file at
nats/testdata/tls/redis.keycontains a private key in PEM format. Since it’s located under a test data folder, it is likely intended solely for non-production testing. However, please verify the following:
- Confirm that this key is a dummy key used exclusively for testing.
- Ensure its usage is clearly documented so that static analysis tools (e.g., Gitleaks) don’t flag it as a security risk.
- Double-check that no production system inadvertently relies on this key.
Once confirmed, please update the documentation (or add comments within the test configuration) to explicitly state its safe, non-production usage.
🧰 Tools
🪛 Gitleaks (8.21.2)
1-28: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
nats/nats_test.go (9)
114-125: Good usage of the newTestStore in test.
This test pattern withdefer testStore.Close()ensures containers are cleaned up. The logic for setting keys and verifying them is correct.
132-137: Appropriate approach to test overwriting keys.
Reusing the same key with a new value is straightforward and is tested thoroughly.
167-177: Well-structured TTL (expiration) test for NATS.
Sleeping for slightly over one second confirms correct expiration. Clear usage.
239-246: No issues for getting non-existent keys.
This test ensures retrieving a non-existent key returns empty. Good coverage.
253-279: Proper delete coverage.
Test scenario for delete is correct, verifying key removal.
281-312: Bulk reset test is comprehensive.
Test scenario ensures all keys are wiped, covering the general reset behavior.
345-359: Straightforward get benchmark.
Logic is correct and ensures repeated retrieval.
362-376: Benchmark set and delete operations.
The approach is valid. Ensuring repeated set and deletes can measure throughput effectively.
42-112:❓ Verification inconclusive
Validate container readiness and cleanup flow.
ThenewTestStorefunction properly starts a NATS container with TLS and registers cleanup. Overall logic looks good. However, consider verifying that the container is fully ready before proceeding with further steps (for instance, having a health check or a more robust wait strategy).If you’d like to confirm the container is indeed ready for TLS-based connections, you can expand the wait strategy to ensure that the TLS port is responsive.
🏁 Script executed:
#!/bin/bash # Example approach: Checking logs for the final "Server is ready" message if present. rg "Server is ready" nats/nats.go -A 10Length of output: 41
Ensure complete container readiness for TLS connections.
ThenewTestStorefunction correctly initializes a NATS container with TLS and handles cleanup. However, relying solely on the log message "Listening for client connections on 0.0.0.0:4443" may not fully guarantee that the container is ready for TLS-based interactions. Consider enhancing the wait strategy—such as by verifying that the TLS port is responsive or by checking for an explicit readiness log like "Server is ready."Please manually verify that the container is indeed fully ready before proceeding with TLS operations.
dynamodb/dynamodb_test.go (13)
13-17: Accurate environment variable usage.
The constants and environment variable logic for the DynamoDB image is correct and consistent with testcontainers best practices.
19-52:newTestStorefunction is well-structured.
Running a local DynamoDB container withtestcontainersis properly set up. Cleanup logic usingtestcontainers.CleanupContainerensures containers don’t linger. Error returns are handled.
60-63: Appropriate test instantiation.
Creating a new store per test prevents cross-test contamination.
74-77: Overwriting keys scenario is tested.
Verifies the same key is set again, ensuring proper coverage.
91-101: Valid GET test logic.
Confirms the store retrieval is consistent.
103-111: Nonexistent key coverage.
Properly verifies that missing keys return no data.
119-132: Deleting entries scenario is validated.
Key is confirmed removed, ensuring the data is wiped as expected.
137-157: Full reset coverage.
Confirms that all keys are removed in a single operation.
159-164: No concerns with closing logic.
Ensures resources are properly freed.
167-172:Conn()function usage is verified.
Checks for a valid connection instance. Straightforward.
178-186: Set benchmark structure.
Uses testcontainers approach for an isolated test environment. Code logic is sound.
189-202: Get benchmark logic is consistent.
Proper approach for repeated retrieval. No issues.
209-220: Set and delete benchmark covers typical usage pattern.
Repeatedly sets and deletes the same key, providing coverage for throughput analysis.
|
@gaby now that I'm part of the org, what's the policy for merging: waiting for +2 approvals? Is it any of you, me the one merging? |
|
@mdelapenya Yes, 2 people excluding the one creating the PR. I posted a link to the PR to the "ready for review channel", but all the maintainers are in different timezones. |
What does this PR do?
It follows the same strategy of adding testcontainers-go to make it possible running the integration tests for the storage layer in the local machine,
not needing to install the given storage technology in the host.
I'm copying the result of executing the
gen-test-certs.shinto the testdata dir of the nats module, although I'm open to discuss whether it's worth it having them statically or dynamically.Why is it important?
Run the tests exactly the same in the Ci as in the local machine, making it possible to test/debug locally.
Related PRs
Summary by CodeRabbit
Tests
Configuration