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

Support a single OOP call to perform all of rename, on top of the individual pieces of rename that are already OOPed #43593

Merged
merged 11 commits into from
Apr 25, 2020

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Apr 23, 2020

Followup to #43592. That PR should go in first.

This also adds OOP tests for direct calls to rename, as well as OOP calling the individual parts of rename

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners April 23, 2020 06:35
@CyrusNajmabadi CyrusNajmabadi requested a review from ryzngard April 25, 2020 06:01
@CyrusNajmabadi
Copy link
Member Author

@ryzngard This is ready for review. Thanks!

@@ -55,7 +56,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
workspace.SetTestLogger(AddressOf helper.WriteLine)

workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host = TestHost.OutOfProcess)))
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host <> TestHost.InProcess)))
Copy link
Member Author

Choose a reason for hiding this comment

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

@ryzngard This is an example of what i was talking about earllier. We want OOP enabled for all optoins that are not 'InProcess'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow. Are there multiple OutOfProcess values? It seems like you're basically comparing value != false instead of value == true (ignoring that value could just be the argument here...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading further now I get it

    Public Enum TestHost
        InProcess	        InProcess
        OutOfProcess	        ' Work out of process, marshaling to/from RenameSymbolAsync
        OutOfProcess_SingleCall
        ' Work out of process, marshaling to/from FindRenameLocations, then marshaling to/from ResolveConflictsAsync
        OutOfProcess_SplitCall
    End Enum	    End Enum

There are multiple out of process hosts. That's the missing bit :)

@CyrusNajmabadi CyrusNajmabadi changed the title Add OOP tests for direct calls to rename, as well as OOP calling the individual parts of rename Support a single OOP call to perform all of rename, on top of the individual pieces of rename that are already OOPed Apr 25, 2020
@@ -55,7 +56,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
workspace.SetTestLogger(AddressOf helper.WriteLine)

workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host = TestHost.OutOfProcess)))
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host <> TestHost.InProcess)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow. Are there multiple OutOfProcess values? It seems like you're basically comparing value != false instead of value == true (ignoring that value could just be the argument here...)

@@ -55,7 +56,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename
workspace.SetTestLogger(AddressOf helper.WriteLine)

workspace.TryApplyChanges(workspace.CurrentSolution.WithOptions(
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host = TestHost.OutOfProcess)))
workspace.Options.WithChangedOption(RemoteHostOptions.RemoteHostTest, host <> TestHost.InProcess)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading further now I get it

    Public Enum TestHost
        InProcess	        InProcess
        OutOfProcess	        ' Work out of process, marshaling to/from RenameSymbolAsync
        OutOfProcess_SingleCall
        ' Work out of process, marshaling to/from FindRenameLocations, then marshaling to/from ResolveConflictsAsync
        OutOfProcess_SplitCall
    End Enum	    End Enum

There are multiple out of process hosts. That's the missing bit :)

@CyrusNajmabadi
Copy link
Member Author

Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit 73b6384 into dotnet:master Apr 25, 2020
@ghost ghost added this to the Next milestone Apr 25, 2020
@CyrusNajmabadi CyrusNajmabadi deleted the renameOOP7 branch April 25, 2020 22:59
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants