Track virtual documents. #2436
Track virtual documents. #2436
Conversation
- Track opening, closing and changing of virtual documents that don't exist on disk. - Updated the various document selectors to properly include virtual files. dotnet#2431
| @@ -81,6 +81,7 @@ export async function activate(context: vscode.ExtensionContext, packageJSON: an | |||
| localDisposables.add(vscode.languages.registerCodeActionsProvider(documentSelector, codeActionProvider)); | |||
| localDisposables.add(reportDiagnostics(server, advisor)); | |||
| localDisposables.add(forwardChanges(server)); | |||
There was a problem hiding this comment.
Based a lot of these changes off of the forwardChanges handler and I didn't see any tests. What's the test guidance to test my virtual document tracker in this repo?
There was a problem hiding this comment.
We don't yet have a strong unit testing story for this, but it is possible to write integration tests -- take a look at the integrationTests folder. Not sure if there's an easy way to create virtual documents programmatically without installing another extension though. The "code action rename" test covers this type of scenario.
There was a problem hiding this comment.
Thanks for the PR. Generally looks pretty good with a couple of small nits. Also I'm sure @akshita31 will have guidance on the best way to do logging.
| let req = { FileName: document.uri.path, changeType: FileChangeType.Create }; | ||
| serverUtils.filesChanged(server, [req]) | ||
| .catch(err => { | ||
| console.warn(`[o] failed to forward virtual document change event for ${document.uri.path}`, err); |
There was a problem hiding this comment.
@akshita31 Can you weigh in on the best way to do this logging?
There was a problem hiding this comment.
Yes, we should use the C# channel to do the logging here.
@NTaylorMullen The way logging works in the extension , is that we post a loggingEvent to an [https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/EventStream.ts](event stream), to which many observers have susbsribed. When we post the event to the event stream, all the observers that have subscribed to the stream, perform the necessary action. For this case, we will have to add the appropriate logging event and make the C# logger observer listen to it. I am not sure if the C# channel observer , should care of these events and show the channel. I will leave that up to you.
There was a problem hiding this comment.
@akshita31 How does this component get access to the eventStream?
There was a problem hiding this comment.
We can pass it down from the extension.ts - https://github.com/OmniSharp/omnisharp-vscode/pull/2436/files#diff-f49a39a20dc9807208c37823c89847a6R84
|
|
||
| function openVirtualDocument(document: TextDocument, server: OmniSharpServer) { | ||
| let req = { FileName: document.uri.path, changeType: FileChangeType.Create }; | ||
| serverUtils.filesChanged(server, [req]) |
There was a problem hiding this comment.
filesChanged and updateBuffer are both async/promise returning and should be awaited
There was a problem hiding this comment.
I thought so as well but that's not done in the changeForwarding class. Was trying to be consistent. Is it a mistake that it's not awaited there?
There was a problem hiding this comment.
In theory, maybe it doesn't matter because your requests get put into the queue in the submission order.
There was a problem hiding this comment.
@NTaylorMullen Recently we are trying to move to using async/await rather than .then/catch for promises. The changeForwarding still has the old pattern, but for this class we should prefer try/catch and async/await
| @@ -81,6 +81,7 @@ export async function activate(context: vscode.ExtensionContext, packageJSON: an | |||
| localDisposables.add(vscode.languages.registerCodeActionsProvider(documentSelector, codeActionProvider)); | |||
| localDisposables.add(reportDiagnostics(server, advisor)); | |||
| localDisposables.add(forwardChanges(server)); | |||
There was a problem hiding this comment.
We don't yet have a strong unit testing story for this, but it is possible to write integration tests -- take a look at the integrationTests folder. Not sure if there's an easy way to create virtual documents programmatically without installing another extension though. The "code action rename" test covers this type of scenario.
| import CompositeDisposable from '../CompositeDisposable'; | ||
|
|
||
| function trackCurrentVirtualDocuments(server: OmniSharpServer) { | ||
| let registration = server.onProjectAdded(() => { |
There was a problem hiding this comment.
This is going to add the existing virtual documents every time O# discovers a project, which can happen multiple times. There is no "done loading" event today, unfortunately.
There was a problem hiding this comment.
(We should test this in a multi-project + razor scenario and verify o# doesn't do anything weird if you add a buffer twice)
There was a problem hiding this comment.
This code is only ever called once for the first project. See at the end of the method where it disposes its own registration to the onProjectAdded.
- Moved bits to async await - Added integration test - Used event stream APIs
Codecov Report
@@ Coverage Diff @@
## master #2436 +/- ##
==========================================
+ Coverage 63.63% 64.35% +0.71%
==========================================
Files 88 89 +1
Lines 4007 4054 +47
Branches 567 573 +6
==========================================
+ Hits 2550 2609 +59
+ Misses 1294 1272 -22
- Partials 163 173 +10
Continue to review full report at Codecov.
|
|
🆙 📅 covered all feedback. Tried writing more tests to no avail sadly. Wanted to also verify the closing text document path but that's not supported by VSCode extension APIs (the IDE does it when it wants to). Also wanted to verify the "already opened" scenario but couldn't figure out how to do that because the Omnisharp server is booted once and run throughout all of the integration tests (need to have a virtual document created before server). |
|
Also, travis failure seems unrelated. |
|
ping |
|
@NTaylorMullen Travis failure sounds like it could be related, given that it's in the file change notification layer, which is what's being changed here. Kind of sounds like we're trying to send a file change notification before O# has started of after it has stopped. |
| } | ||
| } | ||
|
|
||
| registration.dispose(); |
There was a problem hiding this comment.
I'm not sure what the exact semantics of the NodeJS EventEmitter are. Is it possible for this handler to reenter? If so you have a race on disposing the registration/could process the virtual documents more than once, no?
There was a problem hiding this comment.
maybe the vscode semantics are different but in normal nodejs / JavaScript it's single threaded thus making races impossible.
There was a problem hiding this comment.
Ah, actually, the await yields execution. I can see how things can get invoked more than once unintentionally. Will move the registration dispose to the top to ensure that nothing wonky happens.
|
@NTaylorMullen This looks pretty good to me. If the Travis failure isn't obvious to you, we can requeue the build and see if it goes away. If you're interested in getting an "existing virtual document" tested, there are a couple of approaches that might work:
|
|
Thanks for the feedback, I think you were spot on in all accounts. I've addressed all the feedback, verified tests run locally (hopefully travis passes!).
Definitely concerned about doing something like that, sounds too extreme. It would muck with any sort of expectations around what documents are currently open in the editor for every other test and could lead to flakeyness in the requirement to restart omnisharp. If you're alright with it I'd rather pass. 🆙 📅 |
|
@NTaylorMullen Looks like we got the same build failure for your update... |
Ya I saw that too. I pushed another master branch clone and am seeing if travis fails on that. Those failures don't occur locally. |
|
@rchande ya same failure on the master branch (without my changes): https://travis-ci.org/OmniSharp/omnisharp-vscode/builds/410456563?utm_source=github_status&utm_medium=notification. Will that block the inclusion of this PR? |
|
@NTaylorMullen Yes, currently travis is a mandatory test for merging the pull request. I am investigating further into why the builds are failing. |
|
@NTaylorMullen Thanks for investigating, we'll take a look. |
|
@NTaylorMullen The CI issue has been fixed. Thank you for taking the time to contribute to the repo. |
#2431
/cc @DustinCampbell @rchande