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

Ensure the caret is visible after commit #33827

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Mar 2, 2019

Fixes #33822

@sharwell sharwell added this to the 16.0 milestone Mar 2, 2019
@sharwell sharwell force-pushed the caret-visible branch 2 times, most recently from c9a082d to b01972e Compare March 2, 2019 20:10
@sharwell sharwell removed the Blocked label Mar 3, 2019
@sharwell sharwell marked this pull request as ready for review March 3, 2019 20:22
@sharwell sharwell requested a review from a team as a code owner March 3, 2019 20:22
Copy link
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

  1. I tried to repro the integration test. It works without the fix; the caret is visible.
  2. I think we need a unit test to prove the need for the fix.

@sharwell
Copy link
Member Author

sharwell commented Mar 4, 2019

I tried to repro the integration test. It works without the fix; the caret is visible.

The integration tests default to legacy completion. You need to patch the following line or set an environment variable to enable async completion for integration tests. The bug reproduces 100% of the time with async completion.

public override bool ShouldSkip => string.Equals(Environment.GetEnvironmentVariable("ROSLYN_TEST_LEGACY_COMPLETION"), "true", StringComparison.OrdinalIgnoreCase);

I think we need a unit test to prove the need for the fix.

The unit test would be inconclusive relative to an integration test. I will file a follow-up item to add a unit test, but the current integration test is a conclusive demonstration of the issue at hang in the context of the Visual Studio product.

@sharwell sharwell requested a review from ivanbasov March 4, 2019 20:00
@ivanbasov
Copy link
Contributor

I re-evaluated it with the new completion and found the issue. Sorry for confusion!
There is an issue.

The fix is good. I am fine to merge it into master with a follow up issue for a unit test.

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.

4 participants