Add integration test helper for connectors #393
Conversation
|
""" WalkthroughA new integration test wrapper is introduced to facilitate connector testing using an in-memory gRPC server and client setup. Supporting test code and a mock connector are added for integration testing. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Wrapper as IntegrationTestWrapper
participant gRPCServer as In-Memory gRPC Server
participant Connector as Connector Implementation
participant gRPCClient as gRPC Client
participant Syncer as Syncer
participant Manager as C1Z Manager
Test->>Wrapper: Create (with Connector)
Wrapper->>gRPCServer: Start (register services)
Wrapper->>gRPCClient: Create (connect via bufconn)
Wrapper->>Syncer: Initialize (with temp c1z file)
Wrapper->>Manager: Initialize (with c1z file)
Test->>Wrapper: Call Sync()
Wrapper->>Syncer: Sync & Close
Wrapper->>Manager: Access manager/load c1z
Test->>Wrapper: Cleanup (shutdown server, remove temp file)
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/test/integration_wrapper_test.go (2)
14-21: Consider using a more specific trait for the mock resource type.The mock resource type uses
TRAIT_UNSPECIFIEDwhich might not accurately represent the intended behavior. Consider using a more appropriate trait likeTRAIT_USERorTRAIT_GROUPfor clearer test semantics.var mockResourceType = &v2.ResourceType{ Id: "mock_resource_type", Description: "mock resource type", DisplayName: "mock resource type", Traits: []v2.ResourceType_Trait{ - v2.ResourceType_TRAIT_UNSPECIFIED, + v2.ResourceType_TRAIT_USER, }, }
71-86: Consider adding more test cases for comprehensive coverage.The test only covers the happy path. Consider adding test cases for:
- Error scenarios during sync
- Multiple resource types
- Resources with entitlements and grants
- Edge cases like empty resource lists
Would you like me to generate additional test cases to improve test coverage?
pkg/test/integration_wrapper.go (1)
97-101: Consider using t.Cleanup for goroutine error handling.The goroutine that serves the gRPC server uses
log.Fatalfwhich will terminate the entire test process. Consider using the test's error reporting mechanism instead.go func() { if err := s.Serve(lis); err != nil { - log.Fatalf("Server exited with error: %v", err) + t.Errorf("Server exited with error: %v", err) } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/test/integration_wrapper.go(1 hunks)pkg/test/integration_wrapper_test.go(1 hunks)vendor/google.golang.org/grpc/test/bufconn/bufconn.go(1 hunks)vendor/modules.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.23.x, windows-latest)
🔇 Additional comments (2)
vendor/google.golang.org/grpc/test/bufconn/bufconn.go (1)
1-319: LGTM! Appropriate vendoring of gRPC's in-memory connection utility.This vendored
bufconnpackage from the official gRPC project provides a robust in-memory implementation ofnet.Connandnet.Listenerinterfaces, which is ideal for integration testing without network overhead. The implementation includes proper synchronization, timeout handling, and close semantics.vendor/modules.txt (1)
654-654: LGTM! Module list correctly updated.The addition of
google.golang.org/grpc/test/bufconnto the vendor modules list properly tracks the newly vendored package.
MarcusGoldschmidt
force-pushed
the
goldschmidt/add-integration-test-helpers
branch
2 times, most recently
from
a744323 to
ac2c24c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/test/integration_wrapper.go (1)
25-25: Consider increasing the buffer size for larger test payloads.The current buffer size of 1024 bytes might be insufficient for integration tests with larger data payloads. Consider increasing it to accommodate realistic test scenarios.
-const bufSize = 1024 +const bufSize = 1024 * 1024 // 1MB should be sufficient for most test scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/test/integration_wrapper.go(1 hunks)pkg/test/integration_wrapper_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/test/integration_wrapper_test.go
🔇 Additional comments (4)
pkg/test/integration_wrapper.go (4)
1-23: LGTM! Clean imports and package structure.The imports are well-organized and include all necessary dependencies for the integration test wrapper functionality.
27-43: Excellent design pattern for unified client interface.The embedded service clients provide a clean, unified interface for accessing all connector services in tests.
45-52: Well-designed wrapper struct with good encapsulation.The struct provides a clean interface with appropriate encapsulation, especially the intentional hiding of the syncer implementation details.
162-187: Well-implemented wrapper methods with proper error handling.All three methods follow Go best practices:
- Manager() provides clean access to the manager
- LoadC1Z() includes proper error handling and validation
- Sync() correctly handles the sync and close lifecycle
MarcusGoldschmidt
force-pushed
the
goldschmidt/add-integration-test-helpers
branch
from
ac2c24c to
423bdf6
Compare
MarcusGoldschmidt
force-pushed
the
goldschmidt/add-integration-test-helpers
branch
from
423bdf6 to
82158c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/test/integration_wrapper.go(1 hunks)pkg/test/integration_wrapper_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/test/integration_wrapper_test.go
🧰 Additional context used
🧠 Learnings (1)
pkg/test/integration_wrapper.go (2)
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.351Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Learnt from: MarcusGoldschmidt
PR: ConductorOne/baton-sdk#393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.351Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
🔇 Additional comments (2)
pkg/test/integration_wrapper.go (2)
164-189: LGTM! Well-implemented helper methods.The methods provide clean interfaces for accessing the manager, loading c1z files, and performing synchronization. The error handling is appropriate and the Sync method correctly follows the pattern of calling both Sync() and Close() on the syncer.
53-162: Excellent integration test helper design with proper cleanup and instrumentation.The integration wrapper provides a comprehensive solution for connector testing with:
- In-memory gRPC server/client setup using bufconn
- Proper OpenTelemetry instrumentation for observability
- Comprehensive cleanup mechanisms to prevent test artifacts
- Progress logging for debugging
This addresses previous feedback and follows testing best practices.
| connectorV2.RegisterConnectorServiceServer(s, srv) | ||
| connectorV2.RegisterGrantsServiceServer(s, srv) | ||
| connectorV2.RegisterEntitlementsServiceServer(s, srv) | ||
| connectorV2.RegisterResourcesServiceServer(s, srv) | ||
| connectorV2.RegisterResourceTypesServiceServer(s, srv) | ||
| connectorV2.RegisterAssetServiceServer(s, srv) | ||
| connectorV2.RegisterEventServiceServer(s, srv) | ||
| connectorV2.RegisterResourceGetterServiceServer(s, srv) | ||
| connectorV2.RegisterGrantManagerServiceServer(s, srv) | ||
| connectorV2.RegisterResourceManagerServiceServer(s, srv) | ||
| connectorV2.RegisterResourceDeleterServiceServer(s, srv) | ||
| connectorV2.RegisterAccountManagerServiceServer(s, srv) | ||
| connectorV2.RegisterCredentialManagerServiceServer(s, srv) |
There was a problem hiding this comment.
Add missing server registrations for complete service coverage.
The server setup is missing registrations for TicketsServiceServer and ActionServiceServer, but the client includes TicketsServiceClient and ActionServiceClient. This mismatch will cause runtime errors when these services are called.
Add the missing server registrations:
connectorV2.RegisterAccountManagerServiceServer(s, srv)
connectorV2.RegisterCredentialManagerServiceServer(s, srv)
+ connectorV2.RegisterTicketsServiceServer(s, srv)
+ connectorV2.RegisterActionServiceServer(s, srv)📝 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.
| connectorV2.RegisterConnectorServiceServer(s, srv) | |
| connectorV2.RegisterGrantsServiceServer(s, srv) | |
| connectorV2.RegisterEntitlementsServiceServer(s, srv) | |
| connectorV2.RegisterResourcesServiceServer(s, srv) | |
| connectorV2.RegisterResourceTypesServiceServer(s, srv) | |
| connectorV2.RegisterAssetServiceServer(s, srv) | |
| connectorV2.RegisterEventServiceServer(s, srv) | |
| connectorV2.RegisterResourceGetterServiceServer(s, srv) | |
| connectorV2.RegisterGrantManagerServiceServer(s, srv) | |
| connectorV2.RegisterResourceManagerServiceServer(s, srv) | |
| connectorV2.RegisterResourceDeleterServiceServer(s, srv) | |
| connectorV2.RegisterAccountManagerServiceServer(s, srv) | |
| connectorV2.RegisterCredentialManagerServiceServer(s, srv) | |
| connectorV2.RegisterConnectorServiceServer(s, srv) | |
| connectorV2.RegisterGrantsServiceServer(s, srv) | |
| connectorV2.RegisterEntitlementsServiceServer(s, srv) | |
| connectorV2.RegisterResourcesServiceServer(s, srv) | |
| connectorV2.RegisterResourceTypesServiceServer(s, srv) | |
| connectorV2.RegisterAssetServiceServer(s, srv) | |
| connectorV2.RegisterEventServiceServer(s, srv) | |
| connectorV2.RegisterResourceGetterServiceServer(s, srv) | |
| connectorV2.RegisterGrantManagerServiceServer(s, srv) | |
| connectorV2.RegisterResourceManagerServiceServer(s, srv) | |
| connectorV2.RegisterResourceDeleterServiceServer(s, srv) | |
| connectorV2.RegisterAccountManagerServiceServer(s, srv) | |
| connectorV2.RegisterCredentialManagerServiceServer(s, srv) | |
| + connectorV2.RegisterTicketsServiceServer(s, srv) | |
| + connectorV2.RegisterActionServiceServer(s, srv) |
🤖 Prompt for AI Agents
In pkg/test/integration_wrapper.go around lines 85 to 97, the server setup is
missing registrations for TicketsServiceServer and ActionServiceServer, causing
a mismatch with the client that includes TicketsServiceClient and
ActionServiceClient. To fix this, add the missing server registration calls for
TicketsServiceServer and ActionServiceServer to the server setup to ensure
complete service coverage and prevent runtime errors.
[ ] Bugfix
[X] Feature
Add an integration test helper for connectors to run locally as go test.
Summary by CodeRabbit
New Features
Tests