Skip to content

Update integration tests to support both possible Go To Implementation results#59956

Merged
sharwell merged 2 commits intodotnet:mainfrom
sharwell:flaky-impl-test
Mar 7, 2022
Merged

Update integration tests to support both possible Go To Implementation results#59956
sharwell merged 2 commits intodotnet:mainfrom
sharwell:flaky-impl-test

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Mar 4, 2022

Fixes #59950

@JoeRobich

This comment was marked as resolved.

@sharwell sharwell marked this pull request as ready for review March 5, 2022 00:58
@sharwell sharwell requested a review from a team as a code owner March 5, 2022 00:58
@sharwell sharwell requested a review from a team as a code owner March 5, 2022 00:58
if (activeCaption == $"FileImplementation.cs{dirtyModifier}")
{
// The navigation completed synchronously; no further action necessary
identifierWithCaret = "Implementation$$";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is go-to-impl even synchronous when under the time limit? I think it's async for both. Just that if the async portion takes under a certain time it navigates, else it pops up the dialog.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yup:

    private async Task ExecuteCommandWorkerAsync(
        Document document,
        int position,
        CancellationTokenSource cancellationTokenSource)
    {
        // Switch to the BG immediately so we can keep as much work off the UI thread.
        await TaskScheduler.Default;

So, in order to do this, i think you'll need to at least wait the GoToImplementation test listener. than after that you can check teh state of the editor.

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Mar 7, 2022

Choose a reason for hiding this comment

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

We already are waiting for this:

await TestServices.Shell.ExecuteCommandAsync(WellKnownCommands.Edit.GoToImplementation, cancellationToken);
await TestServices.Workspace.WaitForAllAsyncOperationsAsync(
new[] { FeatureAttribute.Workspace, FeatureAttribute.GoToImplementation },
cancellationToken);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah ok. thanks!


// It's not clear why this delay is necessary. Navigation operations are expected to fully complete as part
// of one of the above waiters, but GetActiveWindowCaptionAsync appears to return "Program.cs" (the previous
// window caption) for a short delay after the above complete.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

def should talk to eidtor. from your descriptions, it sounds like tehy're async updating tab titles. but that's def problematic for tests :(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In another PR, I'm consolidating this behavior to a new WorkaroundsInProcess class.

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.

Can this at least link to a tracking bug or that other PR or something?

@sharwell sharwell enabled auto-merge March 7, 2022 03:10
@sharwell sharwell merged commit f813d26 into dotnet:main Mar 7, 2022
@ghost ghost added this to the Next milestone Mar 7, 2022
@sharwell sharwell deleted the flaky-impl-test branch March 7, 2022 23:13
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

PR was merged while I was reviewing; I wholehartedly approve its merging. Random comments in the mean time.


// It's not clear why this delay is necessary. Navigation operations are expected to fully complete as part
// of one of the above waiters, but GetActiveWindowCaptionAsync appears to return "Program.cs" (the previous
// window caption) for a short delay after the above complete.
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.

Can this at least link to a tracking bug or that other PR or something?

Comment on lines +161 to +162
// The navigation completed synchronously; no further action necessary
identifierWithCaret = "Implementation$$";
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, so sync navigation puts the caret in one place, and invoking the navigated item puts in a different place? Not crtiical but @CyrusNajmabadi is this intentional?

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.

CSharpGoToImplementation.SimpleGoToImplementation test is not deterministic

5 participants