Skip to content
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.

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

Retry opening LightBulb when the session is canceled #58947

Closed
wants to merge 1 commit into from

Conversation

JoeRobich
Copy link
Member

Resolves #57722

ShowLightBulb();
WaitForLightBulbSession();
}
catch (InvalidOperationException)
Copy link
Contributor

Choose a reason for hiding this comment

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

When the session is canceled instead of completed we throw an InvalidOperationException

This makes me sad :( It should be a cancellation exception right?

Copy link
Member

Choose a reason for hiding this comment

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

who is 'we' here?

Copy link
Member

Choose a reason for hiding this comment

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

hrmm. i see:

                if (DateTimeOffset.Now > startTime + Helper.HangMitigatingTimeout)
                {
                    throw new InvalidOperationException("Expected a light bulb session to appear.");
                }

But this seems appropriate. if we invoked the lightbulb... and it didn't appear. then we have a product bug. retrying it legit seems to be maskign things.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

this seems to mask a real product failure.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Reviewing relative to substantial changes already made in the new integration tests

@sharwell
Copy link
Member

this seems to mask a real product failure.

It is a real product failure, but outside our control. We can't just leave all code action integration tests disabled for months waiting for a public release with a fix.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This change should use the following approach:

public async Task InvokeCodeActionListAsync(CancellationToken cancellationToken)
{
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.SolutionCrawler, cancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.DiagnosticService, cancellationToken);
if (Version.Parse("17.1.31916.450") > await TestServices.Shell.GetVersionAsync(cancellationToken))
{
// Workaround for extremely unstable async lightbulb prior to:
// https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform/pullrequest/361759
await TestServices.Input.SendAsync(new KeyPress(VirtualKey.Period, ShiftState.Ctrl));
await Task.Delay(5000, cancellationToken);
await TestServices.Editor.DismissLightBulbSessionAsync(cancellationToken);
await Task.Delay(5000, cancellationToken);
}
await ShowLightBulbAsync(cancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.LightBulb, cancellationToken);
}

@CyrusNajmabadi
Copy link
Member

It is a real product failure, but outside our control.

That's fair. Do we have a tracking bug for the real failure? we can then undo this once that goes in.

@sharwell
Copy link
Member

Do we have a tracking bug for the real failure?

Already fixed (internal PR linked in code in my reply)

@JoeRobich
Copy link
Member Author

superseded by #59457

@JoeRobich JoeRobich closed this Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants