Skip to content

Fix DebuggerSetsVariablesWithConversion test #1664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Fix DebuggerSetsVariablesWithConversion test #1664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ea50b7
Clean up `PsesHostFactory`
andyleejordan Jan 13, 2022
fa5db9e
Fix scope guessing logic in `SetVariableAsync`
andyleejordan Jan 13, 2022
2fdae0e
Fix bug in `SetVariableAsync` for strongly typed values
andyleejordan Jan 13, 2022
ccddeba
Suppress warning in `PsesInternalHostTests`
andyleejordan Jan 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 25 additions & 21 deletions src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,14 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str
}

VariableDetailsBase variable = variableContainer.Children[name];
// Determine scope in which the variable lives so we can pass it to `Get-Variable -Scope`.
string scope = null; // TODO: Can this use a fancy pattern matcher?

// Determine scope in which the variable lives so we can pass it to `Get-Variable
// -Scope`. The default is scope 0 which is safe because if a user is able to see a
// variable in the debugger and so change it through this interface, it's either in the
// top-most scope or in one of the following named scopes. The default scope is most
// likely in the case of changing from the "auto variables" container.
string scope = "0";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SeeminglyScience This seem sane to you?

// NOTE: This can't use a switch because the IDs aren't constant.
if (variableContainerReferenceId == localScopeVariables.Id)
{
scope = VariableContainerDetails.LocalScopeName;
Expand All @@ -414,11 +420,6 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str
{
scope = VariableContainerDetails.GlobalScopeName;
}
else
{
// Hmm, this would be unexpected. No scope means do not pass GO, do not collect $200.
throw new Exception("Could not find the scope for this variable.");
}

// Now that we have the scope, get the associated PSVariable object for the variable to be set.
var getVariableCommand = new PSCommand()
Expand Down Expand Up @@ -456,22 +457,25 @@ public async Task<string> SetVariableAsync(int variableContainerReferenceId, str

if (argTypeConverterAttr is not null)
{
// PSVariable *is* strongly typed, so we have to convert it.
_logger.LogTrace($"Setting variable '{name}' using conversion to value: {expressionResult ?? "<null>"}");

// TODO: This is throwing a 'PSInvalidOperationException' thus causing
// 'DebuggerSetsVariablesWithConversion' to fail.
psVariable.Value = await _executionService.ExecuteDelegateAsync(
"PS debugger argument converter",
ExecutionOptions.Default,
(pwsh, _) =>
{
var engineIntrinsics = (EngineIntrinsics)pwsh.Runspace.SessionStateProxy.GetVariable("ExecutionContext");

// TODO: This is almost (but not quite) the same as LanguagePrimitives.Convert(), which does not require the pipeline thread.
// We should investigate changing it.
return argTypeConverterAttr.Transform(engineIntrinsics, expressionResult);
},
// NOTE: We use 'Get-Variable' here instead of 'SessionStateProxy.GetVariable()'
// because we already have a pipeline running (the debugger) and the latter cannot
// run concurrently (threw 'NoSessionStateProxyWhenPipelineInProgress').
IReadOnlyList<EngineIntrinsics> results = await _executionService.ExecutePSCommandAsync<EngineIntrinsics>(
new PSCommand()
.AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjmholt Do you have a better idea or explanation as to why that exception was being thrown?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me. The debugger knows it is running in an active pipeline and runs commands as 'nested' on the target (locally or remotely), whereas SessionStateProxy does not.

.AddParameter("Name", "ExecutionContext")
.AddParameter("ValueOnly"),
CancellationToken.None).ConfigureAwait(false);
EngineIntrinsics engineIntrinsics = results.Count > 0
? results[0]
: throw new Exception("Couldn't get EngineIntrinsics!");

// TODO: This is almost (but not quite) the same as 'LanguagePrimitives.Convert()',
// which does not require the pipeline thread. We should investigate changing it.
Comment on lines +476 to +477
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this, but I don't think it will quite as "well" as the transformation, and I'm unsure actually how to go about it because LanguagePrimitives.ConvertTo() doesn't accept an ArgumentTypeConverterAttribute.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for local debugging sessions. Does this code make local/remote distinctions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not make that distinction. Also, do you mean that LanguagePrimitives would only work for local, or the currently proposed change?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ExecutionContext variable (engineIntrinsics) passes through the serialization system for the remoting case, and is just a PSObject property bag and not a System.Management.Automation.EngineIntrinsics type. The serialization system does not rehydrate returned object into their original types, except for a small set of special cases.

So if you have script code that determines variable scope, that script should run entirely on the remote target, so that it can run in the correct context and 'live' type instances.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure I'm following. Can I ask: will the proposed change behave differently than the existing code in the remoting scenario? If not, then let's kick that down the road a bit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I.e. are both implementations broken in the same way?)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so (remoting may be broken). But I need to look more closely at the code (and not just the changes).

psVariable.Value = argTypeConverterAttr.Transform(engineIntrinsics, expressionResult);
}
else
{
Expand Down Expand Up @@ -641,7 +645,7 @@ private Task<VariableContainerDetails> FetchVariableContainerAsync(string scope)

private async Task<VariableContainerDetails> FetchVariableContainerAsync(string scope, bool autoVarsOnly)
{
PSCommand psCommand = new PSCommand().AddCommand("Get-Variable").AddParameter("Scope", scope);
PSCommand psCommand = new PSCommand().AddCommand(@"Microsoft.PowerShell.Utility\Get-Variable").AddParameter("Scope", scope);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we consistently use the fully qualified name, but since we do, fixed this one too.


var scopeVariableContainer = new VariableContainerDetails(nextVariableId++, "Scope: " + scope);
variables.Add(scopeVariableContainer);
Expand Down
6 changes: 2 additions & 4 deletions test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ await debugService.SetLineBreakpointsAsync(
Assert.Equal(newGlobalIntValue, intGlobalVar.ValueString);
}

[Fact(Skip = "Variable conversion is broken")]
[Fact]
public async Task DebuggerSetsVariablesWithConversion()
{
await debugService.SetLineBreakpointsAsync(
Expand All @@ -628,7 +628,7 @@ await debugService.SetLineBreakpointsAsync(
VariableDetailsBase[] variables = GetVariables(VariableContainerDetails.LocalScopeName);

// Test set of a local string variable (not strongly typed but force conversion)
const string newStrValue = "False";
const string newStrValue = "\"False\"";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was erroneously changed by me previously, and is now back to its original value.

const string newStrExpr = "$false";
VariableScope localScope = Array.Find(scopes, s => s.Name == VariableContainerDetails.LocalScopeName);
string setStrValue = await debugService.SetVariableAsync(localScope.Id, "$strVar2", newStrExpr).ConfigureAwait(true);
Expand Down Expand Up @@ -658,8 +658,6 @@ await debugService.SetLineBreakpointsAsync(
var strVar = Array.Find(variables, v => v.Name == "$strVar2");
Assert.Equal(newStrValue, strVar.ValueString);

scopes = debugService.GetVariableScopes(0);

// Test set of script scope bool variable (strongly typed)
variables = GetVariables(VariableContainerDetails.ScriptScopeName);
var boolVar = Array.Find(variables, v => v.Name == "$scriptBool");
Expand Down
35 changes: 13 additions & 22 deletions test/PowerShellEditorServices.Test/PsesHostFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,13 @@ internal static class PsesHostFactory
// NOTE: These paths are arbitrarily chosen just to verify that the profile paths can be set
// to whatever they need to be for the given host.

public static readonly ProfilePathInfo TestProfilePaths =
new(
Path.GetFullPath(
TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/Test.PowerShellEditorServices_profile.ps1")),
Path.GetFullPath(
TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/ProfileTest.ps1")),
Path.GetFullPath(
TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Test.PowerShellEditorServices_profile.ps1")),
Path.GetFullPath(
TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/ProfileTest.ps1")));
public static readonly ProfilePathInfo TestProfilePaths = new(
Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/Test.PowerShellEditorServices_profile.ps1")),
Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Profile/ProfileTest.ps1")),
Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/Test.PowerShellEditorServices_profile.ps1")),
Path.GetFullPath(TestUtilities.NormalizePath("../../../../PowerShellEditorServices.Test.Shared/ProfileTest.ps1")));

public static readonly string BundledModulePath = Path.GetFullPath(
TestUtilities.NormalizePath("../../../../../module"));

public static System.Management.Automation.Runspaces.Runspace InitialRunspace;
public static readonly string BundledModulePath = Path.GetFullPath(TestUtilities.NormalizePath("../../../../../module"));

public static PsesInternalHost Create(ILoggerFactory loggerFactory)
{
Expand All @@ -53,25 +45,24 @@ public static PsesInternalHost Create(ILoggerFactory loggerFactory)
}

HostStartupInfo testHostDetails = new(
"PowerShell Editor Services Test Host",
"Test.PowerShellEditorServices",
new Version("1.0.0"),
name: "PowerShell Editor Services Test Host",
profileId: "Test.PowerShellEditorServices",
version: new Version("1.0.0"),
psHost: new NullPSHost(),
TestProfilePaths,
profilePaths: TestProfilePaths,
featureFlags: Array.Empty<string>(),
additionalModules: Array.Empty<string>(),
initialSessionState,
initialSessionState: initialSessionState,
logPath: null,
(int)LogLevel.None,
logLevel: (int)LogLevel.None,
consoleReplEnabled: false,
usesLegacyReadLine: false,
bundledModulePath: BundledModulePath);

var psesHost = new PsesInternalHost(loggerFactory, null, testHostDetails);

// NOTE: Because this is used by constructors it can't use await.
// TODO: Should we actually load profiles here?
if (psesHost.TryStartAsync(new HostStartOptions { LoadProfiles = true }, CancellationToken.None).GetAwaiter().GetResult())
if (psesHost.TryStartAsync(new HostStartOptions { LoadProfiles = false }, CancellationToken.None).GetAwaiter().GetResult())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now set this to false because the tests which assert profile loading works do so by explicitly running the same code as would otherwise run in startup:

PowerShellEditorServices/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs

Lines 217 to 221 in 20a3516

if (startOptions.LoadProfiles)
{
await LoadHostProfilesAsync(cancellationToken).ConfigureAwait(false);
_logger.LogInformation("Profiles loaded");
}

I don't think we really need to be calling this delegate for every other test.

{
return psesHost;
}
Expand Down
1 change: 1 addition & 0 deletions test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ await Assert.ThrowsAsync<TaskCanceledException>(() =>
}

[Fact]
[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD003:Avoid awaiting foreign Tasks", Justification = "Explicitly checking task cancellation status.")]
public async Task CanCancelExecutionWithMethod()
{
var executeTask = psesHost.ExecutePSCommandAsync(
Expand Down