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

codelens: a way to use selected launch.json configuration in run test | debug test #855

Open
polinasok opened this issue Oct 28, 2020 · 8 comments
Labels
debug/config Issues for config discrepancies b/n settings.json, launch.json, launch with F5, run test, debug test Debug Issues related to the debugging functionality of the extension.

Comments

@polinasok
Copy link
Contributor

polinasok commented Oct 28, 2020

Users complain that the handy run test | debug test links above their test functions ignore the flags they set in their launch configuration (example, example, example). They can fallback on settings.json using go.testFlags, go.delveConfig, etc, but that is not obvious or intuitive and has surprised users with additional limitations for debug test vs run test (e.g. #1636, microsoft/vscode-go#2894 (comment), microsoft/vscode-go#2115 (comment)).

According to @ramya-rao-a, a while back there was no way to feed current selected launch configuration into the codelens, so another set of dlv-related settings was supported as an alternative. Since then a new mechanism to support this could have been introduced, so we should investigate what is possible now.

Below is a quick way to reproduce the current behavior:

Test function:

func TestA(t *testing.T) {
	log.Println("TestA running")
	if !testing.Verbose() {
		t.Fatal()
	}
}

Selected launch configuration

        {
            "name": "Launch test function",
            "type": "go",
            "request": "launch",
            "mode": "test",
            "program": "${workspaceFolder}",
            "args": [
                "-test.run", "TestA", // no impact with or without on `run/debug test`, which adds its own  `-run ^TestA$`
                "-test.v"
            ]
        },

Debug with ▷ Start Debugging or Run > Start Debugging (F5)

Uses launch.json configuration, so the test passes.
image

Debug with debug test

Doesn't use launch.json configuration, so the test fails.
image

Run with Run > Run Without Debugging (^F5)

Uses launch.json configuration, so the test passes. (Note that this actually goes through the debug adapter, which launches dlv - see #336)
image

Run with run test

Doesn't use launch.json configuration, so the test fails. (Note that this uses go test and bypasses dlv - see #336)
image

Adding flags to settings.json

    "go.testFlags": ["-test.v"]

run test now passes, but debug test behavior is unchanged.

    "go.testFlags": ["-args","-test.v"]

run test passes, but only prints "ok", no details. debug test works as expected.

    "go.testFlags": ["-v", "-args","-test.v"]

Combining these makes both work as expected - pass and print verbose details.

@polinasok polinasok added the Debug Issues related to the debugging functionality of the extension. label Oct 28, 2020
@hyangah
Copy link
Contributor

hyangah commented Oct 28, 2020

Thanks @polinasok for putting up all the details.
The tricky part in the presented example is that the config in the launch.json is not really compatible with general debug test execution. I think we can consider

  1. allow users to select one of the launch configurations and a default one, and
  2. plumb "go.testFlags"when mode is test so users don't need to write launch.json like the example at all.

@polinasok
Copy link
Contributor Author

What do you mean by "not really compatible"?
Do you mean "as the default one"? I am not sure what it would mean to pick two launch configurations.
I think many users want to use launch.json and might not even know about go.testFlags.

@hyangah
Copy link
Contributor

hyangah commented Oct 28, 2020

@polinasok The launch configuration has the following -

            "args": [
                "-test.run", 
                "TestA",
                "-test.v"
            ]

This is not suitable for general 'debug test' codelens launch configuration - the main purpose of debug test codelen is to automatically determine the -test.run flag value.

Users may have multiple launch configurations in the launch.json - for example, I have launch configurations to run my binaries no matter what files are open. That's not suitable for the use with the debug test codelens or even a regular testing. We can choose to present all we found from the launch.json and the default one and let users to choose.

I sent multiple users to the launch.json path and let them specify the args there in addition to go.testFlags. :-)

@polinasok
Copy link
Contributor Author

I see what you mean. I included -test.run to match the individual function setup from run/debug test for demonstration purposes. I figured one might wonder if they should match the choice from run/debug test for this to work, without worrying about the hidden implementation details. But it does not work either way. If the two configurations had been combined as-is, we would have gotten an error like go test: run flag may be set only once, which we do not. I added a comment to highlight this above.

Interestingly, adding ["-test.run", "TestA"] to go.testFlags does lead to an error with run test and run file tests while adding ["-run", "TestA"] is a no-op for both. And adding either of those works with run package tests, which doesn't use its own -run flag by default.

If we do support this feature, it is up to us how to combine the two sources of -test.run, isn't it? We could combine everything as-is and channel the duplication errors or apply some clever logic to make sense of the choices and avoid unnecessary errors (e.g. expect them to match, or one be a superset of the other, or expect that launch configuration has none specified, or always ignore the one specified in launch configuration favor of what run/debug test or run file tests adds). Moreover, we need to sanity check mode and the program in the launch configuration as well. A user might accidentally select something not related to their test at all, in which case we could just fallback to the default configuration.

Whatever we do, we just need to make sure it is documented and also flagged via some kind of warnings/logging, so if the behavior is unexpected, the user can get an explanation without having to search forums or file issues.

@polinasok
Copy link
Contributor Author

Related issue: #128 The many ways to specify build/test flags/tags that sometimes override each other and sometimes are ignored is causing issues in all directions. In the short-term, whatever the behavior is we need to be very careful about documenting it (#860).

In the long-term, we have the option of revisiting this alltogether with dlv-dap. New adapter, new flags. Should we? If according to microsoft/vscode-go#3185 (comment), the options in launch.json were added as a workaround, should they be dropped now that we can channel the settings from the extension? On the other hand, many users are now relying on launch.json as a centralized place to keep all their configurations. It's direct mapping to launch/attach request args is very convenient.

@hyangah
Copy link
Contributor

hyangah commented Aug 11, 2021

One disadvantage of launch.json is users have to configure this per workspace, unlike settings.json that can be defined for user profile and apply to all projects/workspaces.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/366915 mentions this issue: docs/debugging: include logpoints, remove stop conditions, revise faqs

gopherbot pushed a commit that referenced this issue Dec 7, 2021
They are all fixed in the latest dlv version.

Updates #1676
Updates #123
Updates #855
Updates #1840

Change-Id: I6014a1382396865a1921eb6c92e1ccccdea556de
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/366915
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Polina Sokolova <polina@google.com>
@thediveo
Copy link
Contributor

codelens also doesn't support the new 'asRoot' launch option that allows debugging and testing launch configurations to run as root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug/config Issues for config discrepancies b/n settings.json, launch.json, launch with F5, run test, debug test Debug Issues related to the debugging functionality of the extension.
Projects
None yet
Development

No branches or pull requests

4 participants