-
-
Notifications
You must be signed in to change notification settings - Fork 476
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 support for excluding functions from coverage with attribute #2593
base: main
Are you sure you want to change the base?
Add support for excluding functions from coverage with attribute #2593
Conversation
If the approach is valid, I'll refine the tests and add proper documentation. Feedback is welcome! |
99eb66a
to
1c9dd96
Compare
…ibute()] attribute
1c9dd96
to
d1c84df
Compare
53c13ac
to
8608926
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this! It works as intended, though I've suggested some improvements.
While reviewing I realized we could make this more efficient, see comment about FindAll
. So I'd like to discuss an alternative approach. See draft PR #2598 which should produce the same results.
src/functions/Coverage.ps1
Outdated
@@ -297,17 +297,101 @@ function Get-CommandsInFile { | |||
$args[0] -is [System.Management.Automation.Language.BreakStatementAst] -or | |||
$args[0] -is [System.Management.Automation.Language.ContinueStatementAst] -or | |||
$args[0] -is [System.Management.Automation.Language.ExitStatementAst] -or | |||
$args[0] -is [System.Management.Automation.Language.ThrowStatementAst] | |||
$args[0] -is [System.Management.Automation.Language.ThrowStatementAst] -and | |||
-not (IsExcludedByAttribute -Ast $args[0] -TargetAttribute 'System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FindAll
will trigger this for commands in nested scriptblocks even though a parent is excluded. Not ideal to reverse (check parents) on every step.
$ast = {
1 | ForEach-Object {
[System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute()]
param($a, $b)
$a + $b
2 | ForEach-Object {
3 | ForEach-Object {
continue
}
}
}
}.Ast
$ast.FindAll($predicate, $searchNestedScriptBlocks)
# Processing .. output added in IsExcludedByAttribute
Processing 1
Processing ForEach-Object {
[System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute()]
param($a, $b)
$a + $b
2 | ForEach-Object {
3 | ForEach-Object {
continue
}
}
}
Processing $a + $b
Processing 2
Processing ForEach-Object {
3 | ForEach-Object {
continue
}
}
Processing 3
Processing ForEach-Object {
continue
}
Processing continue
Expression Redirections Extent
---------- ------------ ------
1 {} 1
{} ForEach-Object {…
We could avoid this by rewriting to use an AstVisitor
, stopping early at a ScriptBlockAst where the attribute is set.
@fflaten Thanks for the feedback! I agree with your point about improving efficiency by replacing |
Looks like you've simplified yours as well. Feel free the use and complete the Visitor-based code. Was waiting on @nohwnd's thoughts but personally I'd prefer it as it's more extensible. |
c1acbcd
to
e800b90
Compare
9865db2
to
efd3c4c
Compare
efd3c4c
to
82ef868
Compare
Will try to have a look today in the evening. |
$visitor.CoverageLocations.Count | Should -Be 7 -Because "Break, Continue, Throw, and Exit statements should be collected." | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a more high-level api test here, similar to the "Single class using '" tests above. Where we enter and leave the coverage and get the results.
|
||
namespace Pester | ||
{ | ||
public class CoverageLocationVisitor : AstVisitor2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some short info about what this class does if possible. The original predicate is somehow (at least for me) easier to grasp what the intent is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And possibly also why we prefer it over the original predicate.
Great job here,thank you! Some small nitpicks on the code :) |
Thanks. @benjaminfuchs Let me know if you'd like me to address any of the comments. |
I can also address them.
…-j
________________________________
Od: Frode Flaten ***@***.***>
Odesláno: Tuesday, January 14, 2025 8:54:18 PM
Komu: pester/Pester ***@***.***>
Kopie: Jakub Jareš ***@***.***>; Mention ***@***.***>
Předmět: Re: [pester/Pester] Add support for excluding functions from coverage with attribute (PR #2593)
Thanks. @benjaminfuchs<https://github.com/benjaminfuchs> Let me know if you'd like me to address any of the comments.
—
Reply to this email directly, view it on GitHub<#2593 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABLYLYK76PWF4DVMDWV7PSL2KVTOVAVCNFSM6AAAAABUAW2RJ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJQHE4DEMZRG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
PR Summary
Fixes #2268
This PR adds support for excluding specific code constructs from coverage analysis using
[ExcludeFromCodeCoverageAttribute()]
.Changes:
Verifies inclusion of unmarked nodes and relevant control flow statements.
PR Checklist
Create Pull Request
to mark it as a draft. PR can be markedReady for review
when it's ready.