API Mock Improvements #394
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes primarily involve updates to test utility functions across multiple packages. A new Changes
Sequence DiagramsequenceDiagram
participant Test as Test Function
participant TestUtils as TestUtils.NewTestAPIServer
participant Mocks as ApiServerMocks
Test->>TestUtils: Call with test context
TestUtils-->>Mocks: Create mock objects
TestUtils-->>Test: Return API Server, DB, Mocks, Cleanup Function
Test->>Test: Use returned components for testing
The sequence diagram illustrates the new pattern of test utility function calls, showing how the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
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 (
|
![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 (6)
pkg/api/payer/clientManager_test.go (1)
23-25: Consider utilizing the returned mocks for enhanced testingThe test is currently discarding the returned
ApiServerMockswith_. Consider utilizing these mocks to add more comprehensive test cases that verify interactions with the registry, validation service, and message publisher.pkg/testutils/api/api.go (1)
126-132: Consider adding mock verification in cleanupWhile the implementation is correct, consider adding mock verification in the cleanup function to ensure all expected mock interactions occurred.
return svr, db, allMocks, func() { + mockRegistry.AssertExpectations(t) + mockValidationService.AssertExpectations(t) + mockMessagePublisher.AssertExpectations(t) cancel() svr.Close() dbCleanup() }pkg/api/payer/publish_test.go (1)
109-109: Consider utilizing the returned mocks for better test coverageThe test discards the returned
ApiServerMocks. Consider using these mocks to verify interactions with the originator server and enhance test coverage.pkg/api/message/publish_test.go (1)
118-194: Consider extracting common test setup codeThe key package validation tests have duplicated setup code. Consider extracting the common setup into a helper function to improve maintainability.
+func setupKeyPackageTest(t *testing.T) (message_api.ReplicationApiClient, *apiTestUtils.ApiServerMocks, func(), *envelopes.ClientEnvelope) { + api, _, apiMocks, cleanup := apiTestUtils.NewTestReplicationAPIClient(t) + + clientEnv := envelopeTestUtils.CreateClientEnvelope(&envelopes.AuthenticatedData{ + TargetTopic: topic.NewTopic(topic.TOPIC_KIND_KEY_PACKAGES_V1, []byte{1, 2, 3}).Bytes(), + TargetOriginator: 100, + LastSeen: &envelopes.VectorClock{}, + }) + clientEnv.Payload = &envelopes.ClientEnvelope_UploadKeyPackage{ + UploadKeyPackage: &apiv1.UploadKeyPackageRequest{ + KeyPackage: &apiv1.KeyPackageUpload{ + KeyPackageTlsSerialized: []byte{1, 2, 3}, + }, + }, + } + + return api, apiMocks, cleanup, clientEnv +}Then use it in the tests:
func TestKeyPackageValidationSuccess(t *testing.T) { - api, _, apiMocks, cleanup := apiTestUtils.NewTestReplicationAPIClient(t) + api, apiMocks, cleanup, clientEnv := setupKeyPackageTest(t) defer cleanup() - - clientEnv := envelopeTestUtils.CreateClientEnvelope(&envelopes.AuthenticatedData{ - TargetTopic: topic.NewTopic(topic.TOPIC_KIND_KEY_PACKAGES_V1, []byte{1, 2, 3}).Bytes(), - TargetOriginator: 100, - LastSeen: &envelopes.VectorClock{}, - }) - clientEnv.Payload = &envelopes.ClientEnvelope_UploadKeyPackage{ - UploadKeyPackage: &apiv1.UploadKeyPackageRequest{ - KeyPackage: &apiv1.KeyPackageUpload{ - KeyPackageTlsSerialized: []byte{1, 2, 3}, - }, - }, - }pkg/api/message/subscribe_test.go (2)
28-30: LGTM! Consider adding documentation.The signature change aligns well with the PR objective of passing mocks back to test API. Consider adding a brief comment explaining the purpose of each return value for better maintainability.
+// setupTest initializes the test environment and returns: +// - A ReplicationApiClient for making API calls +// - A database connection for test data setup +// - ApiServerMocks for verifying mock interactions +// - A cleanup function to be deferred func setupTest( t *testing.T, ) (message_api.ReplicationApiClient, *sql.DB, testUtilsApi.ApiServerMocks, func()) {
120-120: Consider leveraging the mock objects for enhanced test coverage.While discarding the mock objects with
_is valid for these end-to-end style tests, you might want to consider adding new test cases that verify mock interactions. This could help catch integration issues earlier by ensuring the expected mock calls are made with the correct parameters.Example approach for a new test case:
func TestSubscribeEnvelopesMockInteractions(t *testing.T) { client, _, mocks, cleanup := setupTest(t) defer cleanup() // Setup mock expectations mocks.MockRegistry.EXPECT().YourMethod().Return(nil) // Test your functionality stream, err := client.SubscribeEnvelopes( context.Background(), &message_api.SubscribeEnvelopesRequest{...}, ) require.NoError(t, err) // Verify mock interactions require.True(t, mocks.MockRegistry.AssertExpectations(t)) }Also applies to: 141-141, 163-163, 185-185, 228-228, 250-250, 272-272
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/api/message/publish_test.go(6 hunks)pkg/api/message/subscribe_test.go(8 hunks)pkg/api/payer/clientManager_test.go(1 hunks)pkg/api/payer/publish_test.go(1 hunks)pkg/api/query_test.go(10 hunks)pkg/testutils/api/api.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/api/query_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
pkg/testutils/api/api.go (1)
68-72: LGTM! Well-structured mock consolidationThe new
ApiServerMocksstruct provides a clean way to organize and pass mock dependencies.pkg/api/message/publish_test.go (2)
135-144: LGTM! Good mock expectationsThe mock expectations are well-defined and properly verify the validation service behavior.
174-183: LGTM! Good error case handlingThe test properly verifies the error case when key package validation fails.
neekolas
changed the base branch from
01-06-add_key_package_validation
to
graphite-base/394
neekolas
force-pushed
the
01-06-pass_mocks_back_to_test_api
branch
from
0ef8fb1 to
97d1580
Compare
TL;DR
Added key package validation to the message publishing flow tests and updated test infrastructure to pass mocks to the caller.
Closes
#360
What changed?
ApiServerMocksstructure to handle validation service mockingHow to test?
go test ./pkg/api/message/...Why make this change?
To ensure key packages are properly validated before being published to the network, preventing invalid or malformed key packages from entering the system. This adds an important security layer to the message publishing flow.
Summary by CodeRabbit
Tests
Refactor
ApiServerMocksstruct to group related mock dependencies.The changes primarily focus on improving test infrastructure and mock management across various API test files.