Address code duplication in Access API tests and error handling #140
Conversation
WalkthroughThe changes introduced in this pull request focus on enhancing error handling and reducing code duplication across various API interaction tests and implementations. A new private utility function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (3)
The introduction of the
The new
As previously noted, throwing an Also applies to: 79-80, 94-95 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 (
|
SDK Examples Unit Test Results41 files 41 suites 7m 49s ⏱️ For more details on these failures, see this check. Results for commit 4926b5e. ♻️ This comment has been updated with latest results. |
Common Integration Test Results0 files 0 suites 0s ⏱️ Results for commit 4926b5e. ♻️ This comment has been updated with latest results. |
Integration Test Results 6 files ±0 6 suites ±0 3m 15s ⏱️ ±0s For more details on these failures, see this check. Results for commit 8731d91. ± Comparison against base commit 662759b. ♻️ This comment has been updated with latest results. |
lealobanov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Line range hint
77-306: Maintain consistency in helper method usage across tests.While many test methods use the new helper methods, some still contain inline error handling. For example,
getNodeVersionInfoandgetAccountByAddresstests could use helper methods.Consider extracting these into helper methods for consistency:
private fun getNodeVersionInfo(): FlowNodeVersionInfo = safelyHandle( { handleResult(accessAPI.getNodeVersionInfo(), "Failed to get node version info") }, "Failed to retrieve node version info" ) private fun getAccountByAddress(address: FlowAddress): FlowAccount = safelyHandle( { handleResult(accessAPI.getAccountByAddress(address), "Failed to get account by address") }, "Failed to retrieve account by address" )
Line range hint
1-306: Overall architectural improvements align with PR objectives.The introduction of
safelyHandleand helper methods successfully addresses code duplication in the Access API tests. The changes make the code more maintainable and consistent with the PR objectives.However, consider the following architectural improvements:
- Create an error handling module that can be shared across different test classes
- Consider using a test fixture pattern to reduce setup code duplication
- Add documentation for the error handling pattern to help other contributors
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
87-112: LGTM! Consider adding KDoc comments.The helper functions are well-structured and provide comprehensive mock data. They effectively support test cases by creating reusable mock responses.
Consider adding KDoc comments to document the purpose and return values of these utility functions:
/** * Creates a mock node version info response for testing. * @return Access.GetNodeVersionInfoResponse with predefined test values */ fun createMockNodeVersionInfo(): Access.GetNodeVersionInfoResponse /** * Creates a mock transactions response containing the provided transactions. * @param transactions List of FlowTransaction to include in the response * @return Access.TransactionsResponse containing the provided transactions */ fun createTransactionsResponse(transactions: List<FlowTransaction>): Access.TransactionsResponsesdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
Line range hint
737-749: Consider extracting common error handling test logic.These error handling test blocks contain duplicated verification logic. Consider extracting this pattern into a helper method to reduce duplication.
private suspend fun verifyErrorChannel(errorChannel: ReceiveChannel<Throwable>, expectedMessage: String) { var receivedException: Throwable? = null val job = launch { receivedException = errorChannel.receiveCatching().getOrNull() } advanceUntilIdle() job.join() if (receivedException != null) { assertEquals(expectedMessage, receivedException!!.message) } else { fail("Expected error but got success") } errorChannel.cancel() }Then use it in the test cases:
verifyErrorChannel(errorChannel, testException.message!!)Also applies to: 760-772, 806-818, 855-867
405-405: Consider strengthening account verification.The account verification logic in these test cases could be strengthened by also verifying the code field of the FlowAccount.
assertResultSuccess(result) { assertEquals(flowAccount.address, it.address) assertEquals(flowAccount.keys, it.keys) assertEquals(flowAccount.contracts, it.contracts) assertEquals(flowAccount.balance.stripTrailingZeros(), it.balance.stripTrailingZeros()) + assertEquals(flowAccount.code, it.code) }Also applies to: 421-421, 437-437
Line range hint
876-882: Consider enhancing assertResultSuccess helper method.The helper method could be enhanced to provide more detailed error information by including the actual error message in the exception.
private fun <T> assertResultSuccess(result: FlowAccessApi.AccessApiCallResponse<T>, assertions: (T) -> Unit) { when (result) { is FlowAccessApi.AccessApiCallResponse.Success -> assertions(result.data) - is FlowAccessApi.AccessApiCallResponse.Error -> throw IllegalStateException("Request failed: ${result.message}", result.throwable) + is FlowAccessApi.AccessApiCallResponse.Error -> throw IllegalStateException( + "Request failed: ${result.message}. Error details: ${result.throwable?.message}", + result.throwable + ) } }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2)
437-440: SimplifycompatibleRangeInitializationThe initialization of
compatibleRangecan be simplified using Kotlin's safe-call operator andletfunction, enhancing code readability.Apply this diff to streamline the code:
val compatibleRange = if (response.info.hasCompatibleRange()) { FlowCompatibleRange(response.info.compatibleRange.startHeight, response.info.compatibleRange.endHeight) -} else { null } +} else nullOr use safe-call with
let:+val compatibleRange = response.info.compatibleRange?.let { + FlowCompatibleRange(it.startHeight, it.endHeight) +}
26-41: Use Specific Exception Types in Catch BlocksCatching the general
Exceptionclass in thehandleApiCallmethod may obscure the root cause of errors. Using more specific exception types can aid in debugging and provide clearer error handling.Consider catching specific exceptions like
IOException,GrpcException, or any other relevant exceptions expected from the API calls.-} catch (e: Exception) { +} catch (e: IOException) {Adjust the exception types based on the exceptions that
apiCall()might throw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(7 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt(1 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt(6 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt(31 hunks)
🧰 Additional context used
📓 Learnings (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-07-03T12:30:04.496Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-10-08T17:04:37.869Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
🔇 Additional comments (7)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (3)
15-15: LGTM! Good use of shared test utilities.
The import of createMockAccount from FlowAccessApiImplTest.Companion helps reduce code duplication between test classes, which aligns well with the PR objectives.
121-130: LGTM! Good refactoring of test utilities.
Making MockResponseFactory public and adding new account response factory methods improves code reuse across test classes, which aligns with the PR's goal of reducing duplication.
Line range hint 468-503: LGTM! Effective reduction of test code duplication.
The account-related test methods have been successfully refactored to use the shared createMockAccount utility, which:
- Reduces code duplication
- Maintains comprehensive assertions
- Improves consistency across test cases
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
14-22: LGTM! Good refactoring of imports.
The imports have been organized to leverage shared test utilities from AsyncFlowAccessApiImplTest, which helps reduce code duplication across test classes.
55-58: Well-structured utility methods in companion object.
Good addition of utility methods createMockTransaction and createMockAccount to centralize mock object creation. This promotes code reuse and maintains consistency in test data creation.
87-87: Consistent error handling pattern across test cases.
Good use of shared testException across error test cases. This standardizes error handling and makes the tests more maintainable.
Also applies to: 106-106, 142-142, 172-172, 204-204, 290-290, 322-322, 391-391
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
285-296: Validate Transactions Before Sending
In sendTransaction, ensuring that the transaction is valid before sending can prevent unnecessary API calls and potential errors.
Run the following script to verify the transaction's validity:
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt
Outdated
Show resolved
Hide resolved
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(7 hunks)
🧰 Additional context used
📓 Learnings (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-07-03T12:30:04.496Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-10-08T17:04:37.869Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
🔇 Additional comments (2)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
22-24: Well-implemented error handling utility!
The safelyHandle function effectively simplifies error handling by combining Result handling with exception handling. This implementation addresses the previous feedback about reducing error handling complexity.
27-52: Previous suggestion about error message duplication remains valid.
The helper methods consistently use both handleResult and safelyHandle, but there's still duplication in error messages as noted in the previous review.
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
Closes: #138
Description
Addresses code duplication in error handling and tests related to the
FlowAccessApiandAsyncFlowAccessApiclasses (see issue #138)For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores