misc: replace httpbin.org with a mock server #68
Conversation
lauzadis
added
the
no-changelog
ianbotsf
left a comment
ianbotsf
left a comment
There was a problem hiding this comment.
Minor nits, fix and ship (unless you feel like hunting down a KMP mock server).
| val response = roundTrip(url = "$url/anything", verb = "PUT", body = bodyToSend) | ||
| mockServer.clear(expectedRequest) |
There was a problem hiding this comment.
Nit: The mockServer.clear call should be in a finally block.
| dependencies { | ||
| implementation(kotlin("test-common")) | ||
| implementation(kotlin("test-annotations-common")) | ||
| implementation("org.mock-server:mockserver-netty:$mockServerVersion") | ||
| } |
There was a problem hiding this comment.
Nit: Discussed offline, MockServer belongs in jvmTest since it's not a KMP client.
There was a problem hiding this comment.
Correct that this dependency is jvm only. If we add this we basically have to move the tests to jvmTest as well and we won't be able to have common test suite anymore. This is ok for right now but eventually we'll be wiring up a kotlin/native version as well.
There was a problem hiding this comment.
I relocated them for now, we can discuss if that's the best thing long-term
| "This is a sample to prove that http downloads and uploads work. It doesn't really matter what's in here, we mainly just need to verify the downloads and uploads work." | ||
| private val TEST_DOC_SHA256 = "c7fdb5314b9742467b16bd5ea2f8012190b5e2c44a005f7984f89aab58219534" | ||
|
|
||
| val port = 60108 |
There was a problem hiding this comment.
suggestion: Is there not a way to get a random open port rather than hard coding? Similar to how we do it in smithy-kotlin http test suite?
There was a problem hiding this comment.
There is a way, by setting the port to 0
| dependencies { | ||
| implementation(kotlin("test-common")) | ||
| implementation(kotlin("test-annotations-common")) | ||
| implementation("org.mock-server:mockserver-netty:$mockServerVersion") | ||
| } |
There was a problem hiding this comment.
Correct that this dependency is jvm only. If we add this we basically have to move the tests to jvmTest as well and we won't be able to have common test suite anymore. This is ok for right now but eventually we'll be wiring up a kotlin/native version as well.
ianbotsf
left a comment
ianbotsf
left a comment
There was a problem hiding this comment.
One final minor nit, fix and ship!
| var mockServer: MockServerClient = MockServerClient("localhost", 0) | ||
| var url = "http://localhost:" | ||
|
|
||
| @BeforeAll | ||
| fun setup() { | ||
| mockServer = ClientAndServer.startClientAndServer(0) | ||
| url += mockServer.port | ||
| } |
There was a problem hiding this comment.
Nit: Why set mockServer and url in field initializers and then re-set them in setup? Seems like a perfect case for lateinit:
lateinit var mockServer: MockServerClient
lateinit var url: String
@BeforeAll
fun setup() {
mockServer = ClientAndServer.startClientAndServer(0)
url = "http://localhost:${mockServer.port}"
}
|
Kudos, SonarCloud Quality Gate passed!
|
3 participants
Issue #, if available:
N/A
Description of changes:
This PR replaces our usage of httpbin.org with a mock server. httpbin.org can be flaky at times and causes tests to fail occasionally.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.