Introduce XDBG integration tests #865
Conversation
WalkthroughThe changes transition the integration test suite from "Upgrade Tests" to "Integration Tests" across workflow files and test code. A new utility for managing Docker containers and networks is introduced, including specialized builders for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestXDBGRealMLSPayloads
participant Docker as Docker Engine
participant XMTDP as xmtpd Container
participant XDBG as xdbg Container
Test->>Docker: Start xmtpd container (ghcr.io/xmtp/xmtpd:dev)
Docker-->>Test: Container started, port mapped
loop For each GeneratorType (identity, group, message)
Test->>Docker: Start xdbg container with GeneratorType and count=10
Docker->>XDBG: Run container, wait for exit
XDBG-->>Docker: Exit (success or failure)
Docker-->>Test: Return exit status, logs if failure
end
Test-->>Test: Assert no errors for all steps
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/integration/upgrade_test.go (1)
1-1: Consider utilizing the returned container instances.The updates correctly match the new
runContainersignature. However, the returned container instances could be used for additional assertions or debugging.You could enhance the tests by checking container health or logs:
- _, err := runContainer( + container, err := runContainer( t, image, fmt.Sprintf("xmtpd_test_%s", version), envVars, ) require.NoError(t, err, "Failed to start container version %s", version) + + // Optional: Verify container is healthy + state, stateErr := container.State(t.Context()) + require.NoError(t, stateErr) + require.True(t, state.Running, "Container should be running")Also applies to: 24-29, 33-39, 46-52
.github/workflows/integration-test.yml (1)
1-1: Update the workflow name to match the job renaming.The workflow name still references "Upgrade Tests" but should be updated to "Integration Tests" to be consistent with the job name change on line 11.
-name: Upgrade Tests +name: Integration Testspkg/integration/xdbg_test.go (1)
23-28: Consider enhancing test validation.The test only verifies that XDBG runs without errors but doesn't validate the actual functionality or outputs. Consider adding assertions to verify that the XDBG operations actually performed their intended actions.
For example, you could:
- Verify that the expected number of entities were generated
- Check container logs for success messages
- Validate that the XDBG container completed successfully
// Example enhancement: logs, err := container.Logs(context.Background()) require.NoError(t, err) // Add assertions on log content to verify successful operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/integration-test.yml(2 hunks)pkg/integration/docker_utils_test.go(6 hunks)pkg/integration/main_test.go(2 hunks)pkg/integration/upgrade_test.go(3 hunks)pkg/integration/xdbg_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/integration/xdbg_test.go (1)
pkg/integration/docker_utils_test.go (3)
GeneratorTypeIdentity(195-195)GeneratorTypeGroup(196-196)GeneratorTypeMessage(197-197)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Integration Tests
🔇 Additional comments (9)
pkg/integration/docker_utils_test.go (5)
1-1: LGTM! Package rename and imports are appropriate.The package rename to
integration_testaligns with the PR objective, and the new imports are properly utilized by the new XDBG integration functionality.Also applies to: 13-13, 18-19
73-73: LGTM! Enabling payer component for integration tests.The addition of
XMTPD_PAYER_ENABLEis consistent with enabling other components for comprehensive integration testing.
147-190: Excellent refactoring of the runContainer function!The changes improve the function in several ways:
- Using
t.Context()is more idiomatic for test functions- Returning the container instance gives callers better control over container lifecycle
- The timeout is preserved through the context
192-202: Well-structured type definition for generator types.Good use of typed string constants for compile-time safety. The
String()method implementation is clean and follows Go conventions.
247-275: Excellent error handling implementation!The function provides comprehensive error handling with:
- Clear error messages including exit codes
- Container logs included in error output for debugging
- Graceful handling of log retrieval failures
- Proper resource cleanup
pkg/integration/main_test.go (1)
1-1: LGTM! Consistent naming updates.The package rename and setup message update are consistent with the transition from "Upgrade Tests" to "Integration Tests".
Also applies to: 12-12
.github/workflows/integration-test.yml (1)
11-11: LGTM! Consistent renaming from upgrade to integration tests.The changes properly rename the test suite from "Upgrade Tests" to "Integration Tests" with appropriate updates to:
- Job name
- Step name
- Environment variable
- Test package path
Also applies to: 32-32, 36-37
pkg/integration/xdbg_test.go (2)
9-29: Well-structured integration test with comprehensive generator coverage.The test effectively covers all three generator types (Identity, Group, Message) and uses proper error handling. The approach of testing against a real container provides good integration coverage.
20-21:Details
✅ Verification successful
Verify the context usage.
The
t.Context()method usage needs verification as it's not a standard testing.T method.
🏁 Script executed:
#!/bin/bash # Description: Check if t.Context() is from a custom testing framework or extension # Expected: Find the source of this method, likely from testcontainers or a custom extension # Search for Context method definitions in test utilities ast-grep --pattern 'func ($_ *testing.T) Context() $$$' # Search for any Context method usage in test files rg "\.Context\(\)" --type go -A 2 -B 2 # Look for imports that might extend testing.T rg "testcontainers" --type go -A 5 -B 5Length of output: 24755
Confirm standard testing.T.Context usage
The
Context()method on*testing.Tis provided by the Go standard library (since Go 1.17) and there’s no custom implementation in our codebase. Usingt.Context()here is correct—no additional imports or extensions are required.
Introduce XDBG integration tests by migrating upgrade tests to integration package and adding XDBG container testing capabilitiesThe upgrade testing package is renamed to integration testing package and expanded with XDBG testing capabilities. The changes include: 📍Where to StartStart with the new XDBG integration test in xdbg_test.go to understand the main testing functionality, then review the container builder patterns in docker_utils_test.go. Macroscope summarized 63856c3. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/integration/upgrade_test.go (1)
23-26: Consider testing container functionality beyond startup.While the current test verifies that containers start successfully, it doesn't validate that the upgrade actually preserves functionality or data integrity. Consider adding basic functionality checks after container startup.
t.Logf("Starting old container") -, err := NewXmtpdContainerBuilder(t). +oldContainer, err := NewXmtpdContainerBuilder(t). WithImage(image). Build(t) require.NoError(t, err) + +// Add basic functionality check here +// e.g., health check, API endpoint verificationpkg/integration/docker_utils_test.go (1)
246-272: Consider making timeout configurable.The 10-second timeout for container startup might be too short for some environments or slower systems. Consider making this configurable or increasing the default.
func (b *XmtpdContainerBuilder) Build(t *testing.T) (testcontainers.Container, error) { - ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + timeout := 30*time.Second + if envTimeout := os.Getenv("CONTAINER_START_TIMEOUT"); envTimeout != "" { + if d, err := time.ParseDuration(envTimeout); err == nil { + timeout = d + } + } + ctx, cancel := context.WithTimeout(t.Context(), timeout) defer cancel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/integration/docker_utils_test.go(1 hunks)pkg/integration/upgrade_test.go(1 hunks)pkg/integration/xdbg_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/integration/xdbg_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/integration/docker_utils_test.go (1)
Learnt from: mkysel
PR: xmtp/xmtpd#865
File: pkg/integration/docker_utils_test.go:30-33
Timestamp: 2025-06-03T16:14:31.420Z
Learning: The XDBG image reference `ghcr.io/xmtp/xdbg:sha-78a5ac2` is a valid and existing image in the GitHub Container Registry for XMTP integration tests.
🧬 Code Graph Analysis (1)
pkg/integration/upgrade_test.go (1)
pkg/integration/docker_utils_test.go (1)
NewXmtpdContainerBuilder(200-212)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Integration Tests
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (8)
pkg/integration/upgrade_test.go (1)
11-12: Empty version slice makes upgrade test ineffective.The
xmtpdVersionsslice is currently empty, which meansTestUpgradeToLatestwill not execute any test iterations. This renders the upgrade testing functionality non-operational.Please populate this slice with actual version tags that should be tested for upgrade compatibility, or consider adding a TODO comment if versions will be added later.
pkg/integration/docker_utils_test.go (7)
30-34: Constants are well-defined and consistent.The constants are appropriately named and the XDBG image reference has been confirmed as valid. The naming follows Go conventions.
71-79: Environment variable configuration looks comprehensive.The function properly enables required services and generates unique database names for test isolation. The use of caller name and random string ensures test independence.
98-122: Build function has proper timeout and error handling.The Docker image building function includes appropriate timeout handling and distinguishes between timeout and other execution errors. The 5-minute timeout is reasonable for container builds.
160-189: Excellent error handling for container exit status.The
handleExitedContainerfunction provides comprehensive error reporting by checking exit codes and including container logs when failures occur. This will be invaluable for debugging test failures.
200-212: XmtpdContainerBuilder initialization is well-structured.The builder pattern implementation is clean and the default configuration with anvil.json is appropriate for integration testing. The file mounting setup follows testcontainers best practices.
312-351: XdbgContainerBuilder implementation addresses previous concerns.The error handling for
os.MkdirAllhas been properly implemented (lines 316-318), addressing the previous review feedback. The container configuration and volume mounting are correctly structured.
353-357: Network utility function is concise and effective.The
MakeDockerNetworkfunction provides a clean abstraction for creating test networks with proper error handling.
Introduce XDBG integration tests by renaming upgrade test package to integration and adding XDBG container testing functionality
The upgrade test package has been renamed to integration and expanded with XDBG testing capabilities:
• Renamed GitHub workflow from
upgrade-test.ymltointegration-test.ymlwith updated job names and environment variables• Moved all test files from
pkg/upgrade/topkg/integration/with package name changes fromupgrade_testtointegration_test• Modified
runContainerfunction indocker_utils_test.goto return container instance and added XDBG container support withGeneratorTypeenum andrunXDBGfunction• Added new
xdbg_test.gotest that runs XDBG with identity, group, and message generator types against xmtpd container• Updated test environment to enable
XMTPD_PAYER_ENABLEand use XDBG imageghcr.io/xmtp/xdbg:sha-78a5ac2📍Where to Start
Start with the new XDBG test in
xdbg_test.goto understand the main integration testing functionality, then review therunXDBGfunction indocker_utils_test.go.Macroscope summarized a026122.
Summary by CodeRabbit
New Features
Refactor
Chores