Added tests for builtin type accelerators by dchristian3188 · Pull Request #4230 · PowerShell/PowerShell

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

-        $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

        $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

Conversation

dchristian3188 commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dchristian3188 commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daxian-dbw Jul 13, 2017 •

edited

Loading

daxian-dbw Jul 13, 2017 •

edited

Loading