Implementation exclusion of files according to source file paths #64

ido-namely · 2018-04-26T04:46:32Z

(including file globbing)
Goes to #56

…luding file globbing)

ido-namely · 2018-04-26T05:06:18Z

Would love to hear what you think is the best way to test this feature

MarcoRossignoli · 2018-04-26T07:31:45Z

Would love to hear what you think is the best way to test this feature

@tonerdo i'm not a "msbuild workflow" expert...but if we could have a simple guide/resouce link "how to test/debug" this project will grow much faster(very likely this is obvious for a msbuild workflow dev).

MarcoRossignoli · 2018-04-26T07:45:45Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

+            return fileMatchResult.Files
+                .Select(
+                    f => System.IO.Path.GetFullPath(
+                        System.IO.Path.Combine(directoryInfo.ToString(), f.Path)


i think instead of directoryInfo.ToString() you can use parentDir right?
https://source.dot.net/#System.IO.FileSystem/System/IO/DirectoryInfo.cs,29
https://source.dot.net/#System.IO.FileSystem/System/IO/FileSystemInfo.cs,108

nit: you can avoid System.IO. and use Path.xxx(using System.IO; )

I could, but I'd rather be consistent with directoryInfo as being the source of truth after passing it parentDir, thanks to the sources you've provided I could verify this is basically the same (ToString just passes the value given in the ctor), Thanks!

Will Do, Thanks!

MarcoRossignoli · 2018-04-26T07:52:46Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

 namespace Coverlet.Core.Helpers
 {
-    internal static class InstrumentationHelper
+    public static class InstrumentationHelper


@tonerdo maybe[InternalsVisibleToAttribute]?

I wasn't sure about this, whatever you guys think, it's also not essential to have to put that new method in that class.

Please lets leave this as internal

MarcoRossignoli · 2018-04-26T08:12:22Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

        }
+
+        public static string[] GetExcludedFiles(string[] exclusionRules, string parentDir) {
+            if (exclusionRules == null || exclusionRules.Length == 0 ) return null;


IMHO usually i prefer Array.Empty<string>() https://docs.microsoft.com/en-us/dotnet/api/system.array.empty?view=netcore-2.0 less check condition/code for internal use.

MarcoRossignoli · 2018-04-26T08:22:07Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

+using Microsoft.Extensions.FileSystemGlobbing;

 using Coverlet.Core.Instrumentation;
+using Microsoft.Extensions.FileSystemGlobbing.Abstractions;


@tonerdo one question...do you have some style guide? On corefx repo the standard is
6. Namespace imports should be specified at the top of the file, outside of namespace declarations and should be sorted alphabetically.

MarcoRossignoli · 2018-04-26T08:24:56Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

 using System.IO;
 using System.Linq;
 using System.Reflection.PortableExecutable;
+using Microsoft.Extensions.FileSystemGlobbing;


i did't know this ext...thank's!

MarcoRossignoli · 2018-04-26T08:43:02Z

@ido-namely one useful think i've learn from my contributions(on very first one) is to use "special key" to link PR to issue for simple "navigation".

https://help.github.com/articles/closing-issues-using-keywords/

if for instance on PR explanation you write:

closes #56  (including file globbing)

a link is automatically created on issue thread and you don't need to write anything on issue thread anymore but discussion move on PR.

abe545 · 2018-04-26T16:35:18Z

src/coverlet.core/Instrumentation/Instrumenter.cs

        {
            _module = module;
            _identifier = identifier;
+            _excludedFiles = excludedFiles;


if you change this to _excludedFiles = excludedFiles ?? Enumerable.Empty<string>() you won't need the null checks elsewhere

abe545 · 2018-04-26T16:39:37Z

src/coverlet.core/Instrumentation/Instrumenter.cs

                    foreach (var method in type.Methods)
                    {
+                        var sourceFiles = method.DebugInformation.SequencePoints.Select(s => s.Document.Url).Distinct();
+                        if (_excludedFiles != null && sourceFiles.Any(_excludedFiles.Contains)) {


Odd that the method could be declared in multiple files (I guess this is the case with partial methods in partial classes). If this is indeed case, you'd only want to exclude the method if sourceFiles.All(...). @tonerdo you're obviously a lot more familiar with this - am I interpreting this correctly?

Well we can't have partial methods thats for sure. I think a .First(...) will suffice in this case

They are part of the language: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/partial-method

If it isn't something that is supported in coverlet, then I guess we don't need to worry about it.

OMG I've never even heard of partial methods until today. I guess we learn everyday. Taking my new found knowledge into consideration I'll still go with .First(...), reason being that sequence points start at the body of the method and no matter how many partials of the same method you have, you can only have one implementation (hence, one body) and the document that implementation is contained in is where all sequence points for that method will point to

tonerdo · 2018-04-26T18:27:24Z

@ido-namely @MarcoRossignoli See steps to build and test project here: https://github.com/tonerdo/coverlet#building-the-project

tonerdo · 2018-04-28T10:17:39Z

src/coverlet.msbuild/coverlet.msbuild.props

    <CoverletOutputDirectory Condition="$(CoverletOutputDirectory) == ''">$(MSBuildProjectDirectory)</CoverletOutputDirectory>
    <CoverletOutputName Condition=" '$(CoverletOutputName)' == '' ">coverage</CoverletOutputName>
    <CoverletOutput>$([MSBuild]::EnsureTrailingSlash('$(CoverletOutputDirectory)'))$(CoverletOutputName)</CoverletOutput>
+    <CoverletExclusionParentDir Condition="$(CoverletExclusionParentDir) == ''">$(MSBuildProjectDirectory)</CoverletExclusionParentDir>


Lets default to the current directory as the parent if the path the user provides is relative. Too many options can be confusing

tonerdo · 2018-04-28T10:19:03Z

src/coverlet.msbuild/coverlet.msbuild.targets

  <Target Name="InstrumentModulesNoBuild" BeforeTargets="VSTest">
    <Coverlet.MSbuild.Tasks.InstrumentationTask
      Condition="'$(VSTestNoBuild)' == 'true' and $(CollectCoverage) == 'true'"
+      ExclusionRules="$(ExclusionRules)"


Let's simply call this Exclude. Also you need to define it in the props file

tonerdo · 2018-04-28T10:19:36Z

src/coverlet.msbuild.tasks/InstrumentationTask.cs

+        public string Path { get; set; }
+
+        [Required]
+        public string[] ExclusionRules { get; set; }


s/ExclusionRules/Exclude

tonerdo · 2018-04-28T10:21:27Z

src/coverlet.msbuild.tasks/InstrumentationTask.cs

-                _coverage.PrepareModules();
+                var excludedFiles =  InstrumentationHelper.GetExcludedFiles(
+                    ExclusionRules, ExclusionParentDir);
+                Coverage = new Coverage(Path, Guid.NewGuid().ToString(), excludedFiles);


InstrumentationHelper shouldn't be used outside the core assembly. Pass the exclusion rules into the Coverage constructor and figure out the excluded files from there

tonerdo · 2018-04-28T10:22:22Z

src/coverlet.core/Helpers/InstrumentationHelper.cs

        }
+
+        public static string[] GetExcludedFiles(string[] exclusionRules, string parentDir) {
+            if (!(exclusionRules?.Length > 0) ) return null;


We should instead return an empty array here so we don't have to do null checks in places that use the result of this method

tonerdo

LGTM! Once you fix up the conflicts, this is ready to merge

…luding file globbing)

…t into exclude-files-by-path

ido-namely · 2018-04-29T03:39:13Z

Hey @tonerdo , found some problems, please don't merge yet

MarcoRossignoli · 2018-04-29T07:29:53Z

@tonerdo maybe you already know this app https://github.com/apps/wip useful to permit to PR writer to block merge, used a lot on corefx

ido-namely · 2018-04-29T07:47:50Z

@tonerdo I fixe the problem I faced.
I also added support for absolute paths, updated the readme file and the build.proj file to use the new flag.

I see there is some problem in appveyor, not sure if it's related to the project or not.

tonerdo · 2018-04-29T09:12:05Z

README.md

 Coverlet just uses the type name, so the attributes can be created under any namespace of your choosing.

+#### File Path  
+You can also ignore code coverage by specifing files using the `Exclude` property


Nit: You can also ignore specific source files from code coverage using the Exclude property

ryanmvt · 2018-05-02T17:58:51Z

awesome! this looks great guys. Thank you @ido-namely for taking this on.

…-by-path Implementation exclusion of files according to source file paths

Implementation exclusion of files according to source file paths (inc…

d3b2db1

…luding file globbing)

ido-namely force-pushed the exclude-files-by-path branch from 222eef7 to d3b2db1 Compare April 26, 2018 04:54

ido-namely mentioned this pull request Apr 26, 2018

Is it possible to specify a directory to exclude from coverage? #56

Closed

MarcoRossignoli reviewed Apr 26, 2018

View reviewed changes

abe545 reviewed Apr 26, 2018

View reviewed changes

addressing first comments

8adfb45

ido-namely force-pushed the exclude-files-by-path branch from 3ee4d5d to 8adfb45 Compare April 27, 2018 04:03

tonerdo reviewed Apr 28, 2018

View reviewed changes

addressing further comments

ccdca64

tonerdo approved these changes Apr 29, 2018

View reviewed changes

ido-namely added 4 commits April 28, 2018 21:30

Implementation exclusion of files according to source file paths (inc…

60c4907

…luding file globbing)

addressing first comments

139c238

addressing further comments

3439275

Merge branch 'exclude-files-by-path' of github.com:ido-namely/coverle…

9f120f1

…t into exclude-files-by-path

ido-namely force-pushed the exclude-files-by-path branch from 2671796 to ff0e936 Compare April 29, 2018 01:58

fixing name spaces

4ddf02d

ido-namely force-pushed the exclude-files-by-path branch from ff0e936 to 4ddf02d Compare April 29, 2018 01:59

enhance multiple exclustion rules options and add test

cd538e3

ido-namely added 2 commits April 29, 2018 03:33

adding support for absolute paths, updating readme files

d016d56

invoke ci

529c79e

tonerdo reviewed Apr 29, 2018

View reviewed changes

Add tests; modify readme

23dca2a

tonerdo merged commit b3d108a into coverlet-coverage:master Apr 29, 2018

NorekZ pushed a commit to NorekZ/coverlet that referenced this pull request Nov 8, 2018

Merge pull request coverlet-coverage#64 from ido-namely/exclude-files…

679ecaa

…-by-path Implementation exclusion of files according to source file paths

Implementation exclusion of files according to source file paths #64

Implementation exclusion of files according to source file paths #64

Uh oh!

Conversation

ido-namely commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ido-namely commented Apr 26, 2018

Uh oh!

MarcoRossignoli commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abe545 Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonerdo Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonerdo commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonerdo left a comment

Choose a reason for hiding this comment

Uh oh!

ido-namely commented Apr 29, 2018

Uh oh!

MarcoRossignoli commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ido-namely commented Apr 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanmvt commented May 2, 2018

Uh oh!

Uh oh!

ido-namely commented Apr 26, 2018 •

edited

Loading

MarcoRossignoli Apr 26, 2018 •

edited

Loading

abe545 Apr 26, 2018 •

edited

Loading

tonerdo Apr 26, 2018 •

edited

Loading

MarcoRossignoli commented Apr 29, 2018 •

edited

Loading