From 2c9519ba3be129df15094e19a720a4e289f63488 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 10 Jul 2018 02:14:59 -0700 Subject: [PATCH] [Feature] Address Feedback: Use better error message and add more tests --- .../PowerShellCore_format_ps1xml.cs | 2 +- .../engine/Attributes.cs | 11 +- .../ExperimentalFeature.cs | 60 ++++++--- .../engine/Modules/ModuleCmdletBase.cs | 2 +- .../resources/Metadata.resx | 6 + .../ExperimentalFeature.Basic.Tests.ps1 | 125 +++++++++++++----- .../assets/ExpTest/ExpTest.psm1 | 2 +- 7 files changed, 142 insertions(+), 66 deletions(-) diff --git a/src/System.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs b/src/System.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs index d33c54dd64..7d7906340c 100644 --- a/src/System.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs +++ b/src/System.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs @@ -1249,7 +1249,7 @@ namespace System.Management.Automation.Runspaces yield return new FormatViewDefinition("ExperimentalFeature", TableControl.Create() .AddHeader(Alignment.Left, width: 35) - .AddHeader(Alignment.Right, width: 10) + .AddHeader(Alignment.Right, width: 7) .AddHeader(Alignment.Left, width: 35) .AddHeader(Alignment.Left) .StartRowDefinition() diff --git a/src/System.Management.Automation/engine/Attributes.cs b/src/System.Management.Automation/engine/Attributes.cs index e22e597585..75035dd4b9 100644 --- a/src/System.Management.Automation/engine/Attributes.cs +++ b/src/System.Management.Automation/engine/Attributes.cs @@ -625,16 +625,7 @@ namespace System.Management.Automation /// public ParameterAttribute(string experimentName, ExperimentAction experimentAction) { - if (string.IsNullOrEmpty(experimentName)) - { - throw PSTraceSource.NewArgumentException(nameof(experimentName)); - } - - if (experimentAction == ExperimentAction.None) - { - throw PSTraceSource.NewArgumentException(nameof(experimentAction)); - } - + ExperimentalAttribute.ValidateArguments(experimentName, experimentAction); ExperimentName = experimentName; ExperimentAction = experimentAction; } diff --git a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs index 9434d73375..8af881b3e0 100644 --- a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs +++ b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs @@ -2,8 +2,8 @@ // Licensed under the MIT License. using System.Collections.Generic; -using System.Collections.ObjectModel; using System.Collections.Immutable; +using System.Collections.ObjectModel; using System.Linq; using System.Management.Automation.Configuration; using System.Management.Automation.Internal; @@ -180,16 +180,27 @@ namespace System.Management.Automation /// When specified, we check if the feature name matches the module name internal static bool IsModuleFeatureName(string featureName, string moduleName = null) { - int firstDotIndex = featureName.IndexOf('.'); - int lastDotIndex = featureName.LastIndexOf('.'); - - bool legit = firstDotIndex > 0 && lastDotIndex < featureName.Length - 1; - if (legit && moduleName != null) + // Feature names cannot start with a dot + if (featureName.StartsWith('.')) { - var moduleNamePart = featureName.AsSpan(0, lastDotIndex); - return moduleNamePart.Equals(moduleName.AsSpan(), StringComparison.OrdinalIgnoreCase); + return false; } - return legit; + + // Feature names must contain a dot, but not at the end + int lastDotIndex = featureName.LastIndexOf('.'); + if (lastDotIndex == -1 || lastDotIndex == featureName.Length - 1) + { + return false; + } + + if (moduleName == null) + { + return true; + } + + // If the module name is given, it must match the prefix of the feature name (up to the last dot). + var moduleNamePart = featureName.AsSpan(0, lastDotIndex); + return moduleNamePart.Equals(moduleName.AsSpan(), StringComparison.OrdinalIgnoreCase); } /// @@ -266,16 +277,7 @@ namespace System.Management.Automation /// public ExperimentalAttribute(string experimentName, ExperimentAction experimentAction) { - if (string.IsNullOrEmpty(experimentName)) - { - throw PSTraceSource.NewArgumentException(nameof(experimentName)); - } - - if (experimentAction == ExperimentAction.None) - { - throw PSTraceSource.NewArgumentException(nameof(experimentAction)); - } - + ValidateArguments(experimentName, experimentAction); ExperimentName = experimentName; ExperimentAction = experimentAction; } @@ -290,6 +292,26 @@ namespace System.Management.Automation /// internal static readonly ExperimentalAttribute None = new ExperimentalAttribute(); + /// + /// Validate arguments for the constructor. + /// + internal static void ValidateArguments(string experimentName, ExperimentAction experimentAction) + { + if (string.IsNullOrEmpty(experimentName)) + { + string paramName = nameof(experimentName); + throw PSTraceSource.NewArgumentNullException(paramName, Metadata.ArgumentNullOrEmpty, paramName); + } + + if (experimentAction == ExperimentAction.None) + { + string paramName = nameof(experimentAction); + string invalidMember = ExperimentAction.None.ToString(); + string validMembers = StringUtil.Format("{0}, {1}", ExperimentAction.Hide, ExperimentAction.Show); + throw PSTraceSource.NewArgumentException(paramName, Metadata.InvalidEnumArgument, invalidMember, paramName, validMembers); + } + } + internal bool ToHide => EffectiveAction == ExperimentAction.Hide; internal bool ToShow => EffectiveAction == ExperimentAction.Show; diff --git a/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs b/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs index ca66ffdc0a..5539285cdf 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs @@ -2083,7 +2083,7 @@ namespace Microsoft.PowerShell.Commands { if (writingErrors) { - string invalidNameStr = String.Join(',', invalidNames); + string invalidNameStr = String.Join(", ", invalidNames); string errorMsg = StringUtil.Format(Modules.InvalidExperimentalFeatureName, invalidNameStr); WriteError(new ErrorRecord(new ArgumentException(errorMsg), "Modules_InvalidExperimentalFeatureName", diff --git a/src/System.Management.Automation/resources/Metadata.resx b/src/System.Management.Automation/resources/Metadata.resx index f4ce375cfe..f8a0d75378 100644 --- a/src/System.Management.Automation/resources/Metadata.resx +++ b/src/System.Management.Automation/resources/Metadata.resx @@ -243,4 +243,10 @@ The path argument has no root drive. Supply a full path argument with a root drive. + + The argument value for the parameter '{0}' cannot be null or an empty string. + + + The Enum member '{0}' is not a valid value for the parameter '{1}'. Specify one of the following members and try again: {2}. + diff --git a/test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1 b/test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1 index 21baa4646e..cb8869b265 100644 --- a/test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1 +++ b/test/powershell/engine/ExperimentalFeature/ExperimentalFeature.Basic.Tests.ps1 @@ -11,6 +11,8 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { $originalDefaultParameterValues = $PSDefaultParameterValues.Clone() $PSDefaultParameterValues["it:skip"] = $true } else { + ## Common parameters are defined in the type 'CommonParameters' as public properties. + $CommonParameterCount = [System.Management.Automation.Internal.CommonParameters].GetProperties().Length $TestModule = Join-Path $PSScriptRoot "assets" "ExpTest" $AssemblyPath = Join-Path $TestModule "ExpTest.dll" if (-not (Test-Path $AssemblyPath)) { @@ -62,8 +64,8 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-Name' - $command.Parameters.Count | Should -Be 12 + ## Common parameters + '-Name' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 1) & $Name -Name Joe | Should -BeExactly "Hello World Joe." } @@ -75,8 +77,8 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-VMName', '-Port', '-ThrottleLimit' and '-Command' - $command.Parameters.Count | Should -Be 18 + ## Common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-VMName', '-Port', '-ThrottleLimit' and '-Command' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 7) $command.ParameterSets.Count | Should -Be 2 $command.Parameters["UserName"].ParameterSets.Count | Should -Be 1 @@ -100,13 +102,13 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { $command.Parameters["Command"].ParameterSets.Count | Should -Be 1 $command.Parameters["Command"].ParameterSets.ContainsKey("__AllParameterSets") | Should -Be $true - ## 11 common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-ThrottleLimit' and '-Command' + ## Common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-ThrottleLimit' and '-Command' $command.ParameterSets[0].Name | Should -BeExactly "ComputerSet" - $command.ParameterSets[0].Parameters.Count | Should -Be 16 + $command.ParameterSets[0].Parameters.Count | Should -Be ($CommonParameterCount + 5) - ## 11 common parameters + '-VMName', '-Port', '-ThrottleLimit' and '-Command' + ## Common parameters + '-VMName', '-Port', '-ThrottleLimit' and '-Command' $command.ParameterSets[1].Name | Should -BeExactly "VMSet" - $command.ParameterSets[1].Parameters.Count | Should -Be 15 + $command.ParameterSets[1].Parameters.Count | Should -Be ($CommonParameterCount + 4) & $Name -UserName "user" -ComputerName "localhost" -ConfigurationName "config" | Should -BeExactly "Invoke-MyCommand with ComputerSet" & $Name -VMName "VM" -Port "80" | Should -BeExactly "Invoke-MyCommand with VMSet" @@ -119,8 +121,8 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-SessionName' - $command.Parameters.Count | Should -Be 12 + ## Common parameters + '-SessionName' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 1) $command.Parameters["SessionName"].ParameterType.FullName | Should -BeExactly "System.String" $command.Parameters.ContainsKey("ComputerName") | Should -Be $false } @@ -132,8 +134,8 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-ByUrl', '-ByRadio', '-FileName', '-Configuration' - $command.Parameters.Count | Should -Be 15 + ## Common parameters + '-ByUrl', '-ByRadio', '-FileName', '-Configuration' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 4) $command.ParameterSets.Count | Should -Be 2 $command.Parameters["ByUrl"].ParameterSets.Count | Should -Be 1 @@ -161,13 +163,13 @@ Describe "Experimental Feature Basic Tests - Feature-Disabled" -tags "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-Name' (dynamic parameters are not triggered) - $command.Parameters.Count | Should -Be 12 + ## Common parameters + '-Name' (dynamic parameters are not triggered) + $command.Parameters.Count | Should -Be ($CommonParameterCount + 1) $command.Parameters["Name"] | Should -Not -BeNullOrEmpty $command = Get-Command $Name -ArgumentList "Joe" - ## 11 common parameters + '-Name' and '-ConfigName' (dynamic parameters are triggered) - $command.Parameters.Count | Should -Be 13 + ## Common parameters + '-Name' and '-ConfigName' (dynamic parameters are triggered) + $command.Parameters.Count | Should -Be ($CommonParameterCount + 2) $command.Parameters["ConfigName"].Attributes.Count | Should -Be 2 $command.Parameters["ConfigName"].Attributes[0] | Should -BeOfType [parameter] $command.Parameters["ConfigName"].Attributes[1] | Should -BeOfType [ValidateNotNullOrEmpty] @@ -186,10 +188,13 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { $originalDefaultParameterValues = $PSDefaultParameterValues.Clone() $PSDefaultParameterValues["it:skip"] = $true } else { + ## Common parameters are defined in the type 'CommonParameters' as public properties. + $CommonParameterCount = [System.Management.Automation.Internal.CommonParameters].GetProperties().Length $TestModule = Join-Path $PSScriptRoot "assets" "ExpTest" $AssemblyPath = Join-Path $TestModule "ExpTest.dll" if (-not (Test-Path $AssemblyPath)) { $SourcePath = Join-Path $TestModule "ExpTest.cs" + $SourcePath = (Copy-Item $SourcePath TestDrive:\ -PassThru).FullName Add-Type -Path $SourcePath -OutputType Library -OutputAssembly $AssemblyPath } $moduleInfo = Import-Module $TestModule -PassThru @@ -234,8 +239,8 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-Name' + '-SwitchOne' + '-SwitchTwo' - $command.Parameters.Count | Should -Be 14 + ## Common parameters + '-Name' + '-SwitchOne' + '-SwitchTwo' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 3) $command.ParameterSets.Count | Should -Be 3 & $Name -Name Joe | Should -BeExactly "Hello World Joe." @@ -251,9 +256,9 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-VMName', '-Port', + ## Common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-VMName', '-Port', ## '-Token', '-WebSocketUrl', '-ThrottleLimit' and '-Command' - $command.Parameters.Count | Should -Be 20 + $command.Parameters.Count | Should -Be ($CommonParameterCount + 9) $command.ParameterSets.Count | Should -Be 3 $command.Parameters["UserName"].ParameterSets.Count | Should -Be 1 @@ -285,17 +290,17 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { $command.Parameters["Command"].ParameterSets.Count | Should -Be 1 $command.Parameters["Command"].ParameterSets.ContainsKey("__AllParameterSets") | Should -Be $true - ## 11 common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-ThrottleLimit' and '-Command' + ## Common parameters + '-UserName', '-ComputerName', '-ConfigurationName', '-ThrottleLimit' and '-Command' $command.ParameterSets[0].Name | Should -BeExactly "ComputerSet" - $command.ParameterSets[0].Parameters.Count | Should -Be 16 + $command.ParameterSets[0].Parameters.Count | Should -Be ($CommonParameterCount + 5) - ## 11 common parameters + '-VMName', '-Port', '-ThrottleLimit' and '-Command' + ## Common parameters + '-VMName', '-Port', '-ThrottleLimit' and '-Command' $command.ParameterSets[1].Name | Should -BeExactly "VMSet" - $command.ParameterSets[1].Parameters.Count | Should -Be 15 + $command.ParameterSets[1].Parameters.Count | Should -Be ($CommonParameterCount + 4) - ## 11 common parameters + '-Token', '-WebSocketUrl', '-ConfigurationName', '-Port', '-ThrottleLimit', '-Command' + ## Common parameters + '-Token', '-WebSocketUrl', '-ConfigurationName', '-Port', '-ThrottleLimit', '-Command' $command.ParameterSets[2].Name | Should -BeExactly "WebSocketSet" - $command.ParameterSets[2].Parameters.Count | Should -Be 17 + $command.ParameterSets[2].Parameters.Count | Should -Be ($CommonParameterCount + 6) & $Name -UserName "user" -ComputerName "localhost" | Should -BeExactly "Invoke-MyCommand with ComputerSet" & $Name -UserName "user" -ComputerName "localhost" -ConfigurationName "config" | Should -BeExactly "Invoke-MyCommand with ComputerSet" @@ -314,8 +319,8 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-ComputerName' - $command.Parameters.Count | Should -Be 12 + ## Common parameters + '-ComputerName' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 1) $command.Parameters["ComputerName"].ParameterType.FullName | Should -BeExactly "System.String" $command.Parameters.ContainsKey("SessionName") | Should -Be $false } @@ -327,8 +332,8 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { param($Name, $CommandType) $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-ByUrl', '-ByRadio', '-FileName', '-Destination' - $command.Parameters.Count | Should -Be 15 + ## Common parameters + '-ByUrl', '-ByRadio', '-FileName', '-Destination' + $command.Parameters.Count | Should -Be ($CommonParameterCount + 4) $command.ParameterSets.Count | Should -Be 2 $command.Parameters["ByUrl"].ParameterSets.Count | Should -Be 1 @@ -356,13 +361,13 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { $command = Get-Command $Name $command.CommandType | Should -Be $CommandType - ## 11 common parameters + '-Name' (dynamic parameters are not triggered) - $command.Parameters.Count | Should -Be 12 + ## Common parameters + '-Name' (dynamic parameters are not triggered) + $command.Parameters.Count | Should -Be ($CommonParameterCount + 1) $command.Parameters["Name"] | Should -Not -BeNullOrEmpty $command = Get-Command $Name -ArgumentList "Joe" - ## 11 common parameters + '-Name' and '-ConfigFile' (dynamic parameters are triggered) - $command.Parameters.Count | Should -Be 13 + ## Common parameters + '-Name' and '-ConfigFile' (dynamic parameters are triggered) + $command.Parameters.Count | Should -Be ($CommonParameterCount + 2) $command.Parameters["ConfigFile"].Attributes.Count | Should -Be 2 $command.Parameters["ConfigFile"].Attributes[0] | Should -BeOfType [parameter] $command.Parameters["ConfigFile"].Attributes[1] | Should -BeOfType [ValidateNotNullOrEmpty] @@ -370,3 +375,55 @@ Describe "Experimental Feature Basic Tests - Feature-Enabled" -Tag "CI" { $command.Parameters.ContainsKey("ConfigName") | Should -Be $false } } + +Describe "Expected errors" -Tag "CI" { + It "'[Experimental()]' should fail to construct the attribute" { + { [Experimental()]param() } | Should -Throw -ErrorId "MethodCountCouldNotFindBest" + } + + It "Argument validation for constructors of 'ExperimentalAttribute' and 'ParameterAttribute'" { + { [Experimental]::new("", "None") } | Should -Throw -ErrorId "PSArgumentNullException" + { [Experimental]::new([NullString]::Value, "None") } | Should -Throw -ErrorId "PSArgumentNullException" + { [Experimental]::new("feature", "None") } | Should -Throw -ErrorId "PSArgumentException" + { [Experimental]::new("feature", "Show") } | Should -Not -Throw + { [Experimental]::new("feature", "Hide") } | Should -Not -Throw + + { [Parameter]::new("", "None") } | Should -Throw -ErrorId "PSArgumentNullException" + { [Parameter]::new([NullString]::Value, "None") } | Should -Throw -ErrorId "PSArgumentNullException" + { [Parameter]::new("feature", "None") } | Should -Throw -ErrorId "PSArgumentException" + { [Parameter]::new("feature", "Show") } | Should -Not -Throw + { [Parameter]::new("feature", "Hide") } | Should -Not -Throw + } + + It "Feature name check" { + $psd1Content = @' +@{ +ModuleVersion = '0.0.1' +CompatiblePSEditions = @('Core') +GUID = 'ce31259c-1804-4016-bc29-083bd2599e19' +PrivateData = @{ + PSData = @{ + ExperimentalFeatures = @( + @{ Name = '.Feature1'; Description = "Test feature number 1." } + @{ Name = 'Feature2.'; Description = "Test feature number 2." } + @{ Name = 'Feature3'; Description = "Test feature number 3." } + @{ Name = 'Module.Feature4'; Description = "Test feature number 4." } + @{ Name = 'InvalidFeatureName.Feature5'; Description = "Test feature number 5." } + ) + } +} +} +'@ + $moduleFile = Join-Path $TestDrive InvalidFeatureName.psd1 + Set-Content -Path $moduleFile -Value $psd1Content -Encoding Ascii + + Import-Module $moduleFile -ErrorVariable featureNameError -ErrorAction SilentlyContinue + $featureNameError | Should -Not -BeNullOrEmpty + $featureNameError[0].FullyQualifiedErrorId | Should -Be "Modules_InvalidExperimentalFeatureName,Microsoft.PowerShell.Commands.ImportModuleCommand" + $featureNameError[0].Exception.Message.Contains(".Feature1") | Should -Be $true + $featureNameError[0].Exception.Message.Contains("Feature2.") | Should -Be $true + $featureNameError[0].Exception.Message.Contains("Feature3") | Should -Be $true + $featureNameError[0].Exception.Message.Contains("Module.Feature4") | Should -Be $true + $featureNameError[0].Exception.Message.Contains("InvalidFeatureName.Feature5") | Should -Be $false + } +} diff --git a/test/powershell/engine/ExperimentalFeature/assets/ExpTest/ExpTest.psm1 b/test/powershell/engine/ExperimentalFeature/assets/ExpTest/ExpTest.psm1 index 3f2c1a08f2..27c9864d82 100644 --- a/test/powershell/engine/ExperimentalFeature/assets/ExpTest/ExpTest.psm1 +++ b/test/powershell/engine/ExperimentalFeature/assets/ExpTest/ExpTest.psm1 @@ -54,7 +54,7 @@ function Get-GreetingMessage $message = "Hello World $Name." - if ([ExperimentalFeature]::HasEnabled("ExpTest.FeatureOne")) + if ([ExperimentalFeature]::IsEnabled("ExpTest.FeatureOne")) { if ($SwitchOne) { $message += "-SwitchOne is on." } if ($SwitchTwo) { $message += "-SwitchTwo is on." }