Enable Get-TimeZone for *nix and Mac OS. #3735

adityapatwardhan · 2017-05-08T22:27:46Z

Get-TimeZone can be enabled now as the required classes are available in .Net Standard 2.0.
Removed workaround for TimeZoneNotFoundException
Test fixes
Fixes Port Get-TimeZone cmdlet #3605

JamesWTruher · 2017-05-09T17:35:14Z

src/Microsoft.PowerShell.Commands.Management/commands/management/TimeZoneCommands.cs

@@ -863,4 +848,4 @@ internal static TimeZoneInfo[] LookupSystemTimeZoneInfoByName(string name)
    }
 }

-#endif


additional empty lines?

Removed extra lines at end of file.

iSazonov · 2017-05-09T17:22:31Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

@@ -57,49 +56,53 @@ Describe "Get-Timezone test cases" -Tags "CI" {
    It "Call with ListAvailable switch returns a list containing TimeZoneInfo.Local" {
        $observedIdList = Get-TimeZone -ListAvailable | Select-Object -ExpandProperty Id
        $oneExpectedId = ([System.TimeZoneInfo]::Local).Id
-        $observedIdList -contains $oneExpectedId | Should Be $true
+        $observedIdList -eq $oneExpectedId | Should Be $oneExpectedId


It seems $observedIdList -contains $oneExpectedId | Should Be $true was good.

iSazonov · 2017-05-09T17:23:06Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

    }

    It "Call with ListAvailable switch returns a list containing one returned by Get-TimeZone" {
        $observedIdList = Get-TimeZone -ListAvailable | Select-Object -ExpandProperty Id
        $oneExpectedId = (Get-TimeZone).Id
-        $observedIdList -contains $oneExpectedId | Should Be $true
+        $observedIdList -eq $oneExpectedId | Should Be $oneExpectedId


It seems $observedIdList -contains $oneExpectedId | Should Be $true was good.

iSazonov · 2017-05-09T17:24:49Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

    }

    It "Call Get-TimeZone using ID param and single item" {
-        (Get-TimeZone -Id "Cape Verde Standard Time").Id -eq "Cape Verde Standard Time" | Should Be $true
+        $selectedTZ = $TimeZonesAvailable | Select-Object -Index ((Get-Random) % $TimeZonesAvailable.Count)
+        (Get-TimeZone -Id $selectedTZ.Id).Id | Should Be $selectedTZ.Id


@it seems Get-Random isn't good for tests. Maybe cycle for all $TimeZonesAvailable is better and then we can remove next test.

On a typical Ubuntu 16.04 machine there are 424 time zones. Checking for all time zones does not provide additional value to the test, but increases test execution time.

Though Get-Random picks a different time zone when the test is run, Pester will show what the expected output was, and what the actual output is. So, debugging or reproducing the failure should not be a problem. Your thoughts?

The next test uses multiple time zone IDs for the -Id parameter, hence it is a different test.

@iSazonov let me know what you think?

I think the tests should be deterministic and I would prefer don't use Get-Random because this reproducibly in manual locally but not in CI restart.
Perhaps it makes no sense to test all zones but it is very simple and very fast - I believe we may not worry about it here. (Although we may be limited to some selected time zone names/ids.)

@iSazonov I agree on tests being deterministic. Though it seems like a small increase in execution time (from 80ms to 780ms on my machine), it eventually adds up. Since it does not provide additional coverage, I do not think we should go through all of them.

I cannot use names / IDs because there is no guarantee that the time zone is available on the machine. Time zone names/IDs are different of different operating systems. Example:

Get-TimeZone -Name Pacific* | Select-Object -ExpandProperty Id

It has different output in Windows v/s Ubuntu.

I cannot use Select-Object -First 3, as there is no guarantee that the system has 3 time zones.

What do you think?

Maybe create three separated "testcases" - for Windows, Linux and Mac?

I think a reasonable test would be that if you get multiple timezones, check the first n (no need for random sample) and the break out of the test. No need to check them all.
FWIW, this is still not deterministic because it relies on what is installed which may change across systems, and may not even satisfy the CI restart scenario if the config changes

iSazonov · 2017-05-09T17:29:09Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

-        $idList = @("Cape Verde Standard Time","Morocco Standard Time","Azores Standard Time")
-        $result = (Get-TimeZone -Id $idList).Id
-        Assert-ListsSame $result $idList
+        $randomIndexes = ((Get-Random) % $TimeZonesAvailable.Count), ((Get-Random) % $TimeZonesAvailable.Count),((Get-Random) % $TimeZonesAvailable.Count)


The same about Get-Random.

iSazonov · 2017-05-09T17:29:22Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

    }

    It "Call Get-TimeZone using ID param and multiple items, where first and third are invalid ids - expect error" {
-        $null = Get-TimeZone -Id @("Cape Verde Standard","Morocco Standard Time","Azores Standard") `
+        $selectedTZ = ($TimeZonesAvailable | Select-Object -Index ((Get-Random) % $TimeZonesAvailable.Count)).Id


The same about Get-Random.

iSazonov · 2017-05-09T17:29:35Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

-                               -ErrorAction SilentlyContinue | ForEach-Object Id
-        $expectedIdList = @("Cape Verde Standard Time","Morocco Standard Time","Azores Standard Time")
-        Assert-ListsSame $expectedIdList $result
+        $randomIndexes = ((Get-Random) % $TimeZonesAvailable.Count), ((Get-Random) % $TimeZonesAvailable.Count),((Get-Random) % $TimeZonesAvailable.Count)


The same about Get-Random.

JamesWTruher · 2017-05-09T20:20:46Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

-        $idList = @("Cape Verde Standard Time","Morocco Standard Time","Azores Standard Time")
-        $result = (Get-TimeZone -Id $idList).Id
-        Assert-ListsSame $result $idList
+        $randomIndexes = ((Get-Random) % $TimeZonesAvailable.Count), ((Get-Random) % $TimeZonesAvailable.Count),((Get-Random) % $TimeZonesAvailable.Count)


Get-Random [](start = 27, length = 10)

also, if you're going to use get-random, it might be more efficient to do:
get-random -input (1..$TimeZonesAvailable.Count) -count 3

JamesWTruher · 2017-05-09T20:24:00Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

    }

    It "Call Get-TimeZone using ID param and single item" {
-        (Get-TimeZone -Id "Cape Verde Standard Time").Id -eq "Cape Verde Standard Time" | Should Be $true
+        $selectedTZ = $TimeZonesAvailable | Select-Object -Index ((Get-Random) % $TimeZonesAvailable.Count)


$TimeZonesAvailable | Select-Object -Index ((Get-Random) % $TimeZonesAvailable.Count) [](start = 22, length = 85)

$TimeZonesAvailable[(Get-Random -max ($TimeZonesAvailable.Count +1))] avoids a pipeline

daxian-dbw · 2017-05-11T21:18:24Z

src/Modules/Unix/Microsoft.PowerShell.Management/Microsoft.PowerShell.Management.psd1

@@ -2,7 +2,7 @@
 GUID="EEFCB906-B326-4E99-9F54-8B4BB6EF3C6D"
 Author="Microsoft Corporation"
 CompanyName="Microsoft Corporation"
-Copyright="� Microsoft Corporation. All rights reserved."
+Copyright="� Microsoft Corporation. All rights reserved."


Please retain the original character. This unintentional change is usually done by VSCode.

adityapatwardhan · 2017-05-11T21:54:43Z

@iSazonov @JamesWTruher @daxian-dbw I have done the suggested changes.

daxian-dbw · 2017-05-11T22:07:37Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

@@ -35,71 +30,88 @@ Describe "Get-Timezone test case no switches" -Tags "CI" {
    It "Call without ListAvailable switch returns current TimeZoneInfo" {
        $observed = (Get-TimeZone).Id
        $expected =
A36C
 ([System.TimeZoneInfo]::Local).Id


Would this test work if [System.TimeZoneInfo]::GetSystemTimeZones().Count -eq 0? If not, then it should also be skipped in that case, like what you do for Describe "Get-Timezone test cases.

Fixed. Moved it to the same Describe.

daxian-dbw · 2017-05-11T22:08:03Z

test/powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1

+        $TimeZonesAvailable = [System.TimeZoneInfo]::GetSystemTimeZones()
+
+        $defaultParamValues = $PSdefaultParameterValues.Clone()
+        $PSDefaultParameterValues["it:skip"] = ($TimeZonesAvailable.Count -eq 0)


In what case would $TimeZonesAvailable.Count -eq 0 be true?

If the tzdata package is removed.

good to know. Thanks

daxian-dbw · 2017-05-11T22:12:48Z

src/Modules/Unix/Microsoft.PowerShell.Management/Microsoft.PowerShell.Management.psd1

@@ -2,7 +2,7 @@
 GUID="EEFCB906-B326-4E99-9F54-8B4BB6EF3C6D"
 Author="Microsoft Corporation"
 CompanyName="Microsoft Corporation"
-Copyright="� Microsoft Corporation. All rights reserved."
+Copyright="© Microsoft Corporation. All rights reserved."


When it looks the same but git shows a diff, it's usually due to a special character change. What I would do in this case is to first revert changes to this file (git checkout), and then open it in a different editor (not vscode), and make the Get-TimeZone change.
@iSazonov do you happen to know if there is any configuration file we can use to prevent VSCode from messing up special characters?

Get-TimeZone can be enabled now as the required classes are available in .Net Standard 2.0 Address code review feedback

adityapatwardhan requested a review from JamesWTruher May 8, 2017 22:27

msftclas added the cla-not-required label May 8, 2017

adityapatwardhan force-pushed the GetTimeZone branch from b664ffe to e1b5b42 Compare May 9, 2017 17:33

JamesWTruher reviewed May 9, 2017

View reviewed changes

iSazonov reviewed May 9, 2017

View reviewed changes

JamesWTruher reviewed May 9, 2017

View reviewed changes

daxian-dbw self-assigned this May 11, 2017

daxian-dbw reviewed May 11, 2017

View reviewed changes

Enable Get-TimeZone for *nix and Mac OS.

cfdae30

Get-TimeZone can be enabled now as the required classes are available in .Net Standard 2.0 Address code review feedback

adityapatwardhan force-pushed the GetTimeZone branch from d4751aa to cfdae30 Compare May 11, 2017 22:26

daxian-dbw approved these changes May 11, 2017

View reviewed changes

daxian-dbw merged commit 89638a8 into PowerShell:master May 11, 2017

adityapatwardhan deleted the GetTimeZone branch May 11, 2017 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Get-TimeZone for *nix and Mac OS. #3735

Enable Get-TimeZone for *nix and Mac OS. #3735

                   }
               }
-              #endif

Enable Get-TimeZone for *nix and Mac OS. #3735

Enable Get-TimeZone for *nix and Mac OS. #3735

Conversation

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment