InitialSessionState: Add support for configuring initial working directory

[MatejKafka]

PR Summary

Add support for configuring the initial working directory of a Runspace using a new WorkingDirectory property on InitialSessionState.

I recommend reviewing the 2 commits separately, first one refactors the method without major changes in behavior, second one implements the feature.

PR Context

Resolves #17603.

Design questions

@vexx32 I have 2 questions about the design of the feature and related functionality:

1) If the working directory is invalid (does not exist, unknown drive,...), I propagate the exception back to the caller, without checking ThrowOnRunspaceOpenError. Should I add a check and suppress the exception if it's not set? My reasoning is that if a caller explicitly sets a custom working directory, he probably expects it to be valid and get notified when it's not, even when he's not interested in other initialization errors, but it's inconsistent with how other errors are handled.

2) In the original implementation, there's a check for if (context.EngineSessionState.ProviderCount > 0) and if there are no providers, the function does not do anything, leaving the current drive and location unset. However, this results in subsequent accesses to the location throwing confusing internal errors (even from scripts running inside the Runspace):

$iss = [initialsessionstate]::CreateDefault2()
$iss.ThrowOnRunspaceOpenError = $true
# remove all providers
$iss.Providers.RemoveItem(0, $iss.Providers.Count)

$r = [runspacefactory]::CreateRunspace($Host, $iss)
$r.Open()
[powershell]::Create($r).AddCommand("pwd").Invoke()
# Exception calling "Invoke" with "0" argument(s): "Cannot perform operation because operation "get_CurrentLocation" is not valid. Remove operation "get_CurrentLocation", or investigate why it is not valid."

For now, I've kept the original behavior, but it seems cleaner to me to throw an exception if there are no providers defined to avoid the confusing errors later on. I can either modify the behavior in a subsequent commit, or I can create a new PR after this one is done, just wanted to hear your opinion.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

[jborean93]

The codefactor tests look like they need fixing, the PowerShell team won't really consider PRs ready until CI is green.

There should also be tests added for this new API to verify the new changes added. We should be testing things like

• We can set it to a custom location
• UNC paths work (Windows only and can use \\localhost\...)
• Can set to a root of another drive, i.e. Temp and/or Env
• Fails on invalid path (missing and invalid drive)

[MatejKafka]

I fixed the two issues mentioned in code review.

The codefactor tests look like they need fixing, the PowerShell team won't really consider PRs ready until CI is green.

All of the codefactor issues are doc only, and most of the surrounding code is also breaking the rules. If you want me to fix the tests, I can, but I don't see any benefit from it. :)

There should also be tests added for this new API to verify the new changes added. We should be testing things like

If the WG wants me to add unit tests, I can, but none of the other properties on InitialSessionState are currently unit tested.

[jborean93]

All of the codefactor issues are doc only, and most of the surrounding code is also breaking the rules. If you want me to fix the tests, I can, but I don't see any benefit from it. :)

Unfortunately it's part of the CI tests, it needs to be fixed for it to be considered ready. It's part of the style tests of PowerShell and the documentation even derives what will be part of the docs when the it is live.

If the WG wants me to add unit tests, I can, but none of the other properties on InitialSessionState are currently unit tested.

That doesn't mean you shouldn't add tests. Tests are critical to show that the code you've added works and also ensures there are no regressions in the future. I'm not part of the WG but you should always add tests to anything you touch regardless of whether or not there are existing ones.

[MatejKafka]

I removed the exception docstrings, which fixes most of the codefactor issues. One of the remaining issues does not make any sense (spacing around new(...)), fixing the other one would make the docstring less readable and no other property in InitialSessionState uses the suggested pattern.

I also added a few unit tests for the Location property.

I consider the PR ready to review.

---

Tests are critical to show that the code you've added works and also ensures there are no regressions in the future.

Maybe if PowerShell maintainers heeded that advice, I wouldn't be finding PowerShell bugs every other week, but I digress.

[jborean93]

One of the remaining issues does not make any sense (spacing around new(...)

The rules that PowerShell uses don't like the new() syntax without the type name. You need to use new TypeName(...); instead.

Maybe if PowerShell maintainers heeded that advice, I wouldn't be finding PowerShell bugs every other week, but I digress.

That's why adding tests are critical, we can't do much about existing tests but we can be sure that new features and fixes being added now have tests (where feasible).

[MatejKafka]

I don't believe the Pester test failures are related to my PR.