Skip to content
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

[repo] Dedicated workflows for Instrumentation.Process & Exporter.OneCollector #1355

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

CodeBlanch
Copy link
Member

Relates to #1348
Relates to #1347

Changes

Cleanup:

  • Adds Microsoft.NET.Test.Sdk reference to test projects via common.nonprod.props. Removed direct references from test projects.
  • Removes coverlet.collector where it was being referenced. We're not using it.
  • Removes Microsoft.CodeCoverage reference from common.props. Not sure why it was even there. Tests projects where it is needed will now get it transitively via Microsoft.NET.Test.Sdk.
  • Removes Condition="$([MSBuild]::IsOsPlatform('Windows'))" from xunit.runner.visualstudio where it was defined in test projects. That prevents tests from running on linux in CI.
  • For code coverage we now use dotnet-coverage global tool instead of the powershell script + reportgenerator global tool.

Improvements:

  • Introduces dedicated CI for OpenTelemetry.Process.Instrumentation & OpenTelemetry.Exporter.OneCollector. Other projects can be moved to this style, I just did these two because a) some work was already done for OpenTelemetry.Process.Instrumentation and b) OpenTelemetry.Exporter.OneCollector now has a special manually-approved step ([OneCollector] Integration tests prep #1348) which we don't want every PR to have to suffer.

Maybe Improvements:

  • Our code coverage doesn't seem to work very well. I tried to sort out the mess. May or may not improve things as far as the report, but it should run faster at least!

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #1355 (e7c0150) into main (71655ce) will increase coverage by 0.22%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
+ Coverage   73.91%   74.14%   +0.22%     
==========================================
  Files         267      258       -9     
  Lines        9615     9472     -143     
==========================================
- Hits         7107     7023      -84     
+ Misses       2508     2449      -59     
Flag Coverage Δ
unittests 72.22% <100.00%> (?)
unittests-Exporter.OneCollector 89.71% <ø> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 100.00%
....Owin/Implementation/OwinInstrumentationMetrics.cs 100.00%
...inInstrumentationMeterProviderBuilderExtensions.cs 100.00%

.github/workflows/ci.yml Outdated Show resolved Hide resolved
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkPkgVer)" Condition="'$(IsTestProject)' == 'true'" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up PR - probably we can do the same with xunit, dotnet-xunit and xunit-visualstudio-runner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to that. But I didn't here because I thought I saw one project using MSTest (not xunit). Personally I am OK with component owners using a different framework if they want, no reason in my mind that they MUST use xunit. But we could simplify it down by using a property like <XUnitEnabled>true</XUnitEnabled> and then use that to add in whatever references we need?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with opt-out for XUnitEnabled for all tests packages as major (all?) of our packages are based on this.

BTW I cannot find the MSTest reference in this repository.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked, I couldn't find it either. Maybe I imagined it 😄 Anyway opened this: #1363

@CodeBlanch CodeBlanch merged commit bd71b46 into open-telemetry:main Sep 20, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.