Fix unable to open files in problems/peek windows issue

[rkeithhill]

This is due to PSES not properly storing a language client path in the
ClientFilePath property of ScriptFile. This change ensures that a
ClientFilePath is always stored in the text document Uri that a LSP
client expects. This code has mostly been provided by
@SeeminglyScience. Thanks for the reference to your code.
This fixes issue 1732 in the vscode-powershell repo.

BTW I've also verified that there are issues when you use the reference code lens where refs to other files that you haven't loaded, will not load when you click on the file in the peek window.

[rkeithhill]

Well, looks like that bombs a lot of tests. I'll take a look at the tests tomorrow and see if this is a case where we can "fix" the tests.

[rkeithhill]

OK, the latest update leaves only one (non-test) use of ClientFilePath and that happens starting here:

PowerShellEditorServices/src/PowerShellEditorServices/Session/RemoteFileManager.cs

Line 596 in 1a86d4b

context?.CurrentFile.InsertText(Encoding.UTF8.GetString(fileContent, 0, fileContent.Length));

which calls (and uses ClientPath):

PowerShellEditorServices/src/PowerShellEditorServices/Extensions/FileContext.cs

Line 240 in 1a86d4b

.InsertTextAsync(this.scriptFile.ClientFilePath, textToInsert, insertRange)

which sends this insert test request to the LSP client:

PowerShellEditorServices/src/PowerShellEditorServices.Protocol/Server/LanguageServerEditorOperations.cs

Line 46 in 1a86d4b

FilePath = filePath,

Not sure if we could have a latent bug here where we pass a path that hasn't been converted to a DocumentUri. So I've left this one alone. I think it is used when you edit a remote file. @TylerLeonhardt ?

[rkeithhill]

OK, I think this is ready for review again.

[SeeminglyScience]

LGTM! Just a couple suggestions

[TylerLeonhardt]

This is great @rkeithhill! A couple things...

Can you resolve the merge conflict?

Is it possible to add tests for this change? (I only ask because I think this part of the code is very testable)

[rkeithhill]

Resovled the merge conflicts and addresses other PR comments. I'll look into tests next.

[rkeithhill]

BTW it would be nice if running tests locally was more convenient and more reliable. For instance, when I run the tests from the Visual Studio Test Explorer window, I get 55 failures, yet I get zero failures fom the command line. That is, when the tests don't hang. Often when running from the command line, the tests hang. :-(

[rjmholt]

BTW it would be nice if running tests locally was more convenient and more reliable. For instance, when I run the tests from the Visual Studio Test Explorer window, I get 55 failures, yet I get zero failures fom the command line. That is, when the tests don't hang. Often when running from the command line, the tests hang. :-(

Yeah testing is what I want to focus on when I next get the chance. Once we have better testing we can make the changes we so badly need to make without breaking everything else

[rkeithhill]

@TylerLeonhardt Good call on adding tests. I think I've found an issue but I need to spin up my Ubuntu VM to verify.

[TylerLeonhardt]

Let me know if there's anything I can do to help!

[TylerLeonhardt]

Commit: Try to fix tests for Linux ✔️

Nice! Do you think this is ready for review now?

[rkeithhill]

Yes. It is ready for review. Also could you test this on macOS or Linux? In the released version of the extension, open the examples folder. Insert a few lines of script into StopTest.ps1 that will generate PSSA warnings (use iex or unused variable). The save & close the file. Now reload VSCode and open Stop-Process2.ps1. After PS initializes, click on the references on the Stop-Process2 function. While the reference is being displayed in the peek window, you should see a problem appear in the Problems window for the script you added. Double click on the problem to navigate to it. That is when you should see this:

Now build VSCode with this PR pulled down in PSES and try the above experiment again and it should work this time.

BTW sometimes I've seen references fail before when you try to navigate to a file. Still trying to find a repro for that one.

[TylerLeonhardt]

Before:

After 😊

And when I click on them, they navigate successfully!

Awesome work!

[rkeithhill]

Thanks. So you are seeing the extension dev host use the ISE color theme as well. Hmm... I wonder what is up with that.

[rkeithhill]

@SeeminglyScience Would you mind taking a peek at this again? I changed some of the logic and would like to get you to review it. The main thing I found is that Linux allows file names like foo:bar:baz.txt. That and foo\bar:baz.txt which VSCode chokes on.

[SeeminglyScience]

LGTM! One nit and some possible optimization. The string allocs are nbd though, shouldn't block if they're an issue.

[rkeithhill]

To quote Lego Batman - "What the heck!?"

Build started
2
git clone -q --depth=10 https://github.com/PowerShell/PowerShellEditorServices.git C:\projects\powershelleditorservices
3
fatal: unable to access 'https://github.com/PowerShell/PowerShellEditorServices.git/': Could not resolve host: github.com
4
Command exited with code 128

How do you go about kicking a new build without having to push an unnecessary commit?

[rkeithhill]

OK, just about ready to merge this. @TylerLeonhardt could you run that test one more time since we changed the Linux/macOS code path a bit (based on @SeeminglyScience feedback)? Thanks.

[rkeithhill]

Can someone rekick the build?

[TylerLeonhardt]

LGTM with a not important nit. 😄

[TylerLeonhardt]

those flaky tests are so frustrating but I haven't had time to fix/mitigate them... I'm thinking about implementing 5x retry logic.

[TylerLeonhardt]

@SeeminglyScience @rjmholt if you guys wanna give this one more look after my comments... otherwise I think this is good to go in

[TylerLeonhardt]

p.s. this PR still works on macOS 😄

[SeeminglyScience]

[TylerLeonhardt]

@rjmholt is OOF, merging this in. I marked it for backport. @rkeithhill we should cherry pick this back and test it.