-
Notifications
You must be signed in to change notification settings - Fork 773
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
Add OTLP exporter integration test #1671
Add OTLP exporter integration test #1671
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1671 +/- ##
=======================================
Coverage 82.06% 82.06%
=======================================
Files 249 249
Lines 6730 6730
=======================================
Hits 5523 5523
Misses 1207 1207
|
} | ||
} | ||
|
||
public static string GetEnvironmentVariable(string environmentVariableName) |
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.
I was going to say you could call SkipUnlessEnvVarFoundTheoryAttribute.GetEnvironmentVariable
to achieve some code reuse but then you would have to also include that file in the project which kind of sucks. Probably the duplication is better. So I guess what I'm saying is, nice job 😄
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.
Yea I had some similar thoughts then opted for duplication. We could play around with organizing things like this into a "xUnit extensions" namespace and then you could do something like
<Compile Include="$(RepoRoot)\test\OpenTelemetry.Tests\xunit\**" />
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/DelegatingTestExporter.cs
Outdated
Show resolved
Hide resolved
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.
LGTM
{ | ||
var exporterOptions = new OtlpExporterOptions | ||
{ | ||
Endpoint = CollectorEndpoint, |
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.
Since #1662 is merged, we need to conditionally use Uri/String here.
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.
Approved, need to fix the build issue before merging.
LOL |
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net452;net46</TargetFrameworks> | ||
<TargetFrameworks Condition="$(TARGET_FRAMEWORK) == ''">netcoreapp2.1;netcoreapp3.1;net5.0</TargetFrameworks> | ||
<TargetFrameworks Condition="$(TARGET_FRAMEWORK) == '' AND $(OS) == 'Windows_NT'">$(TargetFrameworks);net452;net46</TargetFrameworks> | ||
<TargetFrameworks Condition="$(TARGET_FRAMEWORK) != ''">$(TARGET_FRAMEWORK)</TargetFrameworks> |
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 third line seems kind of redundant 🤷
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.
Maybe not. This is basically allowing $(TARGET_FRAMEWORK) to override what is in csproj? So you can pass it on the command-line?
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.
Correct, TARGET_FRAMEWORK
is a property that is set when building the project for running the integration tests
opentelemetry-dotnet/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Dockerfile
Line 12 in 808ae2f
RUN dotnet publish "OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests.csproj" -c "${PUBLISH_CONFIGURATION}" -f "${PUBLISH_FRAMEWORK}" -o /drop -p:IntegrationBuild=true -p:TARGET_FRAMEWORK=${PUBLISH_FRAMEWORK} |
Adds a basic integration test for the OTLP exporter. Provides a starting place to expand the test coverage further. I'd like to land this PR and integrate it into #1662. This will enable testing SuppressInstrumentation and validating that #1662 fixes #1251.