Skip to content

Conversation

@TimCurwick
Copy link
Contributor

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.

@msftclas
Copy link

@TimCurwick,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot


It "Script with a class definition can run from a path without a comma" {

$FilePath = '.\MyTest.ps1'
Copy link
Member

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
Copy link
Member

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:

{ 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" {
Copy link
Member

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" {
Copy link
Member

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:


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()
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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" {
Copy link
Member

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
Copy link
Contributor

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.

@daxian-dbw
Copy link
Member

@adityapatwardhan and @lzybkr could you please take another look? I think the author has addressed all your comments.

@daxian-dbw daxian-dbw self-assigned this Jul 5, 2017
@daxian-dbw daxian-dbw merged commit d1e05ef into PowerShell:master Jul 6, 2017
@daxian-dbw
Copy link
Member

@TimCurwick Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants