Skip to content

Reset the interactive window using IInteractiveWindowOperations#52453

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:reset-window
Apr 7, 2021
Merged

Reset the interactive window using IInteractiveWindowOperations#52453
sharwell merged 1 commit intodotnet:mainfrom
sharwell:reset-window

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Apr 7, 2021

The previous implementation relies on using DTE to execute a named command. However, since it is not the purpose of these tests to cover invoking this command by name (a dedicated test could be created if desired), we modify the code to instead directly invoke the backing API for this command. This change reduces the likelihood that a failure in the commanding infrastructure leads to an obscure test failure.

Closes #52409

@sharwell sharwell requested a review from a team as a code owner April 7, 2021 00:19
@ghost ghost added the Area-Interactive label Apr 7, 2021

public void Reset(bool waitForPrompt = true)
{
ExecuteCommand(WellKnownCommandNames.InteractiveConsole_Reset);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why isn't this way stable? Is this actually hiding a bug where the user command won't be reliable?

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Apr 7, 2021

Choose a reason for hiding this comment

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

@tmat and I were not able to find a root cause looking through heap dumps all day. My best guess is we are somehow issuing the command at a point where IsResetting is true and it ends up getting dropped:

https://github.com/dotnet/interactive-window/blob/f45f6bef9e002b4b661ad7abf73f5dc996c0709d/src/Microsoft.VisualStudio.VsInteractiveWindow/VsInteractiveWindowCommandFilter.cs#L374

At least by calling the true underlying command we'll be able to see the task in future heap dumps.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, ExecuteCommand doesn't return a failure if we try executing it when the command is unavailable?

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Apr 7, 2021

Choose a reason for hiding this comment

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

The documentation just says:

ExecuteCommand cannot execute commands that are currently disabled in the environment. The Build method, for example, will not execute while a build is currently in progress.

The method returns void and doesn't declare any exceptions.

I'd check the implementation but DTE...

@sharwell sharwell enabled auto-merge April 7, 2021 18:18
@sharwell sharwell merged commit 275f4c7 into dotnet:main Apr 7, 2021

public void Reset(bool waitForPrompt = true)
{
ExecuteCommand(WellKnownCommandNames.InteractiveConsole_Reset);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, ExecuteCommand doesn't return a failure if we try executing it when the command is unavailable?

@ghost ghost added this to the Next milestone Apr 7, 2021
@sharwell sharwell deleted the reset-window branch April 7, 2021 19:08
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test - Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpInteractive.BclMathCall

3 participants