coverlet-coverage / coverlet

Cross platform code coverage for .NET
MIT License
2.93k stars 385 forks source link

[BUG] Documentation states you can exclude by attribute with short name or full name - cannot by long name #1589

Closed tonyhallett closed 4 months ago

tonyhallett commented 5 months ago

Describe the bug From https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/GlobalTool.md

You can also ignore additional attributes by using the ExcludeByAttribute property (short name or full name supported):

To Reproduce Create a custom attribute, apply it at any level. Supply fully qualified through one of the drivers

Expected behavior Is it excluded

Actual behavior Is not excluded

Additional context https://github.com/coverlet-coverage/coverlet/blob/a0bd87d70f899d955891956ce4173e8bf31c16b0/src/coverlet.core/Instrumentation/Instrumenter.cs#L775

None of the Instrumenter tests supply the attributesToIgnore as fully qualified e.g https://github.com/coverlet-coverage/coverlet/blob/a0bd87d70f899d955891956ce4173e8bf31c16b0/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs#L150

daveMueller commented 5 months ago

OK I see the problem. From my point of view we should adapt the documentation and/or consider implementing also the fully qualified attribute name. I don't have any details about why it was decided in the past for coverlet to only check the attribute name. Maybe forgotten to consider or the one implementing this didn't see the need for it. But from my point of view, it wouldn't be much implementation effort to additionally check for the fully qualified name.

I just wonder about the use cases a bit? The only thing I can think of is someone having two attributes with the same name in different namespaces of his application and wants to exclude both of them.

@MarcoRossignoli @Bertk What do you think? Should we implement this? Even if it is just one use case I can think of, it seems valid to me.

tonyhallett commented 5 months ago

Changing the documentation is probably sufficient. Having two similarly named attribute types serving different purposes seems highly unlikely. Bringing it to your attention as it is a bug given the documentation. Fine Code Coverage has been following the documentation all this time. I expect that the only time you would encounter this bug is with .Net Framework where you cannot apply ExcludeFromCodeCoverage at the assembly level. You create and apply your own exclusion attribute and decide to qualify in coverlet.

Bertk commented 5 months ago

@daveMueller I think full name shall be supported.

MarcoRossignoli commented 5 months ago

I don't recall but maybe we did some regression here, we could support full name too.

daveMueller commented 5 months ago

OK I work on this...