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

Removed Moq dependency from Instrumentation.AspNetCore.Tests #5123

Conversation

ngruson
Copy link
Contributor

@ngruson ngruson commented Dec 3, 2023

Related to #4758.

Changes

  • Refactored BasicTests.cs
  • Removed Moq dependency

@ngruson ngruson requested a review from a team December 3, 2023 13:33
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Merging #5123 (d1e8510) into main (3619a6f) will increase coverage by 0.18%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head d1e8510 differs from pull request most recent head 39a9998. Consider uploading reports for the commit 39a9998 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5123      +/-   ##
==========================================
+ Coverage   82.85%   83.04%   +0.18%     
==========================================
  Files         296      296              
  Lines       12320    12320              
==========================================
+ Hits        10208    10231      +23     
+ Misses       2112     2089      -23     
Flag Coverage Δ
unittests 83.04% <ø> (+0.18%) ⬆️

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

see 7 files with indirect coverage changes

Comment on lines 24 to 26
#pragma warning disable SA1010
public List<string> ExtractValues = [];
public Dictionary<string, Func<PropagationContext, string>> InjectValues = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

SA1010: OpeningSquareBracketsMustBeSpacedCorrectly

Is the pragma warning disable because of the List and Dictionary initializations (Lines 25 & 26)?
Just curious if the pragma disable could be removed if we used = new() instead?

Copy link
Contributor Author

@ngruson ngruson Dec 4, 2023

Choose a reason for hiding this comment

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

@TimothyMothra :
Yes, indeed.
If I use = new() I get the following compiler warning:
IDE0028 (Simplify collection initialization)

Copy link
Contributor

Choose a reason for hiding this comment

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

haha. okay, then leave it as you had it. Thanks for checking :)

internal sealed class CustomTextMapPropagator : TextMapPropagator
{
#pragma warning disable SA1010
public List<string> ExtractValues = [];
Copy link
Member

Choose a reason for hiding this comment

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

nit: Define traceId, spanId and flags as individual fields.

ngruson added a commit to ngruson/opentelemetry-dotnet that referenced this pull request Dec 5, 2023
// limitations under the License.
// </copyright>

using OpenTelemetry.Context.Propagation;
Copy link
Member

Choose a reason for hiding this comment

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

can be removed now that we are using Action delegate

@utpilla utpilla merged commit a963ec8 into open-telemetry:main Dec 6, 2023
40 checks passed
@ngruson ngruson deleted the remove-moq-instrumentation-aspnetcore-tests branch December 6, 2023 06:20
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.

4 participants