Implement better way to test for in-memory file #645
Conversation
Fix #569 PSES would crash quite often when the user viewed a ps1 file in a diff window or an earlier version using GitLens. This approach using the System.Uri class that can detect file vs other schemes. URI chokes on relative file paths. In this case, we fall back to the previous approach of using Path.GetFullPath().
|
@tylerl0706 Can you kick a new build? The |
|
Does this fix also help us fight the "path too long" exception that we were seeing with some of the git diff windows? |
|
If it's possible to write tests for this, I'd really appreciate it. I can see 3 additional test cases already:
|
|
Yes it should fix the non-file system uri scheme PTLEs. Currently what it doesn't handle is file system PTLE but I'm not sure we'd get this far in that case. FS path len limits are at the OS level so in theory VSCode would choke first on these paths. That said, maybe there's a diff in long path between node and .NET. I'll add some unit tests. |
|
Awesome!! Yeah I think you're right. VSCode would choke before it's a problem for us. This is going to close like 3-4 issues. Nice work 😄 |
| // Test long non-file path - known to have crashed PSES | ||
| var longUriForm = "gitlens-git:c%3A%5CUsers%5CKeith%5CGitHub%5Cdahlbyk%5Cposh-git%5Csrc%5CPoshGitTypes%3Ae0022701.ps1?%7B%22fileName%22%3A%22src%2FPoshGitTypes.ps1%22%2C%22repoPath%22%3A%22c%3A%2FUsers%2FKeith%2FGitHub%2Fdahlbyk%2Fposh-git%22%2C%22sha%22%3A%22e0022701fa12e0bc22d0458673d6443c942b974a%22%7D"; | ||
| Assert.True(Workspace.IsPathInMemory(longUriForm)); | ||
| } |
There was a problem hiding this comment.
Do you think you could refactor this test to be an array and an assert?
Something like:
var testCases = new[]{
{ Path = "foo.ps1", IsInMemory = false },
{ Path = Path.Combine(tempDir, "GitHub", "PowerShellEditorServices"), IsInMemory = false },
{ Path = new Uri(shortDirPath).ToString(), IsInMemory = false }
}
foreach (var testCase in testCases) {
Assert.IsEqual(Worspace.IsPathInMemory(testCase.Path), testCase.IsInMemory);
}I think it's a little cleaner and involves writing less Asserts.
There was a problem hiding this comment.
I can make that change (have actually) but when a test fails, we only get this:
Message: Assert.Equal() Failure
Expected: False
Actual: True
And the line number with no indication of which test case failed. I could do a Debug.WriteLine of the Path being tested. Thoughts?
There was a problem hiding this comment.
Think I have a solution to that last issue - found an Assert method that allows me to specify a message to emit when the assert fails.
| // File system absoulute paths will have a URI scheme of file:. | ||
| // Other schemes like "untitled:" and "gitlens-git:" will return false for IsFile. | ||
| var uri = new Uri(filePath); | ||
| isInMemory = !uri.IsFile; |
There was a problem hiding this comment.
nit: maybe combine these into 1 line since uri isn't used? I don't feel strongly either way but it's something I thought about.
There was a problem hiding this comment.
Having uri as a variable makes it easier to inspect the entire URI when debugging - not just the resulting string.
|
Loving the test now 😍 LGTM 👍 |
Fix #569
PSES would crash quite often when the user viewed a ps1 file in a diff window
or an earlier version using GitLens.
This approach using the System.Uri class that can detect file vs other schemes.
URI chokes on relative file paths. In this case, we fall back to the previous
approach of using Path.GetFullPath().