-
Notifications
You must be signed in to change notification settings - Fork 775
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
Refactor OTLP mock collector test #3849
Refactor OTLP mock collector test #3849
Conversation
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/MockCollectorIntegrationTests.cs
Outdated
Show resolved
Hide resolved
})) | ||
.StartAsync(); | ||
|
||
var httpClient = host.GetTestClient(); |
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.
can we use a normal httpclient, not the Test "httpclient"?
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.
Yep, we could. That's what we do in the Grpc.Net.Client tests https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcServer.cs#L71.
Do we have a preference? I kinda like the TestServer/TestClient as I think it makes the test code more concise. That is, I think it'd be required to find an open port, perform the ConfigureKestrel thing, etc. Not a huge deal, but it's extra noise.
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.
Ok, you twisted my arm 😄. Still planning on addressing #3393, but this PR is good to go now.
I hard-coded ports for now making sure to use something that doesn't conflict with our other test with a hard-coded port
opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/InProcServerTests.cs
Line 55 in be78372
var res = await this.client.GetStringAsync("http://localhost:5000"); |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3849 +/- ##
=======================================
Coverage 87.34% 87.35%
=======================================
Files 279 279
Lines 10772 10772
=======================================
+ Hits 9409 9410 +1
+ Misses 1363 1362 -1
|
Follow up from #3788 (comment)