Fixing JSON property accessor to attempt user settings directory creation on the first write operation - #2387

There is a race condition here - another process could create the directory after the Exists test fails. You should probably catch the appropriate exception from CreateDirectory and ignore it if that happens.

CreateDirectory doesn't throw an exception if it already exists. It just returns the directory.
https://msdn.microsoft.com/en-us/library/54a0at6s(v=vs.110).aspx

Now the question becomes... is the check if it exists necessary? It feels like it is slightly faster to check first in scenarios where the directory already exists.

Indeed - CreateDirectory will check if the directory exists, so we can remove our redundant check.

Maybe $IsNotSkipped = $IsWindows?
Or even better, why not just use $IsWindows without $IsNotSkipped? I don't feel this variable helps readability.

Fixed

#Closed

$shouldSkipTest is not used in this test.

fixed

#Closed

Assuming the first module path is the user module path is fragile. This may not work when a dev is running the test with a profile. I think it's safer to construct the path to the user property.json file. That should be easy given that the test only runs on win-platform.

Fixed

#Closed

Minor question: is it really necessary to create the file when it's requested to remove a property? If the file doesn't exist, then no action needed, and the removal will be sure successful.

Good point. I removed it

#Closed

$originalDefaultParameterValues = $PSDefaultParameterValues.Clone()
$IsNotSkipped = ($IsWindows -eq $true);
$PSDefaultParameterValues["it:skip"] = !$IsNotSkipped
$ShouldSkipTest = !$IsNotSkipped

I think all these can be moved to BeforeAll { } and $global:PSDefaultParameterValues = $originalDefaultParameterValues can be moved to AfterAll { }.
Some examples:
PowerShell\test\powershell\Language\Scripting\I18n.Tests.ps1
PowerShell\test\powershell\Modules\Microsoft.PowerShell.Diagnostics\Get-WinEvent.Tests.ps1

This is the preferred technique because it results in less duplication when there are multiple Describe blocks within the try-finally.
For background, see the discussion in PR #2335

Thanks for the link to the discussion. I think it depends on how many Describe sections you expect to be in a file. If there is just one Describe, then it's cleaner to have it in BeforeAlll and AfterAll. But I'm fine keep the try/finally as it's possible that we will add more Describe sections later. #Closed

original the
Should be the original

#Closed

Use the powershell way to do it: if (-not $backupPropertiesFile)

Fixed

#Closed

Do we want to create the directory C:\Users\dongbow\Documents when it already exists?
Probably it should be

elseif (-not (Test-Path $userSettingsDir))
{
    $null = New-Item -Type Directory -Path $userSettingsDir -Force -ErrorAction SilentlyContinue
}

Fixed

#Closed

Maybe add one more check to verify that read operation should not create the file.

Added

#Closed

this line seems not needed given the same command will run in BeforeEach { }

#Closed

Thanks for taking Nano/IoT PS into account. Now it makes sense to have $skipTest = !$IsWindows -or $IsInbox, because the if ($IsWindows) below should actually be if (!skipTest).

You know what, we should exclude the FullCLR powershell as well 😄 Ah, I guess $isInbox already covers that.

#Closed

It should be if (!skipTest)

#Closed

It should be if (!skipTest)

#Closed

It should be if (!skipTest)

#Closed