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

debug: should tests run through the debugger in "noDebug" mode? #336

Closed
eliben opened this issue Jul 13, 2020 · 6 comments
Closed

debug: should tests run through the debugger in "noDebug" mode? #336

eliben opened this issue Jul 13, 2020 · 6 comments
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge

Comments

@eliben
Copy link
Member

eliben commented Jul 13, 2020

The current DA implements noDebug mode ("Run without debugging" in the vscode IDE) by invoking the debuggee directly when in 'debug' mode, but going through Delve in 'test' mode.

Initially the new DAP DA will work the same way.

Is this the right thing to do? Generally speaking, the DA shouldn't invoke Delve at all in noDebug mode. Theoretically, users may not have Delve installed or something might be wrong with the Delve installation - this should not preclude users from running and testing programs without debugging in VSCode.

Another point of reference is that the "run test" codelens (GoRunTestCodeLensProvider) invokes go test directly without going through the debugger.

@hyangah
Copy link
Contributor

hyangah commented Jul 13, 2020

The more I think about it, the more I believe it should be part of DAP. It's part of the protocol spec.
It's not ideal that other editors have to implement the same on their own.

The codelens part of GoRunTestCodeLensProvider is to be replaced with LSP.

How to implement the actual test command still needs investigation. Currently the go extension is directly invoking go test command and implements the cancellation UI separately. In fact, LSP also offers an endpoint for go test execution for other editors. We are not using it. In my opinion, it would be better if this can be implemented in a consistent way that controls execution using the default 'Run without Debugging' UI (that comes with the cancellation UI, etc). But we are not there yet.

@polinasok
Copy link
Contributor

polinasok commented Jul 13, 2020

Is that what is used when you click "run test | debug test" that show up above every test in the source code?
What happens for "debug test"? That one must go through the DA. So "run test" and "Run without Debugging" when you have chosen a test launch.json configuration should ideally work similarly too.
Do these use some built-in configuration instead of whatever the user specifies in their launch.json?

@hyangah
Copy link
Contributor

hyangah commented Jul 13, 2020

When debug test is clicked, we start debugging using vscode.debug.startDebugging. So, that indicates we can also implement run test in a similar way with noDebug is set in theory. But that complicates other features like

  • presenting the test output in a separate output channel with decoration (not the 'Debug' tab).
  • postprocessing the test output.
  • interaction with code coverage.
  • running all tests in the repo, etc (that doesn't map well with launch.json model, right?)

@stamblerre @ianthehat @pjweinbgo also discussed about the possibility of running tests and build in gopls to extract info like code coverage, compiler optimization info.

Sad that there is no clear boundary we can draw.

@polinasok
Copy link
Contributor

Interesting development: by using or not using delve for noDebug with the same flags configuration we end up with different behavior - #1027 due to the differences in how delve and go parse/escape flags.

@polinasok
Copy link
Contributor

polinasok commented Feb 1, 2021

We should handle debug and test modes the same way. As explained in #1111, it is perfectly valid to handle them via dlv. And the advantage is consistent flag handling.

To provide "without debugging" experience via dlv, we have two options:

  1. add no-debug mode to dlv (No-debug mode (dlv-dap only) go-delve/delve#2318)
  2. use dlv as-is, but clear built-in breakpoints for panic and fatal-throw on start-up and skip setting any breakpoints that might get triggered by the UI (i.e. in SetBreakpoints, skip if this.delve.noDebug is set)

As for run test, I guess we could think of it as "Run", which is different from "Run without Debugging" (see #1111 for explanation). Maybe adding 'Run' as a third option to the drop-down UI for programs will make it easier to communicate that distinction?

@gopherbot gopherbot added this to the Untriaged milestone Apr 8, 2021
@hyangah hyangah modified the milestones: Untriaged, Backlog Apr 14, 2021
@hyangah
Copy link
Contributor

hyangah commented Jan 5, 2022

Ship has sailed and dlv dap has noDebug option. However, not all go test runs are implemented through dlv dap's noDebug mode because tests cannot always run through the debugger - for example, there is no way to efficiently implement go test ./... through delve. Now there is test explorer UI that also adds extra requirements and placing delve there complicates things.

@hyangah hyangah closed this as completed Jan 5, 2022
@golang golang locked and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants