-
Notifications
You must be signed in to change notification settings - Fork 74
RUM-12640: [Cronet] Web server for network library integration testing #3126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
🎯 Code Coverage 🔗 Commit SHA: 0006a1f | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3126 +/- ##
===========================================
+ Coverage 70.69% 70.82% +0.12%
===========================================
Files 893 893
Lines 33000 33000
Branches 5549 5549
===========================================
+ Hits 23329 23369 +40
+ Misses 8122 8090 -32
+ Partials 1549 1541 -8 🚀 New features to boost your workflow:
|
domalessi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor tweak but approved!
|
|
||
| - **HTTP Server** (`TestServer`) - Plain HTTP server using Netty | ||
| - **HTTPS Server** (`SecureTestServer`) - Secure server with self-signed certificate and HTTP/2 support | ||
| - Support for all common HTTP methods: GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Support for all common HTTP methods: GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS | |
| - Support for all common HTTP methods: `GET`, `POST`, `PUT`, `DELETE`, `PATCH`, `HEAD`, `OPTIONS` |
| "statusCode": 500, | ||
| "method": "GET", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it useful to have status code and method in the json body? they are already available at the HTTP layer
| val server = SecureTestServer(httpsPort = 8443) | ||
| server.start() | ||
|
|
||
| // Configure Cronet engine with custom certificate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this snippet doesn't show custom key store integration as OkHttp snippet above
|
|
||
| plugins { | ||
| id("org.jetbrains.kotlin.jvm") | ||
| id("com.github.ben-manes.versions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding id("com.datadoghq.dependency-license") here as well
| } | ||
|
|
||
| private fun Route.configureErrorEndpointsWithBody() { | ||
| get("/error/{code}/get") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a duplication of the method in the path as well, is it worth to have it there?
| path = "/post", | ||
| body = body, | ||
| headers = call.request.headers.entries() | ||
| .associate { it.key to it.value.joinToString(", ") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't we missing headers if header key is seen several time? like
HeaderA: firstValue
HeaderA: secondValue
|
|
||
| private lateinit var testServer: SecureTestServer | ||
| private lateinit var client: OkHttpClient | ||
| private lateinit var gson: Gson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private lateinit var gson: Gson | |
| private val gson: Gson = Gson() |
|
|
||
| private lateinit var testServer: TestServer | ||
| private lateinit var client: OkHttpClient | ||
| private lateinit var gson: Gson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private lateinit var gson: Gson | |
| private val gson: Gson = Gson() |
| .url("${server.httpsUrl()}/get") | ||
| .build() | ||
|
|
||
| val response = client.newCall(request).execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify the testing strategy. Is my understanding correct that when we use TestServer in some integration test in the future we will be doing assertions and verifications on the response?
What does this PR do?
Adds a new
tools/testservermodule providing embedded HTTP/HTTPS/QUIC test servers for network integration testing.Motivation
Network integration tests for libraries like
CronetandOkHttprequire a reliable test server to verify behavior without depending on external services. This service gonna be used for both HTTP and QUIC protocol testing in further tasksReview checklist (to be filled by reviewers)