chore: primitive contract tests #628
Conversation
WalkthroughThe pull request introduces significant modifications to various test files within the internal contracts module. Key changes include the simplification of function signatures, particularly the removal of unnecessary parameters in functions like Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Functions
participant Platform as kwilTesting.Platform
participant Stream as StreamLocator
participant DB as Database
Test->>Platform: Setup with new parameters
Platform->>Stream: Create StreamLocator
Stream->>DB: Generate DB ID
DB-->>Stream: Return DB ID
Stream-->>Platform: Provide Stream ID
Platform-->>Test: Complete setup
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
The commit refactors the test setup by consolidating Ethereum address creation and signer assignment, improving code clarity and reducing redundancy. This change ensures consistent address handling across various test cases while leveraging a new helper function for setting up platforms with specific signers.
outerlook
force-pushed
the
chore/prim-tests
branch
from
4a83644 to
d9d0a16
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (9)
internal/contracts/tests/utils/procedure/execute.go (2)
103-108: Approve the addition of WithSigner function with suggestions for improvementThe new
WithSignerfunction is a valuable addition that allows for easy creation of platform instances with different signers, which aligns well with the PR objective of testing unauthorized inserts. The function maintains immutability by not modifying the original platform, which is a good practice.However, there are a couple of points to consider for improvement:
The function performs a shallow copy of the platform struct. If
kwilTesting.Platformcontains any pointer fields, this could lead to unexpected behavior. Consider using a deep copy method if available, or document this limitation.The function doesn't validate the input
signerbyte slice. Consider adding a check for the expected length or format of the signer to prevent potential issues.Here's a suggested improvement to address these points:
// WithSigner returns a new platform with the given signer, but doesn't mutate the original platform func WithSigner(platform *kwilTesting.Platform, signer []byte) (*kwilTesting.Platform, error) { if len(signer) != ExpectedSignerLength { // Replace ExpectedSignerLength with the actual expected length return nil, fmt.Errorf("invalid signer length: expected %d, got %d", ExpectedSignerLength, len(signer)) } newPlatform := platform.DeepCopy() // Assuming a DeepCopy method exists newPlatform.Deployer = signer return newPlatform, nil }If a
DeepCopymethod doesn't exist, consider adding a comment to document the shallow copy behavior.
Line range hint
1-108: Consider reorganizing the file structure for better readabilityThe addition of the
WithSignerfunction at the end of the file is logical and doesn't disrupt existing code. However, to improve the overall structure and readability of the file, consider grouping related functions together.Here's a suggested reorganization:
- Group all platform-related utility functions (including the new
WithSigner) at the beginning of the file, after the imports.- Follow with the procedure execution functions (
GetRecord,GetIndex,GetIndexChange).- Place helper functions like
processResultRowsat the end.This organization will make it easier for developers to find related functionality and understand the file's structure at a glance.
internal/contracts/tests/composed_test.go (2)
45-47: LGTM! Consider adding a comment for clarity.The changes look good and align with the PR objectives. The shift from
ComposedStreamNametoStreamIdusingcomposedStreamId.String()suggests an improvement in how stream identifiers are handled.Consider adding a brief comment explaining the significance of using
composedStreamId.String()instead of a plain string. This would enhance code readability and help future maintainers understand the rationale behind this change.+ // Use composedStreamId.String() to ensure consistent stream identifier representation StreamId: composedStreamId.String(),
Line range hint
1-134: Overall, the changes improve consistency and align with PR objectives.The modifications to both
testComposedLastAvailableandtestComposedNoPastDatafunctions consistently replaceComposedStreamNamewithStreamId, usingcomposedStreamId.String(). This change aligns well with the PR objectives of enhancing the testing framework for primitive contracts.To further improve the codebase:
- Ensure that this change in stream identifier handling is consistently applied across all relevant parts of the codebase.
- Consider updating the documentation to reflect this change in how stream identifiers are managed.
- If not already done, it might be beneficial to add unit tests specifically for the
composedStreamIdobject and itsString()method to ensure its reliability.internal/contracts/tests/index_change_test.go (3)
72-75: LGTM: Centralized Ethereum address handlingThe changes improve the code by centralizing the creation and usage of the Ethereum address. This refactoring enhances code clarity and consistency.
Consider adding a comment explaining the purpose of creating the
signervariable, especially if this pattern is repeated across multiple test functions. For example:// Create signer from platform.Deployer for consistent Ethereum address handling signer, err := util.NewEthereumAddressFromBytes(platform.Deployer)Also applies to: 83-84
143-145: LGTM: Consistent Ethereum address handling in testYoYIndexChangeThe changes in this function mirror those made in
testIndexChange, ensuring consistent handling of Ethereum addresses across different test functions. This refactoring improves code clarity and maintainability.For consistency with the suggestion for
testIndexChange, consider adding a similar comment here:// Create signer from platform.Deployer for consistent Ethereum address handling signer, err := util.NewEthereumAddressFromBytes(platform.Deployer)Also applies to: 159-163, 170-171
Line range hint
1-256: Summary: Improved Ethereum address handling across test functionsThe changes in this file primarily focus on refactoring how Ethereum addresses are created and used within the test functions. This refactoring improves code consistency and clarity by centralizing the address creation logic. The modifications don't alter the actual test scenarios or assertions, suggesting that the core functionality remains unchanged.
Key improvements:
- Consistent use of
StreamIdinSetupPrimitiveFromMarkdowncalls.- Centralized creation of Ethereum addresses using
signervariables.- Uniform updating of
SignerandCallerfields inTransactionData.To further enhance the code:
- Consider extracting the common Ethereum address creation logic into a helper function to reduce duplication across test functions.
- Ensure all test functions, including
testDivisionByZero, follow the new pattern for consistency unless there's a specific reason not to.- Add comments explaining the purpose of the refactoring to improve code maintainability for future developers.
internal/contracts/tests/utils/setup/primitive.go (1)
213-217: Simplifying deployer derivation in insertPrimitiveDataThe
deployeris derived frominput.primitiveStream.StreamLocator.DataProvider.Bytes(), then converted back to anEthereumAddress. Simplify by directly usingDataProvider:- deployer, err := util.NewEthereumAddressFromBytes(input.primitiveStream.StreamLocator.DataProvider.Bytes()) + deployer := input.primitiveStream.StreamLocator.DataProviderThis change enhances code readability and eliminates unnecessary conversions.
internal/contracts/tests/primitive_test.go (1)
276-276: Clarify the comment for better readability.The current comment could be clearer. Consider rephrasing it to improve understanding.
Apply this diff to enhance the comment:
- // we set in 2022 not to mix up with the initial data set in 2021 + // Use dates in 2022 to avoid mixing with the initial data set in 2021
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- internal/contracts/tests/common_test.go (17 hunks)
- internal/contracts/tests/complex_composed_test.go (1 hunks)
- internal/contracts/tests/composed_test.go (2 hunks)
- internal/contracts/tests/index_change_test.go (4 hunks)
- internal/contracts/tests/primitive_test.go (5 hunks)
- internal/contracts/tests/system_contract_test.go (5 hunks)
- internal/contracts/tests/utils/procedure/execute.go (2 hunks)
- internal/contracts/tests/utils/setup/composed.go (10 hunks)
- internal/contracts/tests/utils/setup/primitive.go (9 hunks)
Additional comments not posted (31)
internal/contracts/tests/composed_test.go (1)
94-96: LGTM! Verify the impact of these changes across the codebase.The changes are consistent with those made in the
testComposedLastAvailablefunction, which is good for maintaining a uniform approach across different test functions.To ensure that these changes have been consistently applied across the codebase, please run the following script:
This script will help identify any inconsistencies in the usage of
StreamIdvsComposedStreamNameacross the Go files in the project.Verification successful
Changes Verified!
All instances of
ComposedStreamNamehave been successfully removed, andStreamIdis consistently used across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent use of StreamId instead of ComposedStreamName # Test: Search for any remaining instances of ComposedStreamName echo "Searching for remaining instances of ComposedStreamName:" rg --type go 'ComposedStreamName' # Test: Verify the consistent use of StreamId echo "Verifying the use of StreamId:" rg --type go 'StreamId:\s*composedStreamId\.String\(\)'Length of output: 539
internal/contracts/tests/index_change_test.go (2)
52-55: LGTM: Addition of StreamId parameterThe addition of the
StreamIdparameter to theMarkdownPrimitiveSetupInputstruct is consistent with the refactoring efforts to centralize the handling of Ethereum addresses. This change improves the clarity and consistency of the code.
Line range hint
228-256: Verify: Consistency in testDivisionByZero functionThe
testDivisionByZerofunction hasn't been updated to use the new Ethereum address handling pattern seen in other test functions. This might be an oversight or intentional.Please verify if this function should be updated for consistency with the other test functions. If it should be updated, consider applying the following changes:
- Add the
StreamIdparameter to theSetupPrimitiveFromMarkdowncall.- Create a
signervariable fromplatform.Deployer.- Update the
SignerandCallerfields inTransactionDatato use the newsignervariable.If these changes are not necessary for this specific test, please add a comment explaining why to maintain code clarity.
internal/contracts/tests/complex_composed_test.go (2)
Line range hint
1-328: Missing tests for frozen dates and unauthorized inserts.The PR objectives mention adding tests for frozen dates and unauthorized inserts. However, I don't see any new tests in this file addressing these scenarios. Could you please clarify:
- Are these tests implemented in other files not included in this review?
- If not, do we need to add these tests to this file or elsewhere?
- If these tests are not part of this PR, should we update the PR description to accurately reflect the changes?
To help locate any relevant tests that might exist elsewhere in the codebase, you can run the following script:
This will help us determine if these tests exist elsewhere or if they need to be added.
49-51: LGTM: Improved stream identification method.The change from
ComposedStreamNametoStreamIdis a good improvement. It likely provides a more robust and standardized way of identifying streams, which can help prevent naming conflicts and improve system scalability.To ensure this change doesn't introduce inconsistencies, please run the following script to check for any remaining uses of
ComposedStreamNamein the codebase:Also, could you clarify how this change relates to the PR objectives of adding tests for frozen dates and unauthorized inserts? These don't seem to be directly addressed in this file.
internal/contracts/tests/system_contract_test.go (6)
16-16: LGTM: New import added for procedure utilities.The addition of the
procedurepackage import is appropriate and likely related to the changes in thedeployPrimitiveStreamWithDatafunction.
124-124: Consistent change, but objectives clarification still needed.This modification is consistent with the previous change to use
procedure.WithSigner. However, as mentioned earlier, it's still not clear how these changes relate to the PR objectives of adding tests for frozen dates and unauthorized inserts. Could you provide more context on how these objectives are being addressed?
224-228: LGTM: Function signature simplified and made consistent with usage.The changes to the
deployPrimitiveStreamWithDatafunction align well with the PR summary mentioning modifications to test utilities. The function signature has been simplified by removing thedataProviderparameter, and theStreamIdis now generated within the function. These changes make the function more consistent with its usage in the test functions we've seen earlier.A few observations:
- The removal of the
dataProviderparameter suggests this information is now handled differently, possibly through theprocedure.WithSignerutility.- Generating the
StreamIdwithin the function usingutil.GenerateStreamId(streamName)centralizes this logic, which is a good practice.These modifications appear to improve the overall structure and consistency of the test suite.
70-70: Approved: Updated function call, but clarification needed.The modification to use
procedure.WithSigneris consistent with the PR summary mentioning changes to test utilities. However, it's not immediately clear how this change relates to the PR objectives of adding tests for frozen dates and unauthorized inserts. Could you please clarify the connection?To verify the implementation of tests for frozen dates and unauthorized inserts, let's search for related changes:
#!/bin/bash # Search for new or modified test cases related to frozen dates and unauthorized inserts rg -n -A 5 'func.*Test.*Frozen|func.*Test.*Unauthorized' internal/contracts/tests/
Line range hint
1-359: Summary: Refactoring improvements made, but clarification needed on PR objectives.The changes in this file primarily involve refactoring the
deployPrimitiveStreamWithDatafunction and its usage across multiple test functions. These modifications align well with the PR summary mentioning changes to test utilities, improving the overall consistency and structure of the test suite.However, the connection to the PR objectives of adding tests for frozen dates and unauthorized inserts is not immediately clear from the changes in this file. This suggests that either:
- These objectives are addressed in other files not shown in this review.
- The PR description might not accurately reflect all the changes made.
To ensure a comprehensive review:
- Could you clarify how the PR objectives (tests for frozen dates and unauthorized inserts) are addressed, either in this file or in others?
- If these objectives are met in other files, could you provide references to those files or changes?
- If the PR objectives have changed or evolved, consider updating the PR description to accurately reflect the current scope of changes.
To help clarify the scope of changes across the project, let's run a search for modified test files:
#!/bin/bash # List all modified test files in the PR git diff --name-only origin/main... | grep '_test\.go$'This will help identify if there are other test files that might contain the changes related to frozen dates and unauthorized inserts.
153-153: Consistent changes across file, but overall explanation needed.This change, along with the previous two, shows a consistent pattern of updating the
deployPrimitiveStreamWithDatafunction calls to useprocedure.WithSigner. While these modifications appear to be part of a systematic refactoring of the test suite, it's not immediately clear how they address the PR objectives of adding tests for frozen dates and unauthorized inserts.Could you provide an overall explanation of how these changes contribute to meeting the PR objectives? Are there additional changes or tests that directly address frozen dates and unauthorized inserts that might not be visible in this file?
To verify the implementation of the PR objectives, let's search for any new test functions or significant changes in the test files:
internal/contracts/tests/common_test.go (7)
209-209: Simplified getDBID function calls across multiple tests: Verify test suite functionalityThe
getDBIDfunction calls have been simplified by removing theplatformparameter across multiple test functions, consistent with the previous changes.To ensure these changes don't affect the functionality of the test suite, please run the entire test suite and verify that all tests pass. You can use the following command:
#!/bin/bash # Description: Run the entire test suite go test -v ./internal/contracts/tests/Also applies to: 245-245, 286-286, 321-321, 358-358, 385-385
431-431: Updated function calls: Verify test functionality and implementationThe
getDBIDfunction calls have been simplified by removing theplatformparameter, consistent with previous changes. Additionally, thecheckComposePermissionsfunction call has been updated to remove thedeployerparameter.To ensure these changes don't affect the test's functionality and that the
checkComposePermissionsfunction is implemented correctly, please:
- Run the
TestIsStreamAllowedToComposetest:#!/bin/bash # Description: Run the TestIsStreamAllowedToCompose test go test -v -run TestIsStreamAllowedToCompose ./internal/contracts/tests/
- Verify the implementation of the
checkComposePermissionsfunction:#!/bin/bash # Description: Check the implementation of the checkComposePermissions function rg --type go "func checkComposePermissions" internal/contracts/tests/common_test.goAlso applies to: 447-447, 449-449, 462-462
510-510: Updated helper functions: Review implementation thoroughlyThe
getDBIDfunction has been simplified by removing theplatformparameter, consistent with changes throughout the file. ThecheckComposePermissionsfunction now derives the deployer from the platform, centralizing this logic.These changes improve code consistency and centralize the deployer retrieval logic. However, please ensure that:
- The
getDBIDfunction still works correctly without theplatformparameter.- The
checkComposePermissionsfunction correctly retrieves and uses the deployer address.To verify these changes, please review the implementation of both functions:
#!/bin/bash # Description: Check the implementation of getDBID and checkComposePermissions functions rg --type go "func (getDBID|checkComposePermissions)" internal/contracts/tests/common_test.goAlso applies to: 601-605
131-131: Simplified getDBID function call: Verify test functionalityThe
getDBIDfunction call has been simplified by removing theplatformparameter, consistent with the previous changes.To ensure this change doesn't affect the test's functionality, please run the test suite and verify that the
TestOnlyOwnerCanInsertMetadatatest passes. You can use the following command:
63-63: Simplified getDBID function call: Verify implementationThe
getDBIDfunction call has been simplified by removing theplatformparameter. This change aligns with the reported modifications in the summary.To ensure this change doesn't affect the functionality, please verify the implementation of the
getDBIDfunction. Run the following command to check its definition:Verification successful
Simplified
getDBIDfunction call verifiedThe
getDBIDfunction call has been successfully simplified by removing theplatformparameter. This change aligns with the function's current implementation ininternal/contracts/tests/common_test.goand does not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of the getDBID function rg --type go "func getDBID" internal/contracts/tests/common_test.goLength of output: 146
Script:
#!/bin/bash # Description: Search for the definition of the getDBID function across the entire codebase rg --type go "func getDBID" .Length of output: 122
7-8: New import added: Verify its usageThe new import
"github.com/truflation/tsn-db/internal/contracts/tests/utils/procedure"has been added. This suggests the introduction of a utility package for procedures.Please ensure that this new import is being used effectively in the test suite. Run the following command to verify its usage:
160-160: Simplified getDBID function call: Verify test functionalityThe
getDBIDfunction call has been simplified by removing theplatformparameter, consistent with the previous changes.To ensure this change doesn't affect the test's functionality, please run the test suite and verify that the
TestDisableMetadatatest passes. You can use the following command:internal/contracts/tests/utils/setup/primitive.go (13)
6-7: Importing types.StreamLocatorThe import of
github.com/truflation/tsn-sdk/core/typesis necessary for the use oftypes.StreamLocator.
21-21: Updating PrimitiveStreamDefinition to use StreamLocatorReplacing the
StreamIdfield withStreamLocatorenhances encapsulation by combiningStreamIdandDataProviderinto a single structure.
35-38: Modifying MarkdownPrimitiveSetupInput structureThe
Deployerfield has been replaced withStreamId, reflecting a shift in how deployment information is managed. This change aligns with the updated use ofStreamLocator.
52-53: Setting primitive schema name using StreamLocator
primitiveSchema.Nameis now assigned usingsetupInput.PrimitiveStreamWithData.StreamLocator.StreamId.String(), ensuring consistency with the updated data structures.
68-68: Generating dbid with consistent deployer
dbidis generated usingsetupInput.PrimitiveStreamWithData.StreamLocator.StreamId.String()anddeployer.Bytes(). Ensure thatdeployercorrectly corresponds toStreamLocator.DataProvider.Bytes()for consistency.
71-71: Passing Deployer to initializeContractThe
deployervariable is passed toinitializeContract. Confirm that it aligns with theDataProviderwhen initializing contracts involving third-party providers.
109-112: Constructing StreamLocator with StreamId and DataProvider
StreamLocatoris correctly constructed usinginput.StreamIdanddeployer. Ensure thatdeployeraccurately represents theDataProvider.
146-150: Updating InsertMarkdownDataInput structureFields have been updated to include
StreamLocatorinstead ofPrimitiveStreamName, allowing for more flexible data provider configurations.
160-160: Generating dbid using StreamLocator in InsertMarkdownPrimitiveData
dbidis generated usinginput.StreamLocator.StreamId.String()andinput.StreamLocator.DataProvider.Bytes(), ensuring that the dataset identifier is correctly formed based on the newStreamLocatorstructure.
180-181: Setting Signer and Caller in TransactionData
SignerandCallerare set usingsigner.Bytes()andsigner.Address(). Confirm thatsignercorrectly represents the intended data provider.
206-209: Generating dbid in insertPrimitiveData
dbidis generated usinginput.primitiveStream.StreamLocator.StreamId.String()andinput.primitiveStream.StreamLocator.DataProvider.Bytes(), ensuring consistency with the updated data structures.
224-225: Setting Signer and Caller in TransactionDataUsing
deployer.Bytes()anddeployer.Address()forSignerandCallerensures that transactions are executed with the correct identity.
60-61: Setting Signer and Caller in TransactionData
SignerandCallerare set usingdeployer.Bytes()anddeployer.Address(), respectively. Verify thatdeployercorrectly represents the intended signer, especially in the context of third-party data providers.To confirm the deployer address, you can check for discrepancies:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Related Problem
How Has This Been Tested?
Summary by CodeRabbit
New Features
testFrozenDataRetrievalandtestUnauthorizedInsertsfor enhanced test coverage.WithSignerto simplify the handling of signers in tests.Improvements
StreamLocator.Bug Fixes