-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add AttributeTargets.Interface to ExcludeFromCodeCoverageAttribute and StackTraceHiddenAttribute #123686
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
base: main
Are you sure you want to change the base?
Conversation
…d StackTraceHiddenAttribute Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…assembly Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
...stem.Runtime/tests/System.Runtime.Tests/System/Diagnostics/StackTraceHiddenAttributeTests.cs
Outdated
Show resolved
Hide resolved
…/Diagnostics/StackTraceHiddenAttributeTests.cs
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics |
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.
Pull request overview
This PR adds AttributeTargets.Interface to both ExcludeFromCodeCoverageAttribute and StackTraceHiddenAttribute to support interfaces with default implementation members (C# 8+) that contain executable code.
Changes:
- Added
AttributeTargets.InterfacetoExcludeFromCodeCoverageAttributeandStackTraceHiddenAttributein implementation and reference assemblies - Added API compatibility suppressions for the attribute usage changes
- Added tests verifying the attributes can be applied to interfaces
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/ExcludeFromCodeCoverageAttribute.cs | Added AttributeTargets.Interface to the attribute usage declaration in alphabetical order |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTraceHiddenAttribute.cs | Added AttributeTargets.Interface to the attribute usage declaration in alphabetical order |
| src/coreclr/nativeaot/Test.CoreLib/src/System/Diagnostics/StackTraceHiddenAttribute.cs | Added AttributeTargets.Interface to maintain consistency with the main implementation |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updated reference assembly to reflect the attribute usage changes for both attributes |
| src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml | Added suppressions for CP0015 diagnostic due to attribute usage changes across multiple assemblies |
| src/libraries/apicompat/ApiCompatBaseline.netstandard2.1.xml | Added suppression for CP0015 diagnostic for netstandard compatibility |
| src/libraries/System.Runtime/tests/System.Diagnostics.Tools.Tests/System/Diagnostics/CodeAnalysis/ExcludeFromCodeCoverageAttributeTests.cs | Added test verifying ExcludeFromCodeCoverageAttribute can be applied to interfaces |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Diagnostics/StackTraceHiddenAttributeTests.cs | Added test verifying StackTraceHiddenAttribute can be applied to interfaces |
| } | ||
|
|
||
| [Fact] | ||
| public void InterfaceHidden_CanApplyAttribute() |
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.
Should we instead have a test that checks the methods on the interface are actually hidden in stack traces?
The tests in this file all tests the actual behavior (Exception.StackTrace/Environment.StackTrace). This added test is more of a reflection test than anything else and I'm pretty sure we have such coverage elsewhere.
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.
Replaced the reflection test with a behavioral test (InterfaceHidden_EnvironmentStackTrace) that verifies default interface methods are actually hidden from stack traces, consistent with the other tests in this file. Commit b572aee.
…e hiding Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| string stacktrace = ((IHiddenInterface)new HiddenInterfaceImpl()).GetStackTraceMethodA(); | ||
| Assert.DoesNotContain(nameof(IHiddenInterface.GetStackTraceMethodA), stacktrace); | ||
| Assert.DoesNotContain(nameof(IHiddenInterface.GetStackTraceMethodB), stacktrace); | ||
| Assert.DoesNotContain(nameof(IHiddenInterface.GetStackTraceMethodC), stacktrace); |
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.
@copilot can you add a similar INotHiddenInterface and validate that with exactly the same setup except for the attribute that those stacktraces do contain the relevant method names?
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.
Description
Interfaces with default implementation members (C# 8+) can contain executable code that appears in code coverage reports. This PR adds
AttributeTargets.Interfaceto both attributes as approved in API review.Changes
AttributeTargets.InterfacetoExcludeFromCodeCoverageAttributeandStackTraceHiddenAttributein System.Private.CoreLibStackTraceHiddenAttributein Test.CoreLibSystem.Runtime.csApiCompatBaseline.NetCoreAppLatestStable.xml(one entry per assembly where the attribute is type-forwarded:System.Diagnostics.Tools.dll,System.Runtime.dll,netstandard.dll, andSystem.dll) andApiCompatBaseline.netstandard2.1.xml[StackTraceHidden]are actually hidden from stack tracesAPI Diff
namespace System.Diagnostics.CodeAnalysis { - [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | ... | AttributeTargets.Event, ...)] + [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Class | ... | AttributeTargets.Event | AttributeTargets.Interface, ...)] public sealed class ExcludeFromCodeCoverageAttribute : Attribute; } namespace System.Diagnostics { - [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Constructor | AttributeTargets.Method, ...)] + [AttributeUsage(AttributeTargets.Class | ... | AttributeTargets.Method | AttributeTargets.Interface, ...)] public sealed class StackTraceHiddenAttribute : Attribute; }Example
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.