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

Add a filter for namespaces #385

Closed
manfred-brands opened this issue Sep 8, 2020 · 8 comments
Closed

Add a filter for namespaces #385

manfred-brands opened this issue Sep 8, 2020 · 8 comments

Comments

@manfred-brands
Copy link
Contributor

Describe the bug
Our assembly has internal .NUnit classes in the same assembly as the actual code.
To get proper code coverage reports I would like to exclude *.NUnit.* classes as they skew the metrics.
I tried this with a -classfilters:-*.NUnit.* argument but this didn't work.

Debugging the issue, I found that the class filter is called with ClassName and not FullName
I misinterpreted that class name does not include namespace.
I managed to sort of get what I needed by using -classfilters:-*Fixture, but that doesn't catch all test classes.

To not break existing functionality, can we add an -namespacefilters argument?

I don't mind contributing this functionality myself.

@danielpalme
Copy link
Owner

It's more work to add this feature as you might expect:

@manfred-brands
Copy link
Contributor Author

Thanks for pointing out all area. I will give it a go.

@manfred-brands
Copy link
Contributor Author

manfred-brands commented Sep 8, 2020

I have implemented this for the DynamicCodeCoverageParser and updated all files, except the Azure Task/Github Action.

Whilst checking the other parsers, I noticed that all other parsers pass the full class name to the classfilter.

E.g. DotCoverParser combines them explicitly:

    Select(c => c.Parent.Attribute("Name").Value + "." + c.Attribute("Name").Value)

So does the VisualStudioParser:

    return c.Parent.Parent.Element("NamespaceName").Value + "." + fullname;

The LCovParser and GCovParser actually pass the fileName + extension to the class filter:

   string className = fileName.Substring(fileName.LastIndexOf(Path.DirectorySeparatorChar) + 1);

The JaCoCoParser passes items like "test/AbstractClass_SampleImpl1". This might need stripping the "test/" prefix, but I don't know the format.

Only the DynamicCodeCoverageParser seems to explicitly separate them into a local ClassWithNamespace class.
This was added as part of issue #275.

So how do you want me to go on from here?

  • Use the full name in the class filter for DynamicCodeCoverageParser to match the others?
  • Split the full name in all other parsers into Namespace and ClassName and use two filters?

I have pushed up my work so far as a draft pull request #387

@danielpalme
Copy link
Owner

danielpalme commented Sep 8, 2020

I would suggest, that DynamicCodeCoverageParser should match the behavior of the other parsers.
But let me review the different parsers and the changes of #275 before we continue.

LCovParserand GCovParser might be different because these formats are pretty limited.

@danielpalme
Copy link
Owner

danielpalme commented Sep 8, 2020

I reviewed the code and the problem is, that the namespace is not considered in the filter.

By changing .Where(c => this.ClassFilter.IsElementIncludedInReport(c.ClassName)) to .Where(c => this.ClassFilter.IsElementIncludedInReport(c.FullName)) filtering by namespace should be fully supported as in the other parsers.
I already applied that change in 2290f09.

I guess adding a separate namespace filter is obsolete. But thanks for your effort!

@danielpalme
Copy link
Owner

Release 4.6.7 includes the fix.

@manfred-brands
Copy link
Contributor Author

I guess adding a separate namespace filter is obsolete. But thanks for your effort!
It is obsolete, but was a good exercise going through the code structure.

I only had suggested it based upon the one parser I was using and I didn't want to break that functionality, not realising that it was the only parser behaving that way. Maybe you can update the documentation to say that the class filter works on the full class name including namespace.

You probably want to fix the broken parsers:

The LCovParser and GCovParser actually pass the fileName + extension to the class filter:

   string className = fileName.Substring(fileName.LastIndexOf(Path.DirectorySeparatorChar) + 1);

Use string className = Path.GetFileNameWithoutExtension(fileName); instead.

@manfred-brands
Copy link
Contributor Author

Release 4.6.7 includes the fix.

Works properly with my original filter: -*.NUnit.*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants