Add `[xunit]` to default excluded modules filter if not specified #462

MarcoRossignoli · 2019-06-10T14:08:24Z

closes #461

src/coverlet.core/Helpers/InstrumentationHelper.cs

MarcoRossignoli · 2019-06-10T15:00:17Z

msbuild test

MarcoRossignoli · 2019-06-10T15:04:38Z

Test with console

C:\git\coverlet\src\coverlet.console\bin\Debug\netcoreapp2.2 (defaultexcludedfiles -> origin)
λ dotnet coverlet.console.dll "C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.tests.dll" --target "dotnet" --targetargs "test C:\git\coverlet\test\coverlet.core.tests\coverlet.core.tests.csproj --no-build" --verbosity detailed
Excluded module filter '[xunit*]*'
Instrumented module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.dll'
Excluded module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.reporters.netcoreapp10.dll'
Excluded module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.utility.netcoreapp10.dll'
Excluded module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.visualstudio.dotnetcore.testadapter.dll'
Test run for C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.tests.dll(.NETCoreApp,Version=v2.0)
Microsoft (R) Test Execution Command Line Tool Version 16.1.0
Copyright (c) Microsoft Corporation.  All rights reserved.
Starting test execution, please wait...
[xUnit.net 00:00:01.4208241]     Coverlet.Core.Tests.Instrumentation.ModuleTrackerTemplateTests.HitsOnMultipleThreadsCorrectlyCounted [SKIP]
  ! Coverlet.Core.Tests.Instrumentation.ModuleTrackerTemplateTests.HitsOnMultipleThreadsCorrectlyCounted [1ms]
[xUnit.net 00:00:03.7948155]     Coverlet.Core.Instrumentation.Tests.InstrumenterTests.TestCoreLibInstrumentation [SKIP]
  ! Coverlet.Core.Instrumentation.Tests.InstrumenterTests.TestCoreLibInstrumentation [1ms]
Test Run Successful.
Total tests: 113
     Passed: 111
    Skipped: 2
 Total time: 4,5155 Seconds

C:\git\coverlet\src\coverlet.console\bin\Debug\netcoreapp2.2 (defaultexcludedfiles -> origin)
λ dotnet coverlet.console.dll "C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.tests.dll" --target "dotnet" --targetargs "test C:\git\coverlet\test\coverlet.core.tests\coverlet.core.tests.csproj --no-build" --verbosity detailed --exclude AAA
Excluded module filter 'AAA'
Instrumented module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.dll'
Instrumented module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.reporters.netcoreapp10.dll'
Instrumented module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.utility.netcoreapp10.dll'
Instrumented module: 'C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\xunit.runner.visualstudio.dotnetcore.testadapter.dll'
Test run for C:\git\coverlet\test\coverlet.core.tests\bin\Debug\netcoreapp2.0\coverlet.core.tests.dll(.NETCoreApp,Version=v2.0)
Microsoft (R) Test Execution Command Line Tool Version 16.1.0
Copyright (c) Microsoft Corporation.  All rights reserved.
Starting test execution, please wait...
[xUnit.net 00:00:01.5159707]     Coverlet.Core.Tests.Instrumentation.ModuleTrackerTemplateTests.HitsOnMultipleThreadsCorrectlyCounted [SKIP]
  ! Coverlet.Core.Tests.Instrumentation.ModuleTrackerTemplateTests.HitsOnMultipleThreadsCorrectlyCounted [1ms]
[xUnit.net 00:00:03.9041438]     Coverlet.Core.Instrumentation.Tests.InstrumenterTests.TestCoreLibInstrumentation [SKIP]
  ! Coverlet.Core.Instrumentation.Tests.InstrumenterTests.TestCoreLibInstrumentation [1ms]
Test Run Successful.
Total tests: 113
     Passed: 111
    Skipped: 2
 Total time: 4,7460 Seconds

MarcoRossignoli · 2019-06-10T15:23:13Z

@vagisha-nidhi PTAL

MarcoRossignoli · 2019-06-10T15:30:14Z

PS. x @tonerdo, all this non-testable code of console/msbuild makes me a bit nervous 😄 we should inject everything #410 (comment), collectors are completely testable, I don't need any manual test.

vagisha-nidhi · 2019-06-10T15:58:02Z

@vagisha-nidhi PTAL

LGTM! 👍

pape77 · 2019-07-02T13:20:29Z

src/coverlet.console/Program.cs

@@ -57,6 +57,12 @@ static int Main(string[] args)
                    logger.Level = verbosity.ParsedValue;
                }

+                // We add default exclusion filter if no specified
+                if (excludeFilters.Values.Count == 0)


Just curious, why did we chose to include if Count == 0 instead of excludeFilters.Values.Contains("[xunit*]*")?
Since i cannot think an scenario in which we want to include this dlls, and maybe we forgot to include it in our custom exclude filters. Or we missincluded it using the wrong wildcards or whatever
@MarcoRossignoli

Comment says "We add default exclusion filter if no specified" but you are not adding the CoverletConstants. DefaultExcludeFilter here, but the xunit one, which i find confusing

Since i cannot think an scenario in which we want to include this dlls

The idea is to remove only in default config...after first include users win.

When are you adding the CoverletConstants. DefaultExcludeFilter btw?
Not sure about the code, but if this later goes into CoverletSettingsParser then it will never reach the code you added in that file (the if statement), since you add this before reaching that part, and then all it does is add the constants, so count is always 2
If you pass some custom exclude filters as argument, here you don't add xunit, and later count will be >1 , then your if statement is also not excecuted...

CoverletConstants.DefaultExcludeFilter is needed only by collector.

Today we've 3 entry point/driver for coverlet console/msbuild/collector they behave in a different way on defaults.

check manual tests FYI
#462 (comment)
#462 (comment)

pape77 · 2019-07-02T13:27:50Z

src/coverlet.collector/DataCollection/CoverletSettingsParser.cs

@@ -131,6 +131,13 @@ private string[] ParseExcludeFilters(XmlElement configurationElement)
                }
            }

+            // if we've only one element mean that we only added CoverletConstants.DefaultExcludeFilter


wouldn't make that assumption, but more like excludeFilters.Contains(CoverletConstants.DefaultExcludeFilter) && excludeFilters.Count==1
is way safer.

CoverletConstants.DefaultExcludeFilter will be always present

pape77 · 2019-07-02T13:36:57Z

Why not including xunit to the CoverletConstants.DefaultExcludeFilter directly?
Or someone might name it's project xunit as well and then it's on a dll? By default they will be screwed as well anyways :P

MarcoRossignoli · 2019-07-02T13:51:32Z

Or someone might name it's project xunit as well and then it's on a dll?

If we fix that filter we won't have plan b for possible name collision

pape77 · 2019-07-02T14:08:25Z

Or someone might name it's project xunit as well and then it's on a dll?

If we fix that filter we won't have plan b for possible name collision

Same for [coverlet*]*] :)
It's more easily fixable from the user side, to just rename their project to something that doesn't start with xunit and done...
You may argue is more unlikely to use that same name for a project but maybe same for xunit.
Now you have different custom behaviour when you include or not a filter. So you have to remember to also exclude xunit if you decide to exclude something by yourself (bit annoying)

Btw. My pr which i pointed before, magically excluded all dlls that have nothing to do to your code itself without any exclude filters. You might want to take a look to see if it's still applicable.
That's what I'm currently using as a fork

MarcoRossignoli · 2019-07-02T14:20:50Z

Sure @pape77 we can go on with discussion on that PR

MarcoRossignoli requested a review from tonerdo June 10, 2019 14:09

vagisha-nidhi reviewed Jun 10, 2019

View reviewed changes

src/coverlet.core/Helpers/InstrumentationHelper.cs Outdated Show resolved Hide resolved

MarcoRossignoli changed the title ~~Add [xunit*]* to default excluded modules filter if not specified~~ [WIP]Add [xunit*]* to default excluded modules filter if not specified Jun 10, 2019

exclude xunit if empty exclude filters

773a23c

MarcoRossignoli force-pushed the defaultexcludedfiles branch from 9d82ca0 to 773a23c Compare June 10, 2019 15:21

MarcoRossignoli changed the title ~~[WIP]Add [xunit*]* to default excluded modules filter if not specified~~ Add [xunit*]* to default excluded modules filter if not specified Jun 10, 2019

udpate test

a01a5cb

tonerdo approved these changes Jun 10, 2019

View reviewed changes

MarcoRossignoli merged commit 4e2c357 into coverlet-coverage:master Jun 10, 2019

MarcoRossignoli deleted the defaultexcludedfiles branch June 10, 2019 18:56

MarcoRossignoli mentioned this pull request Jul 2, 2019

Removes the not instrumented/unused instrumenter results so coverage file doesn't get polluted with unused modules #225

Closed

pape77 reviewed Jul 2, 2019

View reviewed changes

MarcoRossignoli mentioned this pull request Aug 14, 2019

Revert "Add [xunit*]* to default excluded modules filter if not specified" #519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `[xunit]` to default excluded modules filter if not specified #462

Add `[xunit]` to default excluded modules filter if not specified #462

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

vagisha-nidhi commented Jun 10, 2019

pape77 Jul 2, 2019 •

edited

Loading

pape77 Jul 2, 2019

MarcoRossignoli Jul 2, 2019

pape77 Jul 2, 2019 •

edited

Loading

MarcoRossignoli Jul 2, 2019 •

edited

Loading

MarcoRossignoli Jul 2, 2019

MarcoRossignoli Jul 2, 2019

pape77 Jul 2, 2019 •

edited

Loading

MarcoRossignoli Jul 2, 2019

pape77 commented Jul 2, 2019

MarcoRossignoli commented Jul 2, 2019

pape77 commented Jul 2, 2019 •

edited

Loading

MarcoRossignoli commented Jul 2, 2019

Add [xunit*]* to default excluded modules filter if not specified #462

Add [xunit*]* to default excluded modules filter if not specified #462

Conversation

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

MarcoRossignoli commented Jun 10, 2019

vagisha-nidhi commented Jun 10, 2019

pape77 Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

pape77 Jul 2, 2019

Choose a reason for hiding this comment

MarcoRossignoli Jul 2, 2019

Choose a reason for hiding this comment

pape77 Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Jul 2, 2019

Choose a reason for hiding this comment

MarcoRossignoli Jul 2, 2019

Choose a reason for hiding this comment

pape77 Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Jul 2, 2019

Choose a reason for hiding this comment

pape77 commented Jul 2, 2019

MarcoRossignoli commented Jul 2, 2019

pape77 commented Jul 2, 2019 • edited Loading

MarcoRossignoli commented Jul 2, 2019

Add `[xunit]` to default excluded modules filter if not specified #462

Add `[xunit]` to default excluded modules filter if not specified #462

pape77 Jul 2, 2019 •

edited

Loading

pape77 Jul 2, 2019 •

edited

Loading

MarcoRossignoli Jul 2, 2019 •

edited

Loading

pape77 Jul 2, 2019 •

edited

Loading

pape77 commented Jul 2, 2019 •

edited

Loading