Merge pull request #238 from PowerShell/VerboseMessageForDSCResourceBranch

Rule to validate the presence of Verbose statements in DSC Resource

Author: raghushantha

Rules/ScriptAnalyzerBuiltinRules.csproj

@@ -75,7 +75,7 @@
     <Compile Include="MissingModuleManifestField.cs" />
     <Compile Include="PossibleIncorrectComparisonWithNull.cs" />
     <Compile Include="ProvideCommentHelp.cs" />
-    <Compile Include="ProvideVerboseMessage.cs" />
+    <Compile Include="UseVerboseMessageInDSCResource.cs" />
     <Compile Include="Strings.Designer.cs">
       <AutoGen>True</AutoGen>
       <DesignTime>True</DesignTime>

Rules/Strings.Designer.cs

@@ -276,17 +276,14 @@
   <data name="SourceName" xml:space="preserve">
     <value>PS</value>
   </data>
-  <data name="ProvideVerboseMessageDescription" xml:space="preserve">
-    <value>Checks that Write-Verbose is called at least once in every cmdlet or script. This is in line with the PowerShell best practices.</value>
+  <data name="UseVerboseMessageInDSCResourceDescription" xml:space="preserve">
+    <value>It is a best practice to emit informative, verbose messages in DSC resource functions. This helps in debugging issues when a DSC configuration is executed.</value>
   </data>
-  <data name="ProvideVerboseMessageErrorFunction" xml:space="preserve">
-    <value>There is no call to Write-Verbose in the function ‘{0}’.</value>
+  <data name="UseVerboseMessageInDSCResourceErrorFunction" xml:space="preserve">
+    <value>There is no call to Write-Verbose in DSC function ‘{0}’. If you are using Write-Verbose in a helper function, suppress this rule application.</value>
   </data>
-  <data name="ProvideVerboseMessageCommonName" xml:space="preserve">
-    <value>Verbose</value>
-  </data>
-  <data name="ProvideVerboseMessageScript" xml:space="preserve">
-    <value>There is no call to Write-Verbose in the script.</value>
+  <data name="UseVerboseMessageInDSCResourceCommonName" xml:space="preserve">
+    <value>Use verbose message in DSC resource</value>
   </data>
   <data name="MissingModuleManifestFieldDescription" xml:space="preserve">
     <value>Some fields of the module manifest (such as ModuleVersion) are required.</value>
@@ -459,8 +456,8 @@
   <data name="MissingModuleManifestFieldName" xml:space="preserve">
     <value>MissingModuleManifestField</value>
   </data>
-  <data name="ProvideVerboseMessageName" xml:space="preserve">
-    <value>ProvideVerboseMessage</value>
+  <data name="UseVerboseMessageInDSCResourceName" xml:space="preserve">
+    <value>UseVerboseMessageInDSCResource</value>
   </data>
   <data name="CommandNotFoundCommonName" xml:space="preserve">
     <value>Command Not Found</value>

Rules/Strings.resx

@@ -35,8 +35,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
-            IEnumerable<Ast> assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true);
-            IEnumerable<Ast> assingmentVarAsts;
+            IEnumerable<Ast> assignmentAsts = ast.FindAll(testAst => testAst is AssignmentStatementAst, true);            
             IEnumerable<Ast> varAsts = ast.FindAll(testAst => testAst is VariableExpressionAst, true);
             IEnumerable<Ast> varsInAssignment;
 

Rules/UseDeclaredVarsMoreThanAssignments.cs

@@ -22,78 +22,64 @@
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
     /// <summary>
-    /// ProvideVerboseMessage: Analyzes the ast to check that Write-Verbose is called at least once in every cmdlet or script.
+    /// ProvideVerboseMessage: Analyzes the ast to check that Write-Verbose is called for DSC Resources
     /// </summary>
-    [Export(typeof(IScriptRule))]
-    public class ProvideVerboseMessage : SkipNamedBlock, IScriptRule
+    [Export(typeof(IDSCResourceRule))]
+    public class UseVerboseMessageInDSCResource : SkipNamedBlock, IDSCResourceRule
     {
         /// <summary>
-        /// AnalyzeScript: Analyzes the ast to check that Write-Verbose is called at least once in every cmdlet or script.
+        /// AnalyzeDSCResource: Analyzes the ast to check that Write-Verbose is called for DSC Resources
         /// <param name="ast">The script's ast</param>
         /// <param name="fileName">The script's file name</param>
         /// </summary>
-        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        public IEnumerable<DiagnosticRecord> AnalyzeDSCResource(Ast ast, string fileName)
         {
-            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
-            
-            ClearList();
-            this.AddNames(new List<string>() { "Configuration", "Workflow" });
-            DiagnosticRecords.Clear();
-            
-            this.fileName = fileName;
-            //We only check that advanced functions should have Write-Verbose
-            ast.Visit(this);
-
-            return DiagnosticRecords;
-        }
-
-        /// <summary>
-        /// Visit function and checks that it has write verbose
-        /// </summary>
-        /// <param name="funcAst"></param>
-        /// <returns></returns>
-        public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst funcAst)
-        {
-            if (funcAst == null)
+            if (ast == null)
             {
-                return AstVisitAction.SkipChildren;
-            }
+                throw new ArgumentNullException(Strings.NullAstErrorMessage);
+            }            
+
+            IEnumerable<Ast> functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);
 
-            //Write-Verbose is not required for non-advanced functions
-            if (funcAst.Body == null || funcAst.Body.ParamBlock == null
-           || funcAst.Body.ParamBlock.Attributes == null ||
-           funcAst.Body.ParamBlock.Parameters == null ||
-                !funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
+            foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts)
             {
-                return AstVisitAction.Continue;
-            }
+                var commandAsts = functionDefinitionAst.Body.FindAll(testAst => testAst is CommandAst, false);
+                bool hasVerbose = false;
 
-            var commandAsts = funcAst.Body.FindAll(testAst => testAst is CommandAst, false);
-            bool hasVerbose = false;
+                if (null != commandAsts)
+                {
+                    foreach (CommandAst commandAst in commandAsts)
+                    {
+                        hasVerbose |= String.Equals(commandAst.GetCommandName(), "Write-Verbose", StringComparison.OrdinalIgnoreCase);
+                    }
+                }
 
-            if (commandAsts != null)
-            {
-                foreach (CommandAst commandAst in commandAsts)
+                if (!hasVerbose)
                 {
-                    hasVerbose |= String.Equals(commandAst.GetCommandName(), "Write-Verbose", StringComparison.OrdinalIgnoreCase);
+                    yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceErrorFunction, functionDefinitionAst.Name),
+                        functionDefinitionAst.Extent, GetName(), DiagnosticSeverity.Information, fileName);
                 }
-            }
 
-            if (!hasVerbose)
-            {
-                DiagnosticRecords.Add(new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageErrorFunction, funcAst.Name),
-                    funcAst.Extent, GetName(), DiagnosticSeverity.Information, fileName));
             }
-
-            return AstVisitAction.Continue;
+        }
+        
+        /// <summary>
+        /// AnalyzeDSCClass: This function returns nothing in the case of dsc class.
+        /// </summary>
+        /// <param name="ast"></param>
+        /// <param name="fileName"></param>
+        /// <returns></returns>
+        public IEnumerable<DiagnosticRecord> AnalyzeDSCClass(Ast ast, string fileName)
+        {
+            return Enumerable.Empty<DiagnosticRecord>();
         }
 
         /// <summary>
         /// Method: Retrieves the name of this rule.
         /// </summary>
         public string GetName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideVerboseMessageName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseVerboseMessageInDSCResourceName);
         }
 
         /// <summary>
@@ -102,7 +88,7 @@ public string GetName()
         /// <returns>The common name of this rule</returns>
         public string GetCommonName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageCommonName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceCommonName);
         }
 
         /// <summary>
@@ -111,7 +97,7 @@ public string GetCommonName()
         /// <returns>The description of this rule</returns>
         public string GetDescription()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.ProvideVerboseMessageDescription);
+            return string.Format(CultureInfo.CurrentCulture, Strings.UseVerboseMessageInDSCResourceDescription);
         }
 
         /// <summary>
@@ -136,7 +122,7 @@ public RuleSeverity GetSeverity()
         /// </summary>
         public string GetSourceName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.DSCSourceName);
         }
     }
-}
+}

Rules/ProvideVerboseMessage.cs renamed to Rules/UseVerboseMessageInDSCResource.cs

@@ -130,11 +130,11 @@ Describe "TestSeverity" {
 Describe "TestWildCard" {
     It "filters rules based on the -Name wild card input" {
         $rules = Get-ScriptAnalyzerRule -Name PSDSC*
-        $rules.Count | Should be 6
+        $rules.Count | Should be 7
     }
 
     It "filters rules based on wild card input and severity"{
         $rules = Get-ScriptAnalyzerRule -Name PSDSC*　-Severity Information
-        $rules.Count | Should be 3
+        $rules.Count | Should be 4
     }
 }

Tests/Rules/ProvideVerboseMessage.tests.ps1 renamed to Tests/Disabled Rules/ProvideVerboseMessage.tests.ps1

@@ -5,7 +5,7 @@ Param(
 
 function SuppressMe ()
 {
-    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideVerboseMessage")]
+    [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideCommentHelp")]
     [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "unused1")]
     Param([string]$unUsed1, [int] $unUsed2)
     {

Tests/Engine/GetScriptAnalyzerRule.tests.ps1

@@ -5,7 +5,7 @@ $violations = Invoke-ScriptAnalyzer $directory\RuleSuppression.ps1
 Describe "RuleSuppressionWithoutScope" {
     Context "Function" {
         It "Does not raise violations" {
-            $suppression = $violations | Where-Object { $_.RuleName -eq "PSProvideVerboseMessage" }
+            $suppression = $violations | Where-Object { $_.RuleName -eq "PSProvideCommentHelp" }
             $suppression.Count | Should Be 0
         }
     }

Tests/Engine/RuleSuppression.ps1

@@ -29,6 +29,8 @@ function Get-TargetResource
         [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value
     )
 
+    Write-Verbose "In Get-TargetResource"
+
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
 
     $b = @{"hash" = "table"}
@@ -75,10 +77,14 @@ function Set-TargetResource
         [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value
     )
 
+    Write-Verbose "In Set-TargetResource"
+
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
 
     if ($PSBoundParameters["Verbose"])
     {
+        Write-Verbose "Calling xMachine with Verbose parameter"
+
         PSDSCxMachine\Set-_InternalPSDscXMachineTR `
                -RemoteResourceId $ResourceName `
                -RemoteMachine $NodeName `
@@ -130,6 +136,8 @@ function Test-TargetResource
         [Uint32] $ThrottleLimit = 32 #Powershell New-CimSession default throttle value
     )
 
+    Write-Verbose "In Test-TargetResource"
+
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
 
     $a = $true

Tests/Engine/RuleSuppression.tests.ps1

@@ -0,0 +1,26 @@
+Import-Module PSScriptAnalyzer
+
+$violationMessage = "There is no call to Write-Verbose in DSC function ‘Set-TargetResource’. If you are using Write-Verbose in a helper function, suppress this rule application."
+$violationName = "PSDSCUseVerboseMessageInDSCResource"
+$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
+$violations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAll\MSFT_WaitForAll.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$noViolations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAny\MSFT_WaitForAny.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$noClassViolations = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\DSCResources\MyDscResource\MyDscResource.psm1 | Where-Object {$_.RuleName -eq $violationName}
+
+Describe "UseVerboseMessageInDSCResource" {
+    Context "When there are violations" {
+        It "has 2 Verbose Message violations" {
+            $violations.Count | Should Be 2
+        }
+
+        It "has the correct description message" {
+            $violations[0].Message | Should Match $violationMessage
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations" {
+            $noViolations.Count | Should Be 0
+        }        
+    }
+}