Better configurability for MockEngine #4846
Conversation
WalkthroughThe changes make the Changes
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (5)
✨ 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 (
|
jonapoul
changed the title
jonapoul
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: 0
🧹 Nitpick comments (3)
ktor-client/ktor-client-mock/api/ktor-client-mock.api (1)
6-6: The enqueue method return value could use more documentationThe boolean return value from
enqueueimplies success or failure, but it's not clear from this API definition when it would return false.ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockEngine.kt (2)
98-108: Well-documented implementation of handler management methodsThe documentation clearly explains what the methods do. However, the
enqueuemethod could benefit from additional documentation explaining what the boolean return value represents.Consider adding to the KDoc for
enqueue:/** * Appends a new [MockRequestHandler], to be called/removed after any previous handlers have been consumed. + * @return true if the handler was successfully added, false otherwise. */
125-141: Well-documented factory method with clear usage guidanceThe documentation thoroughly explains:
- What the method does
- The implications of using an empty engine
- When this approach is most useful
- A warning about handler disposal
Consider adding a parameter to customize the
reuseHandlersvalue, as some users might want to reuse handlers even with an empty initial configuration.-public fun empty(): MockEngine { +/** + * @param reuseHandlers Whether handlers should be reused after they are consumed. + * Defaults to false to ensure each handler is used only once. + */ +public fun empty(reuseHandlers: Boolean = false): MockEngine { val config = MockEngineConfig().apply { // Every time a handler is called, it gets disposed. So make sure enough handlers are registered for // requests you intend to make! - reuseHandlers = false + this.reuseHandlers = reuseHandlers } return MockEngine(config, throwIfEmptyConfig = false) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ktor-client/ktor-client-mock/api/ktor-client-mock.api(1 hunks)ktor-client/ktor-client-mock/api/ktor-client-mock.klib.api(2 hunks)ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockEngine.kt(4 hunks)ktor-client/ktor-client-mock/jvm/test/io/ktor/client/tests/mock/MockEngineTests.kt(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-client/ktor-client-mock/jvm/test/io/ktor/client/tests/mock/MockEngineTests.kt (2)
ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockUtils.kt (2)
respondOk(67-69)respondError(56-60)ktor-server/ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/Utils.kt (1)
assertFailsWith(94-104)
🔇 Additional comments (11)
ktor-client/ktor-client-mock/api/ktor-client-mock.api (3)
4-4: New constructor with throwIfEmptyConfig parameter is a good additionThis new constructor allows for creating a MockEngine without requiring an initial handler, which supports the goal of allowing more flexible test setup.
13-13: Good syntactic sugar with plusAssign operatorThe
plusAssignoperator provides a cleaner syntax for adding handlers, which will make test code more readable.
18-18: The empty() factory method solves the stated problem wellThis factory method directly addresses the PR objective of allowing initialization without an initial handler, making it easier to structure tests with handler setup in each test case.
ktor-client/ktor-client-mock/jvm/test/io/ktor/client/tests/mock/MockEngineTests.kt (2)
77-77: Fixed typo in test nameChanged "testWithContentNegotationPlugin" to "testWithContentNegotiationPlugin" to correct the spelling of "Negotiation".
95-114: Well-structured test for the new empty engine functionalityThis test thoroughly validates the new features:
- Creating an empty MockEngine
- Verifying the initial empty state
- Testing both enqueue (via +=) and handler response
- Verifying proper exception when no handlers remain
The test covers all the critical aspects of the new functionality and demonstrates the intended use case.
ktor-client/ktor-client-mock/api/ktor-client-mock.klib.api (3)
11-11: Constructor addition is consistent across API filesThis matches the constructor added in the main API file, ensuring consistent functionality across platforms.
23-24: New methods for dynamically adding handlers are properly exposed in the APIBoth
enqueueandplusAssignmethods are correctly defined in the API, making them accessible to clients.
29-29: The empty() factory method is properly exposed in the APIThis ensures the new factory method is available across all supported platforms.
ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockEngine.kt (3)
20-24: Good implementation of constructor with backward compatibilityThe implementation:
- Adds the new parameter to the primary constructor
- Provides a secondary constructor that maintains backward compatibility
- Uses descriptive parameter naming
This is a clean way to extend functionality while ensuring existing code continues to work.
41-46: Core implementation of the empty engine functionalityThis conditional check is the key change that allows initializing the engine without handlers. By making the check conditional based on the
throwIfEmptyConfigparameter, you enable the creation of an initially empty engine.
119-124: Simplified invoke operator in companion objectThis simplification makes the code cleaner while maintaining the same functionality.
|
❌ Junie can be launched only by a repository collaborator |
There was a problem hiding this comment.
Nice idea! I think the introduction of the queue functions on the MockEngine does create a little dissonance in the abstraction since the engine is externally immutable. I think it might make sense here to introduce something like class MockEngine.Queue: MockEngine() to expose the queue methods.
Then the test would look something like:
val engine = MockEngine.Queue()
assertTrue(engine.isEmpty())
val client = HttpClient(engine)
engine += { respondOk("hello") }
val response1 = client.get { url("http://127.0.0.1/normal-request") }
assertEquals("hello", response1.body<String>())
engine += { respondError(HttpStatusCode.BadRequest) }
val response2 = client.get { url("http://127.0.0.1/failed-request") }
assertEquals(HttpStatusCode.BadRequest, response2.status)
assertFailsWith<IllegalStateException> {
client.get { url("http://127.0.0.1/no-more-handlers-registered") }
}
|
❌ Junie can be launched only by a repository collaborator |
|
Thats fair (I'll take a look a bit later today). Out of interest - is there any reason why MockEngine should have an immutable interface? |
Subsystem
Client testing
Motivation
I recently started using Ktor as a replacement for OkHttp for various reasons, and I came across MockEngine as an apparent replacement for MockWebServer. One thing that I did quite a lot with MockWebServer was to:
@BeforeblockBut with MockEngine as it is I need to pass in an initial MockRequestHandler, otherwise an exception is thrown.
Solution
This change adds the option to intialise MockEngine without a MockRequestHandler using the
MockEngine.empty()function - allowing us to enqueue one or more handlers later in the test lifecycle. If no handler exists at the time of a call being made, it still throwserror()as it did before.Obviously this isn't going to cover all user's needs so I left the default constructor behaving as it always has, but personally I think this will help people to structure their tests a little better.
CI seems to be failing, but I'm fairly sure(???) that's not the fault of this branch.