Handle Rename and OpenDocument responses from Omnisharp #1854

rchande · 2017-11-10T18:18:01Z

Also adds an integration test that validates the behavior of the "rename
file to match type" refactoring:

File is renamed on disk
Old buffer is closed
New buffer is opened

Also adds an integration test that validates the behavior of the "rename file to match type" refactoring: * File is renamed on disk * Old buffer is closed * New buffer is opened

rchande · 2017-11-10T18:18:25Z

Omnisharp side: OmniSharp/omnisharp-roslyn#1023

rchande · 2017-11-10T18:22:33Z

cc @akshita31

DustinCampbell

Looks good, though I'm betting you'll have further updates to react to changes on the omnisharp-roslyn side.

DustinCampbell · 2017-11-10T19:12:04Z

src/features/codeActionProvider.ts

-                    edit.set(uri, edits);
+                    if (change.ModificationType == FileModificationType.Modified)
+                    {
+                        let uri = vscode.Uri.file(change.FileName);


Based on your comment when FileModificationType.Renamed is true, should we check to see if this file is included in renamedFiles? And if so, does that change the order in which we loop through the changes?

Right. We should probably collect all the renames first, so we don't try to edit files that got renamed.

DustinCampbell · 2017-11-10T19:12:52Z

src/features/codeActionProvider.ts

+                // Unfortunately, the textEditor.Close() API has been deprecated
+                // and replaced with a command that can only close the active editor.
+                // If files were renamed that weren't the active editor, their tabs will
+                // be left open and marked as "deleted" by VS Code


Do we have a bug on VS Code about this?

Filed microsoft/vscode#38239

DustinCampbell · 2017-11-10T19:14:22Z

test/providertest.ts

@@ -0,0 +1,12 @@
+import { should } from 'chai';


What is this file? Did it get added accidentally?

…into rename

DustinCampbell · 2017-11-13T17:47:44Z

OK. CI should properly fail for this now, since this PR requires a changes in omnisharp-roslyn

TheRealPiotrP · 2017-11-15T01:33:46Z

test/integrationTests/codeActionRename.integration.test.ts

+            await csharpExtension.activate(); 
+        }
+
+        testAssetWorkspace.deleteBuildArtifacts();


do you need to delete build artifacts?

Copypasta, will remove.

TheRealPiotrP · 2017-11-15T01:34:00Z

test/integrationTests/codeActionRename.integration.test.ts

+
+        testAssetWorkspace.deleteBuildArtifacts();
+
+        await fs.rimraf(testAssetWorkspace.vsCodeDirectoryPath);


do you need to delete the .vscode directory?

Copypasta, will remove.

rchande · 2017-11-15T23:55:05Z

@DustinCampbell Do these tests automatically consume the latest bits from omnisharp?

TheRealPiotrP · 2017-11-16T03:08:40Z

test/integrationTests/codeActionRename.integration.test.ts

+    });
+
+    test("Code actions can remame and open files", async () => {                
+        let fileUri = await testAssetWorkspace.projects[0].addFileWithContents("test.cs", "class C {}");


running this test more than once will give us false positives. The first execution will put C.cs in the workspace, the second will simply find it.

My first thought was to add a line that deletes both test.cs and C.cs... but this feels like we will have lots of this sort of cleanup code running around. What if we added a method on e.g. testAssetWorkspace that performed a git clean -xfd? This would allow tests to easily put the asset back into a known state [we could eliminate the custom methods for deleting bin/obj, as well as for .vscode]. Thoughts?

I somewhat dislike having the projects manipulated by tests under the repo because they show up in my git status, so it would be nice if we could do something to get them out of there.

Relatedly, Omnisharp-roslyn has a pretty nice system where you can request a real on-disk project from a test helper, and one is created for you under a random folder under %temp%. If we implemented that, we wouldn't have to worry about cleaning up test projects after we're done testing because a test would never see the same instance of a project.

O#r's build knows about the test projects so it can build/restore them as part of the solution build. That means deploying a project for test simply copies all the files into a temp directory.

I think that might be a good solution for these tests. What do you think?

Agreed 100%. This is why the tests in omnisharp-roslyn that operate on test projects go to an effort to copy test projects to a temporary folder and delete them when the test is finished.

CLI does the same, @DustinCampbell. However, in this scenario we have a non-trivial startup cost for opening a new folder in VS Code. We also don't have a known model for plugging in an outer loop test orchestrator. Such a thing would need to enumerate all the tests, understand which test asset is needed by which test, make a copy of the asset & launch VS Code in that copy's folder, and then get vscode to execute just that test. I agree that this would be cool to have [if we can make it fast] but it feels like a considerable amount of work...

TheRealPiotrP · 2017-11-16T03:11:42Z

@rchande the Code version gets pinned in a few places:
https://github.com/OmniSharp/omnisharp-vscode/search?utf8=%E2%9C%93&q=1.17.2&type=

We pinned it because 1.18 broke our integration tests. In general, I'd prefer we stick to this model so that we can deal with updates to VSCode separately from other feature work. @DustinCampbell got blocked on a feature recently, for example, because 1.18 happened to ship while he was working on his PR.

TheRealPiotrP · 2017-11-16T03:14:31Z

test/integrationTests/codeActionRename.integration.test.ts

+        let fileUri = await testAssetWorkspace.projects[0].addFileWithContents("test.cs", "class C {}");
+        await vscode.commands.executeCommand("vscode.open", fileUri);
+        let c = await vscode.commands.executeCommand("vscode.executeCodeActionProvider", fileUri, new vscode.Range(0, 7, 0, 7)) as {command: string, arguments: string[]}[];
+        expect(c.length).to.equal(3);


3 seems like a magic number. Can we provide some text [expect should take a second parameter that is used to create the failure output string] that tells us why 3 is the right answer?

TheRealPiotrP · 2017-11-16T03:15:35Z

test/integrationTests/codeActionRename.integration.test.ts

+        let c = await vscode.commands.executeCommand("vscode.executeCodeActionProvider", fileUri, new vscode.Range(0, 7, 0, 7)) as {command: string, arguments: string[]}[];
+        expect(c.length).to.equal(3);
+        await vscode.commands.executeCommand(c[1].command, ...c[1].arguments)
+        expect(vscode.window.activeTextEditor.document.fileName).contains("C.cs");


here as well... this might not be obvious... "Because the active file was renamed and its new name should be reflected" or something?

TheRealPiotrP · 2017-11-16T03:16:31Z

I suspect tests are failing because 3 is the right magic context but not in the other [sln vs. csproj]. Do we need to run this test in both contexts? Perhaps we should just limit it to the csproj context?

DustinCampbell · 2017-11-16T14:30:37Z

@rchande:

@DustinCampbell Do these tests automatically consume the latest bits from omnisharp?

Only once C# for VS Code is updated with new bits from OmniSharp. A release from omnisharp-roslyn has not yet been built with your changes. Once that happens, C# for VS Code will need to be updated with them (after using the tool you were building for uploading them to the CDNs).

rchande · 2017-11-16T16:36:20Z

@TheRealPiotrP I think tests are failing because as, @DustinCampbell mentioned, we haven't consumed the new omnisharp bits that would provide the behavior being tested. That said, I will incorporate the changes to the test that you suggested.

DustinCampbell · 2017-11-16T16:43:59Z

FWIW, I did state that very fact above:

OK. CI should properly fail for this now, since this PR requires a changes in omnisharp-roslyn

david-driscoll · 2017-11-16T16:51:07Z

We could deploy the latest omnisharp bits to a cdn / azure storage / myget / and have it act as a beta feed.

Handle Rename and OpenDocument responses

6c33b49

Also adds an integration test that validates the behavior of the "rename file to match type" refactoring: * File is renamed on disk * Old buffer is closed * New buffer is opened

rchande requested review from DustinCampbell, david-driscoll and TheRealPiotrP November 10, 2017 18:18

dnfclas added the cla-already-signed label Nov 10, 2017

rchande mentioned this pull request Nov 10, 2017

Support rename/openDocument operations from CodeActions OmniSharp/omnisharp-roslyn#1023

Merged

DustinCampbell approved these changes Nov 10, 2017

View reviewed changes

DustinCampbell and others added 3 commits November 13, 2017 09:39

Merge branch 'master' into rename

ae0b609

Consume protocol changes

c7f588b

Merge branch 'rename' of https://github.com/rchande/omnisharp-vscode …

5641128

…into rename

Remove extra file

95cf274

TheRealPiotrP reviewed Nov 15, 2017

View reviewed changes

Ravi Chande added 2 commits November 15, 2017 09:03

Remove unnecessary code

1afc674

Merge branch 'master' into rename

578d676

TheRealPiotrP reviewed Nov 16, 2017

View reviewed changes

Merge branch 'master' into rename

e7ed150

ghost removed the cla-already-signed label Dec 7, 2017

DustinCampbell added the Blocked on OmniSharp label Dec 12, 2017

Merge branch 'master' into rename

ddbbe0a

rchande mentioned this pull request Jan 24, 2018

Attempt to fix rename file same as type issue. OmniSharp/omnisharp-roslyn#1095

Closed

Ravi Chande added 2 commits January 24, 2018 10:01

Fix build errors

2c6b94a

fix test

10f3b47

rchande merged commit 3633e82 into dotnet:master Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Rename and OpenDocument responses from Omnisharp #1854

Handle Rename and OpenDocument responses from Omnisharp #1854

rchande commented Nov 10, 2017

rchande commented Nov 10, 2017

rchande commented Nov 10, 2017

DustinCampbell left a comment

DustinCampbell Nov 10, 2017

rchande Nov 13, 2017

DustinCampbell Nov 10, 2017

rchande Nov 13, 2017

DustinCampbell Nov 10, 2017

DustinCampbell commented Nov 13, 2017

TheRealPiotrP Nov 15, 2017

rchande Nov 15, 2017

TheRealPiotrP Nov 15, 2017

rchande Nov 15, 2017

rchande commented Nov 15, 2017

TheRealPiotrP Nov 16, 2017

rchande Nov 16, 2017

DustinCampbell Nov 16, 2017

TheRealPiotrP Nov 18, 2017

TheRealPiotrP commented Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP Nov 16, 2017

TheRealPiotrP commented Nov 16, 2017

DustinCampbell commented Nov 16, 2017 •

edited

Loading

rchande commented Nov 16, 2017

DustinCampbell commented Nov 16, 2017

david-driscoll commented Nov 16, 2017


		testAssetWorkspace.deleteBuildArtifacts();

		await fs.rimraf(testAssetWorkspace.vsCodeDirectoryPath);

Handle Rename and OpenDocument responses from Omnisharp #1854

Handle Rename and OpenDocument responses from Omnisharp #1854

Conversation

rchande commented Nov 10, 2017

rchande commented Nov 10, 2017

rchande commented Nov 10, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealPiotrP commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealPiotrP commented Nov 16, 2017

DustinCampbell commented Nov 16, 2017 • edited Loading

rchande commented Nov 16, 2017

DustinCampbell commented Nov 16, 2017

david-driscoll commented Nov 16, 2017

DustinCampbell commented Nov 16, 2017 •

edited

Loading