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.

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

Jump to bottom

Update NavigateTo tests to support AllInOneSearch #64582

Merged
merged 4 commits into from
Oct 10, 2022
Merged

Update NavigateTo tests to support AllInOneSearch #64582

merged 4 commits into from
Oct 10, 2022

Conversation

JoeRobich
Copy link
Member

@JoeRobich JoeRobich commented Oct 7, 2022 •
edited
Loading

  • Add ShowNavigateToDialogAsync method which executes the NavigateTo command and waits for the dialog to be active.
  • Update the SendToNavigateToAsync method to pause between applying InputKey to give the search dialog time to update.
  • Update the telemetry events being verified to support AllInOneSearch events

Resolves bullet 1 from #64545 (comment)

@JoeRobich JoeRobich requested a review from a team as a code owner October 7, 2022 22:27
Cosifne
Cosifne approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member

@Cosifne Cosifne left a comment

Choose a reason for hiding this comment

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

Ultimatly we might want to talk to platform team to find a good solution, but this would be an acceptable workaround for me right now

jasonmalinowski
jasonmalinowski approved these changes Oct 8, 2022
View reviewed changes
Copy link
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.

Some questions for possible cleanup but this is fine for getting things off the ground.

src/VisualStudio/IntegrationTest/New.IntegrationTests/CSharp/CSharpNavigateTo.cs

if (isAllInOneSearch)
{
await telemetry.VerifyFiredAsync(new[] { "vs/ide/vbcs/navigateto/search", "vs/ide/search/completed" }, HangMitigatingCancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we even asserting telemetry events in the first place? Isn't this just asserting an implementation detail of the underlying dialog?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I didn't really think it was useful, but I was trying to preserve it. The other tests which drive the NavigateTo dialog do not check for these events.

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/InputInProcess.cs Show resolved Hide resolved
src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/ShellInProcess.cs

await TestServices.Shell.ExecuteCommandAsync(VSConstants.VSStd12CmdID.NavigateTo, cancellationToken);

return await WaitForNavigateToFocusAsync(cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

Why have the local function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes I like to name a bit of code to make it very clear what the code is trying to achieve. Of course I could do that with a comment as well.

src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/ShellInProcess.cs
cancellationToken.ThrowIfCancellationRequested();

// Take no direct action regarding activation, but assert the correct item already has focus
TestServices.JoinableTaskFactory.Run(async () =>
Copy link
Member

Choose a reason for hiding this comment

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

Why calling .Run() here since we're already in an async method?

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 borrowed this code from the InputInProcess.SendToNavigateToAsync. Will admit that I need to learn more about JTF generally. I guess we could move this into the method and add a transition off the main thread following the control check.

@JoeRobich JoeRobich merged commit 33f5f98 into main Oct 10, 2022
@ghost ghost added this to the Next milestone Oct 10, 2022
@allisonchou allisonchou mentioned this pull request Oct 14, 2022
Closed
@Cosifne Cosifne mentioned this pull request Oct 21, 2022
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.5 P1 Oct 24, 2022
@JoeRobich JoeRobich deleted the dev/jorobich/fix-integration-tests branch May 7, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cosifne Cosifne Cosifne approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.5 P1
Development

Successfully merging this pull request may close these issues.
4 participants
@JoeRobich @jasonmalinowski @Cosifne @RikkiGibson