Added tests for builtin type accelerators #4230

dchristian3188 · 2017-07-12T05:44:28Z

First pass of tests for built in type accelerators.

Referenced in issue: #4221

iSazonov · 2017-07-12T11:48:09Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+
+        $TypeAccelerators = $TypeAcceleratorsType::Get
+        $TypeAcceleratorsType::Add('msft_2174855', [int])
+        $TypeAcceleratorsType::Add('msft_2174855_rm', [int])


it would good to decode 'msft_2174855' and 'msft_2174855_rm' - can we use more informative names and comments for it?

iSazonov · 2017-07-12T11:48:49Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+        } else
+        {
+            $count = 82 
+        }


I believe it is better use the pattern and formatting https://github.com/PowerShell/PowerShell/blob/master/test/powershell/engine/DefaultCommands.Tests.ps1

Please address the comment.

Resolved in latest commit

iSazonov · 2017-07-12T11:50:06Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+
+    It 'Should have a type acclerator for: <Accelerator>' -TestCases $TypeAcceleratorTestCases {
+        param($Accelerator, $Type)
+        $TypeAcceleratorsType::Get[$Accelerator] | Should be ($Type)


Can we use Should BeOfType ?

This did not work as expected. The object returned by the type accelerator get is System.RuntimeType.

$TypeAcceleratorsType = [psobject].Assembly.GetType("System.Management.Automation.TypeAccelerators") $TypeAcceleratorsType::Get['String'] $TypeAcceleratorsType::Get['String'] | Get-Member

daxian-dbw · 2017-07-12T23:47:48Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

    }

    It "Can query type accelerators" {
-        if ( $IsCoreCLR ) { $count = 80 } else { $count = 82 }
-        $TypeAccelerators.Count -gt $count | Should Be $true
+        $TypeAccelerators.Count -gt $totalAccelerators | Should Be $true


I think it would be good to test against the exact number of built-in accelerators (89). And it's recommended to write as $TypeAccelerators.Count | Should Be $totalAccelerators because it would give more information compared to Should Be $true in case the test fails.

daxian-dbw · 2017-07-12T23:48:16Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+        } 
+        else
+        {
+            $totalAccelerators = 82


I think it's better to test the exact number of built-in accelerators.

Understood, will make the update tonight.

daxian-dbw

Thanks for adding tests for accelerators. Some comments below.

daxian-dbw · 2017-07-12T23:50:02Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

    }

    It "Can query type accelerators" {
-        if ( $IsCoreCLR ) { $count = 80 } else { $count = 82 }
-        $TypeAccelerators.Count -gt $count | Should Be $true
+        $TypeAccelerators.Count -gt $totalAccelerators | Should Be $true
        $TypeAccelerators['xml'] | Should Be ([System.Xml.XmlDocument])
        $TypeAccelerators['AllowNull'] | Should Be ([System.Management.Automation.AllowNullAttribute])


These two tests can be removed now as we are testing the complete set of accelerators 😄

daxian-dbw · 2017-07-12T23:52:57Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

    }

    It "Can query type accelerators" {
-        if ( $IsCoreCLR ) { $count = 80 } else { $count = 82 }
-        $TypeAccelerators.Count -gt $count | Should Be $true
+        $TypeAccelerators.Count -gt $totalAccelerators | Should Be $true
        $TypeAccelerators['xml'] | Should Be ([System.Xml.XmlDocument])
        $TypeAccelerators['AllowNull'] | Should Be ([System.Management.Automation.AllowNullAttribute])
    }

    It "Can remove type accelerator" {


I recommend to group this test and It "Basic type accelerator usage" together in a Context, and then move the initialization of userDefinedAcceleratorType and userDefinedAcceleratorTypeToRemove to the BeforeAll of the Context. And remember to clean them up in AfterAll block of the same Context.

dchristian3188 · 2017-07-13T03:25:12Z

Changes made, let me know how it looks.

iSazonov · 2017-07-13T05:35:48Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+
+        It 'Should have all the type accelerators' {
+            $TypeAccelerators.Count | Should be $totalAccelerators
+        }


Let's clarify. I still believe that we should follow the pattern to make the tests deterministic and well formatting.

It makes no sense to check the number of because that one accelerator can be replaced with other and we won't see the difference.

Accelerator may not be supported on all protforms. So we should check this by " $($FullCLR -or $CoreWindows -or $CoreUnix)".

Formatting should be tabular. It is more compact and we can read easy:

$TypeAcceleratorTestCases = @" "Accelerator", "Type", "Present" "wmisearcher", "System.Management.ManagementObjectSearcher", $($FullCLR -or $CoreWindows -or $CoreUnix) "@

It makes no sense to check the number of because that one accelerator can be replaced with other and we won't see the difference.

There are tests to check each of the accelerators, so we will see a failure if one is replaced with another. This "count" test makes sure no new accelerator is added accidentally.

Accelerator may not be supported on all protforms. So we should check this by " $($FullCLR -or $CoreWindows -or $CoreUnix)".

Accelerators are essentially .NET types, so I think there is no difference between CoreWindows and CoreUnix as they both use .NET Core.

I still believe that we should follow the pattern to make the tests deterministic and well formatting.

I agree that the DefaultCommands.Tests.ps1 is well-formatted and I recommend to use a similar pattern. But I don't think a PR should be blocked if it's clear enough (not confusing in any ways).

Thanks for clarify!

There are tests to check each of the accelerators, so we will see a failure if one is replaced with another. This "count" test makes sure no new accelerator is added accidentally.

If we have explicit full accelerator list in the file we catch the problem - in DefaultCommands.Tests.ps1 we compare two arrays for this.

Yes, I see your point. In DefaultCommands.Tests.ps1 we guarantee no unexpected alias or cmdlet by comparing the full list and that's why we don't need the count check in that test.

daxian-dbw · 2017-07-13T19:28:42Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

+            {
+                $totalAccelerators = 89 
+            } else
+            {


Should be

} else {

daxian-dbw · 2017-07-13T19:31:41Z

test/powershell/Language/Parser/TypeAccelerator.Tests.ps1

-    It "Basic type accelerator usage" {
-        [msft_2174855] | Should Be ([int])
-    }
+                $nonCoreTypeAcceleratorTestCases = @(


Can you please change the name to $extraFullPSAcceleratorTestCases?

…TestCases'

daxian-dbw

LGTM

Added tests for builtin type accelerators

e7207aa

msftclas added the cla-already-signed label Jul 12, 2017

iSazonov reviewed Jul 12, 2017

View reviewed changes

daxian-dbw assigned adityapatwardhan Jul 12, 2017

dchristian3188 added 2 commits July 12, 2017 08:15

Moved variables to before all

a457b39

Cleaned up user type accelerator names

b406cac

daxian-dbw reviewed Jul 12, 2017

View reviewed changes

Updated Tests

f406a1c

iSazonov reviewed Jul 13, 2017

View reviewed changes

daxian-dbw reviewed Jul 13, 2017

View reviewed changes

Change '$nonCoreTypeAcceleratorTestCases' to '$extraFullPSAccelerator…

307a843

…TestCases'

daxian-dbw approved these changes Jul 14, 2017

View reviewed changes

daxian-dbw merged commit e333ac3 into PowerShell:master Jul 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for builtin type accelerators #4230

Added tests for builtin type accelerators #4230

dchristian3188 commented Jul 12, 2017

iSazonov Jul 12, 2017

iSazonov Jul 12, 2017

iSazonov Jul 12, 2017

iSazonov Jul 12, 2017

dchristian3188 Jul 12, 2017

iSazonov Jul 12, 2017

dchristian3188 Jul 12, 2017

iSazonov Jul 12, 2017

daxian-dbw Jul 12, 2017

daxian-dbw Jul 12, 2017

dchristian3188 Jul 12, 2017

daxian-dbw left a comment

daxian-dbw Jul 12, 2017

daxian-dbw Jul 12, 2017

dchristian3188 commented Jul 13, 2017

iSazonov Jul 13, 2017

daxian-dbw Jul 13, 2017

iSazonov Jul 14, 2017

daxian-dbw Jul 14, 2017

daxian-dbw Jul 13, 2017 •

edited

Loading

daxian-dbw Jul 13, 2017 •

edited

Loading

daxian-dbw left a comment

Added tests for builtin type accelerators #4230

Added tests for builtin type accelerators #4230

Conversation

dchristian3188 commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchristian3188 commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

daxian-dbw Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

daxian-dbw Jul 13, 2017 •

edited

Loading

daxian-dbw Jul 13, 2017 •

edited

Loading