-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Added comma to replaced characters in assemblyname #4136
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
Conversation
@TimCurwick, |
|
||
It "Script with a class definition can run from a path without a comma" { | ||
|
||
$FilePath = '.\MyTest.ps1' |
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.
Please use $TestDrive for temporary files.
} | ||
|
||
catch { | ||
$Success = $False |
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.
Instead use the Should Not Throw pattern. Example:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/Out-File.Tests.ps1
Line 4 in edf685d
{ 1 | Out-File -PSPath $tempFile } | Should Not Throw |
$Success | Should Be $True | ||
} | ||
|
||
It "Script with a class definition can run from a path with a comma" { |
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.
Same comments as above test
$Success | Should Be $True | ||
} | ||
|
||
It "Script with a class definition can run from a path with a comma" { |
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.
These two test cases can be combined into 1 using the -TestCases parameter for It. Example:
PowerShell/test/powershell/Language/Parser/LanguageAndParser.TestFollowup.Tests.ps1
Line 147 in 02b5f35
$testCase = @( |
@@ -1113,12 +1113,13 @@ internal static Assembly DefineTypes(Parser parser, Ast rootAst, TypeDefinitionA | |||
var definedTypes = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |||
|
|||
// First character is a special mark that allows us to cheaply ignore dynamic generated assemblies in ClrFacede.GetAssemblies() |
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.
Typo in comment for ClrFacade.
$FilePath = '.\MyTest.ps1' | ||
|
||
try { | ||
'class MyClass { [string]$MyProperty }; $True' | Out-File -FilePath $FilePath |
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.
Use testdrive:
or $testdrive
- then Pester will ensure the file is deleted when the test completes.
Remove-Item -Path $FilePath | ||
} | ||
|
||
$Success | Should Be $True |
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.
Avoid Should Be $True
- this results in a typically unhelpful message in the log.
I suggest a test that actually uses the class, so maybe something like:
Instead, try something like:
`@
class MyClass { static [string]$MyProperty = $PSScriptRoot }
[MyClass]::MyProperty
'@ > $testDrive/My,Test.ps1
& $testDrive/My,Test.ps1 | Should Match ".*My,Test.ps1"
Or something like that.
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.
$PSScriptRoot doesn't work inside a class.
@@ -1,42 +1,21 @@ | |||
Describe "Script with a class definition run path" -Tags "CI" { | |||
Describe "Script with a class definition run path" { |
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.
Missing -Tags 'CI'
} | ||
|
||
$Success | Should Be $True | ||
{ . $FilePath } | Should Not Throw |
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.
This test is not necessary - the next test covers this case.
@adityapatwardhan and @lzybkr could you please take another look? I think the author has addressed all your comments. |
@TimCurwick Thanks for your contribution! 👍 |
A script with a native class will not run if it is saved with a name or full path with a comma.
I added the comma to the list of characters that are replaced with a similar character when creating an assemblyName based on a file path. Added pester test to test.
Discussed with Jason Shirk.