feat(debugger): Add support for debugging tests (#5208) #7

anaPerezGhiglia · 2024-08-07T20:58:00Z

Description

Extend REPL and DAP debugger interfaces to support debugging a specific test function

Problem

for noir-lang#5208

Summary

modify dap to receive a test_function and abi as parameters to be able to notify the user if the test passed or failed after the debugging execution ended
modify debug repl command to support debugging a test
- add --test-name option
- the debug of multiple test functions is disabled
- the debug of test functions with arguments is disabled
modify repl interface to expose the execution result (witness_stack or NargoError) to be able to compare against the test_function definition
add new Debug test codelens action to test functions
create new map_execution_error function in nargo crate (errors): extract common behavior of mapping opcode resolution errors
create new build_workspace_file_manager function in nargo crate: extract common behavior of creating a new FileManager that has all the workspace files loaded into it

Additional Context

Here some commits that may be better to review isolated

Modify the debugger context to map the opcode resolution erros as done in the ProgramExecutor.execute_circuit (536a89f)
- for extracting the common behavior I had to separate the error mapping from the modification of the call_stack done in execute.rs

Pending

Move repl responsibility from test_cmd to debug_cmd to have repl and dap interfaces consistent, to ensure that in both cases it's the debugger responsibility
Relocate execution_helpers.rs functions and delete file

Out of scope of this PR - future issues

when debugging a test if the user clicks on "restart session" button in the IDE, the debugger starts the main function instead of the currently running test function

Documentation*

Check one:

No documentation needed.
Documentation included in this PR.
[For Experimental Features] Documentation to be submitted in a separate PR.

Associated changes in vs-code extension

compare commits

Evidence

Evidence of debugging simple tests and test with should_fail_with

Dap

Screen.Recording.2024-08-09.at.18.21.41.mov

REPL

Screen.Recording.2024-08-09.at.18.24.59.mov

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

we need this to return the error from the debugger as a NargoError for validatin the tests

- add TestFunction to dap - add Abi dependency to debugger crate

github-actions · 2024-08-07T20:58:13Z

Thank you for your contribution to the Noir language.

Please do not force push to this branch after the Noir team have started review of this PR. Doing so will only delay us merging your PR as we will need to start the review process from scratch.

Thanks for your understanding.

mverzilli

Looking great! I think given we're adding an important feature, we should also add the new feature/s to the documentation.

We should add adequate stuff to docs/docs/reference/debugger and docs/docs/how_to/debugger

tooling/debugger/src/dap.rs

tooling/nargo/src/lib.rs

now it only returns a FileManager

ggiraldez

I left a bunch of comments and suggestions. In general, I don't think this is good to merge. The logic for launching the debugger was already too complicated and I think this complicates it even further.

There are a couple of important asymmetries this PR introduces as well, which make following the code harder: they way the DAP and the REPL handle tests is different. The DAP knows it's debugging a test, while the REPL doesn't. The other I already pointed out in the comments: preparing main and preparing a test function are too different. Plus preparing for the DAP and for the REPL are different as well. While not ideal, the compilation/preparation code in dap_cmd seems to be a bit simpler. I understand much of this is a result of pre-existing spaghetti code, but I'd try to disentangle it first. There is also the nesting of the results in debug_cmd which is definitely a no-go.

We're probably missing some abstraction. One idea is a DebugTarget that can encapsulate the differences between debugging main and debugging a test.

ggiraldez · 2024-08-21T22:15:27Z

docs/docs/how_to/debugger/debugging_with_the_repl.md

+}
+```
+
+To start the REPL debugger for a test function, using a terminal, go to a Noir circuit's home directory. Then invoke the `debug` command setting the `--test-name` argument.


Suggested change

To start the REPL debugger for a test function, using a terminal, go to a Noir circuit's home directory. Then invoke the `debug` command setting the `--test-name` argument.

To debug a test function using the REPL debugger, navigate to a Noir project directory inside a terminal, and run the `nargo debug` command passing the `--test-name your_test_name_here` argument.

ggiraldez · 2024-08-21T22:17:06Z

docs/docs/how_to/debugger/debugging_with_the_repl.md

+
+### Test result
+
+The debugger does not end the session automatically. Once you finish debugging the execution of the test function you will notice that the debugger remain in the `Execution finished` state. If you are done debugging the test function you should exit the debugger by using the `quit` command. Once you finish the debugging session you should see the test result.


Suggested change

The debugger does not end the session automatically. Once you finish debugging the execution of the test function you will notice that the debugger remain in the `Execution finished` state. If you are done debugging the test function you should exit the debugger by using the `quit` command. Once you finish the debugging session you should see the test result.

The debugger does not end the session automatically. Once you finish debugging the execution of the test function you will notice that the debugger remains in the `Execution finished` state. When you are done debugging the test function you can exit the debugger by using the `quit` command. Once you finish the debugging session you should see the test result.

ggiraldez · 2024-08-21T22:18:30Z

docs/docs/how_to/debugger/debugging_with_vs_code.md


-You should see something like this:
+If you don't see the codelens options `Compile|Info|..|Debug` over the `main` function or `Run test| Debug test` over a test function then you probably have the codelens feature disabled. For enabling it head to the extension configuration and turn on the `Enable Code Lens` setting.


Suggested change

If you don't see the codelens options `Compile|Info|..|Debug` over the `main` function or `Run test| Debug test` over a test function then you probably have the codelens feature disabled. For enabling it head to the extension configuration and turn on the `Enable Code Lens` setting.

If you don't see the codelens options `Compile|Info|..|Debug` over the `main` function or `Run test| Debug test` over a test function then you probably have the codelens feature disabled. To enable it open the extension configuration page and check the `Enable Code Lens` setting.

ggiraldez · 2024-08-21T22:22:21Z

docs/docs/reference/debugger/debugger_repl.md

+| `--print-acir`                    | Display the ACIR for compiled circuit                                               |
+| `--deny-warnings`                 | Treat all warnings as errors                                                        |
+| `--silence-warnings`              | Suppress warnings                                                                   |
+| `--test-name` <TEST_NAME>         | The name of the test function to debug - which name contains this string            |


ggiraldez · 2024-08-21T22:24:49Z

tooling/debugger/src/context.rs

+    // TODO: validate comments
+    // The debugging session is over successfully
    Done,
+    // The session is active and we should continue with the execution
    Ok,
+    // Execution should be paused since we reached a Breakpoint
    BreakpointReached(DebugLocation),
+    // Session is over with an error


In all of these, it's probably desirable to use /// to use the comments as documentation.

ggiraldez · 2024-08-22T05:51:01Z