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

Add integration tests for VS Code debugging, eliminate FileAccessor #1107

Merged
merged 12 commits into from
Mar 12, 2024

Conversation

minestarks
Copy link
Member

@minestarks minestarks commented Feb 2, 2024

Depends on #1106

VSCode's debugging related APIs sometimes use filesystem paths instead of URIs to represent sources. Previously, our solution was to have a FileAccessor which tried to treat the path both ways (first as a filesystem path, then a URI). But by being strategic about exactly where we expect filesystem paths and sanitizing them, we can just consistently use URIs in the rest of the code.

Additionally this PR introduces integration tests for the debugger.

We also fix two small-ish bugs here:

  • launch.json variables like {file} and {workspaceFolder} weren't treated properly on all platforms/filesystems
  • when launching the debugger on an untitled file, the user may be prompted to save the file. If they choose to save the file, the debugger will fail to launch. (This is probably not as big an issue anymore, since we disabled the auto-save behavior by default since I've opened this PR.)

@minestarks minestarks marked this pull request as ready for review February 2, 2024 23:52
@billti
Copy link
Member

billti commented Feb 3, 2024

I wish I could remember the issues we hit (maybe @idavis does) but we hit repeated problems with this code when implementing the debugger and when to use strings vs Uris. (I was hoping we'd not touch it again, else we should have commented it with the details).

The biggest challenge I recall was specifically with the GitHub VFS/scheme, so would only occur when you'd open the editor on github.dev, for which I needed to deploy a test extension to be able to test and debug.

If I vaguely recall the root cause, it was that at some point in the infra it marshals one of the values via JSON, which for a string works fine, but for a Uri converting it to JSON and back loses the Uri methods (as the Uri base class is not on the prototype chain anymore). However the GitHub file system would detect that one of the Uri properties is present (e.g. 'scheme'), assume IT IS a Uri, and then try and call Uri methods on it (like .from() or .with()) which would crash with the expected TypeError: from is not a function.

Either way, if touching this stuff again, be sure to REALLY test it, included when it's running the browser as an extension working with the GitHub virtual file system on github.dev

@idavis
Copy link
Collaborator

idavis commented Feb 7, 2024

The issue was around paths coming in from VS being windows paths instead of URIs. Normally we'd be able to tell the underlying debug session that we want paths always as URIs, but it is broken in web extensions. See this issue: microsoft/vscode-debugadapter-node#298 .

I haven't dug into this PR, but when the call to createDebugAdapterDescriptor is made, we sanitize the input to get it as a URI before feeding it into the project resolver. This way we never read the value config.program as the project resolver was fed the sanitized URI and loaded all related source.

We also use resolvePathToUri to sanitize paths coming in for breakpointLocationsRequest, setBreakPointsRequest, and stackTraceRequest callbacks.

I've left out some historical detail as the project loader changed how the initialization of the debug session is performed.

@minestarks
Copy link
Member Author

minestarks commented Feb 7, 2024

@billti @idavis yeah I'm in no rush to check this in without extensive testing, since so far every environment I tried had a unique behavior which made me revise my assumptions... like even Windows browser behaves differently from Linux browser, even when you're using a virtual filesystem - what the heck.

I have this test matrix which I'm manually going through but I can't test github.dev / vscode.dev without shipping the dev build, I think. But please let me know if there's another way!

Test:

Launch configs:

  • launch.json ${file}
  • launch.json ${workspaceFolder}
  • launch.json no program
  • debug command
  • debug command on untitled buffer, saving
  • debug command on untitled buffer, without saving

Actions:

  • Set and run until breakpoints in main file
  • step in/out of stdlib
  • Set and run until breakpoints in stdlib
  • Inspect call stacks
  • (Multi-file only)
    • step in/out of child files, if any
    • Set and run until breakpoints in child files

VSCode environments:

  • desktop, local folder
  • desktop, wsl
  • desktop, playground (qsharp-vfs://)
  • locally hosted vscode web (@vscode/test-web), test fs (vscode-test-web://)
  • vscode.dev, playground (qsharp-vfs://)
  • vscode.dev, local folder
  • github.dev, local folder

Platforms:

  • Windows
  • Linux
  • Mac

github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2024
A `Location` refers to a span within a specific source file. It's
expressed as a source name and `Range`. See:
https://github.com/microsoft/vscode/blob/7923151da4b3567636c1346df9e8ba682744bbf3/src/vscode-dts/vscode.d.ts#L6581

It was defined in the language service protocol, but I'm moving it to a
more common location in `compiler/qsc` since I need the debugger to use
it in #1107.

To summarize this PR and #1038, here's how locations are represented in
the compiler vs. higher-level components that interface with VS Code
(debugger, language service):
 
| Compiler representation | Line/column representation |
|---|---|
| `u32` offset (into `SourceMap`) | `Position` (into specific source
file) |
| `Span` | `Range` |
| (`PackageId`, `Span`) | `Location` |
Base automatically changed from minestarks/location to main February 7, 2024 18:44
Copy link

Benchmark for e15caad

Click to view benchmark
Test Base PR %
Array append evaluation 777.2±19.08µs 824.3±4.60µs +6.06%
Array literal evaluation 543.6±15.11µs 505.5±16.55µs -7.01%
Array update evaluation 768.9±8.08µs 819.2±6.95µs +6.54%
Deutsch-Jozsa evaluation 7.0±0.05ms 7.3±0.18ms +4.29%
Large file parity evaluation 35.2±0.15ms 35.5±0.17ms +0.85%
Large input file 40.7±1.47ms 40.1±2.05ms -1.47%
Large nested iteration 77.6±0.65ms 81.5±0.96ms +5.03%
Standard library 22.7±0.55ms 23.2±0.75ms +2.20%
Teleport evaluation 96.2±1.90µs 100.0±3.24µs +3.95%

@minestarks minestarks enabled auto-merge March 11, 2024 23:33
Copy link

Benchmark for 70c23d8

Click to view benchmark
Test Base PR %
Array append evaluation 760.3±4.41µs 758.2±19.88µs -0.28%
Array literal evaluation 500.0±8.61µs 517.4±37.09µs +3.48%
Array update evaluation 756.4±23.39µs 752.3±5.42µs -0.54%
Deutsch-Jozsa evaluation 7.1±0.05ms 7.1±0.06ms 0.00%
Large file parity evaluation 35.2±0.08ms 35.1±0.10ms -0.28%
Large input file 41.5±1.07ms 40.8±1.05ms -1.69%
Large nested iteration 77.6±0.91ms 76.7±0.92ms -1.16%
Standard library 23.4±0.70ms 23.0±0.56ms -1.71%
Teleport evaluation 94.8±3.51µs 96.0±1.95µs +1.27%

@minestarks minestarks added this pull request to the merge queue Mar 11, 2024
Merged via the queue into main with commit 19fcae9 Mar 12, 2024
14 checks passed
@minestarks minestarks deleted the minestarks/eliminate-file-accessor branch March 12, 2024 00:16
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