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] Add .NET 8 target for W3C Trace Context Integration Test in CI #5800

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Aug 21, 2024

Towards #5799
Design discussion issue #

Changes

  • Add .NET 8 in CI to run W3C Trace Context Integration Test.
  • Fixed Dockerfile to build successfully by adding --break-system-packages. (mcr.microsoft.com/dotnet/sdk:net6.0 image uses Python 3.9, which doesn't have --break-system-packages option, thus adding an alternative to run without this option if it fails.)

The W3C integration test on .NET 8 has 40 failing test cases, which I asserted with Assert.StartsWith("FAILED (failures=40)", lastLine); in this PR.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • [ ] Unit tests added/updated
  • [ ] Appropriate CHANGELOG.md files updated for non-trivial changes
  • [ ] Changes in public API reviewed (if applicable)

@github-actions github-actions bot added the infra Infra work - CI/CD, code coverage, linters label Aug 21, 2024
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.34%. Comparing base (6250307) to head (d348b4d).
Report is 305 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
+ Coverage   83.38%   86.34%   +2.95%     
==========================================
  Files         297      257      -40     
  Lines       12531    11150    -1381     
==========================================
- Hits        10449     9627     -822     
+ Misses       2082     1523     -559     
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 85.95% <ø> (?)
unittests-Project-Stable 86.20% <ø> (?)
unittests-Solution 86.32% <ø> (?)
unittests-UnstableCoreLibraries-Experimental 85.76% <ø> (?)

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

see 213 files with indirect coverage changes

@xiang17 xiang17 marked this pull request as ready for review August 21, 2024 23:17
@xiang17 xiang17 requested a review from a team August 21, 2024 23:17
@@ -88,6 +88,10 @@ public void W3CTraceContextTestSuiteAsync(string value)
{
Assert.StartsWith("FAILED (failures=3)", lastLine);
}
else if (AspNetCoreHostingVersion.Major == 8)
{
Assert.StartsWith("FAILED (failures=40)", lastLine);
Copy link
Member

Choose a reason for hiding this comment

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

@xiang17 - Could you further investigate to check what is the difference between net8.0 and other versions. Thats lot of failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not be trivial to fix. Can we complete this PR first? The issue can be kept open to track the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiang17, I do not think that Vishwesh is asking for the fix in the scope of the PR, but it will be great to find what is the root cause.

Copy link
Member

@vishweshbankwar vishweshbankwar Aug 29, 2024

Choose a reason for hiding this comment

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

Yes, just looking for the root cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've figured it out. Apparently .NET 8 on Linux changed the default port for ASP.NET Core (but not changed on Windows, which explained why local Windows works but not in Docker).

…inux changed. (But it's not changed on Windows, which explains why the test would pass on Windows if you download the W3C tests locally and change the path at line `RunCommand("python", "trace-context/test/test.py http://localhost:5000/")`.)
@CodeBlanch CodeBlanch changed the title Add .NET 8 target for W3C Trace Context Integration Test in CI [repo] Add .NET 8 target for W3C Trace Context Integration Test in CI Sep 4, 2024
@CodeBlanch CodeBlanch merged commit dc8d3fd into open-telemetry:main Sep 4, 2024
43 checks passed
@xiang17 xiang17 deleted the xiang17/CIDotnet8Target branch November 15, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infra work - CI/CD, code coverage, linters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants