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

Allow [ExcludeFromCodeCoverage()] on the param() block to exclude a function from code coverage #2268

Open
3 tasks done
alexwnovak opened this issue Nov 22, 2022 · 7 comments · May be fixed by #2593
Open
3 tasks done

Comments

@alexwnovak
Copy link

Checklist

Summary of the feature request

I don't know whether this is possible, but I hope so.

I know Pester has configurable options to exclude files, but I would love to exclude a function in the familiar way we'd do this in C#. PowerShell already allows us to use [SuppressMessageAttribute()] on the param block for PSScriptAnalyzer violations.

Bringing this configuration into the code makes it more powerful, since the author can specify the expectations--some functions shouldn't be counted in the overall coverage. Otherwise, Pester must be (separately) invoked correctly, leaving room for discrepancy.

How should it work?

Here's an example:

function Invoke-MyFunction {
    # Perform logic here that should be under test

    $value = Invoke-Dependency  # This dependency would be mocked out, but shouldn't count toward code coverage

    # Perform more logic with $value
}

function Invoke-Dependency {
    # Perform some operation. This may be a simple pass-through that's tedious/non-valuable/difficult to test.
}
@fflaten
Copy link
Collaborator

fflaten commented Nov 23, 2022

Thanks for the suggestion!

Attributes like [ExcludeFromCodeCoverage()] would require either authors to strip it during build-process or for all users to have Pester imported during runtime to be able to resolve the custom attribute. Both is a bad user experience IMO.

PSScriptAnalyzer reuse a diagnostics-attribute included in .NET itself which is always available. Technically we could reuse it or something similar, but attributes are attached to certain symbols like function definitions, properties etc. which is also a limitation with PSSA atm.

An alternative like comment-based regions (#PesterStartExcludeCC and #PesterEndExcludeCC) could be possible, but it adds additional processing to analyze every file. We could also support more granular CC inclusion/exclusion configuration in general and let third-party modules create that using their preferred method.

Waiting on feedback from the CC-wizard @nohwnd 🧙‍♂️

@FilipDeVos
Copy link

The ExcludeFromCodeCoverageAttribute is part of .net standard 2.0 which was shiped with .net framework 4.6.2 (which is now the lowest supported version of the .net framework) https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.excludefromcodecoverageattribute?view=netstandard-2.0

I think it's safe to use for this purpose.

@fflaten
Copy link
Collaborator

fflaten commented Oct 16, 2023

@FilipDeVos thanks for correcting me 🙂 Not sure how I missed that.

Waiting on @nohwnd's view on this as we don't use attributes atm. Normally we'd provide a CodeCoverage.ExcludeFunction option

The user experience should be considered with #2145 and possibly replacement for the removed hashtable-configuration for a consistent API (attribute vs comment vs configuration).

@nohwnd
Copy link
Member

nohwnd commented Oct 31, 2023

I like it, I think it would be pretty easy to implement.

function a  {
    [System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute()]
    param()
 }


(get-command a).ScriptBlock.Attributes
(Get-Command a).ScriptBlock.Ast.Extent.File
(get-command a).ScriptBlock.Ast.Extent.StartLineNumber
(get-command a).ScriptBlock.Ast.Extent.EndColumnNumber

If someone wants to try to implement this into either of the coverages then I can offer some advice. Lot of the code would also be found in pester 4, where similar (even more advanced functionality) exists.

@nohwnd
Copy link
Member

nohwnd commented Oct 31, 2023

The high level view of code coverage is that we take note of every single place that we want to hit. I the breakpoint based coverage we put a breakpoint (🔴) to every place that we want to hit. In profiler based code coverage we make a table of locations. All these locations are our 100% coverage. Like this:

function a () { 
🔴	b
}

function b () {
🔴  "I call c:"
🔴  c
}

function c () {
🔴   "hello"
🔴  "this"
🔴  "is"
🔴  "dog"
}

This is called instrumentation. The code that does the instrumentation (finds all executable nodes, and places breakpoints), is already using AST, so all you need to do is inspect the code for exclusions, and don't instrument it. This will remove the excluded code from the 100% code coverage.

@nohwnd
Copy link
Member

nohwnd commented Nov 6, 2023

Been looking a bit more on how to implement this, this way I can resolve the function from parser, and make exclude rules. The whole old infra for excluding is still in place, but is not publicly exposed, but we should avoid parsing the file twice, because that is expensive, but pieces of that existing code can be re-used I think.

The way it works now, is that we get config from user, we translate it from the new format to the other, ignoring some of the features that used to be there. The old code that has all those capabilities then runs, and figures out a list of interesting places in code (we call them breakpoints, but they are not real breakpoints). We then take this list and translate it to either real breakpoint locations, and set those breakpoints, or to ast locations, and use profiler based code coverage.

So good news is that implementing the exclusions can be done once for both types of coverage.

# in file a.ps1
function a {
    [Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute()]
    param ()

    "hello"
}

# parsing to get the function from the attribute (I don't check the attribute type here yet
$pre = { $args[0] -is [System.Management.Automation.Language.CommandBaseAst] }
$errs = @()
$p = [System.Management.Automation.Language.Parser]::ParseFile("S:\t\pwsh1\a.ps1", [ref] $null, [ref] $errs)
$errs

$pre = {$args[0] -is [System.Management.Automation.Language.AttributeAst] -and $args[0].Parent -and $args[0].Parent -is [System.Management.Automation.Language.ParamBlockAst] -and $args[0].Parent.Parent -is [System.Management.Automation.Language.ScriptBlockAst] -and $args[0].Parent.Parent.Parent -is [System.Management.Automation.Language.FunctionDefinitionAst]  }
$p.FindAll($pre, $true).Parent.Parent.Parent.Parent.Statements.Name

If you as why not simply expose the additional features? I think that this attribute based approach is much better, we can already exclude whole files, and I don't think anyone wants to fiddle with specifying the exact location of their function by lines, when that can change with any code change.

@benjaminfuchs
Copy link

benjaminfuchs commented Dec 21, 2024

I've opened a PR (#2593) to add support for excluding functions from code coverage using the [ExcludeFromCodeCoverageAttribute()] attribute. While this seems to address the issue, I'm not entirely sure if it's the most valid or optimal fix. Any feedback, suggestions, or pointers to a better solution would be greatly appreciated!

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.

5 participants