go: Add runnables for Go #12003

anilsenay · 2024-05-17T22:39:32Z

Implemented runnables for specially for running tests for Go.

I'm grateful for your feedback because this is my first experience with Rust and Zed codebase.

Release Notes:

Added Runnables/Tasks for:
- Run test functions which start with "Test"
- Run subtests
- Run benchmark tests
- Run main function

Screen.Recording.2024-05-21.at.23.16.15.mov

cla-bot · 2024-05-17T22:39:34Z

We require contributors to sign our Contributor License Agreement, and we don't have @anilsenay on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

anilsenay · 2024-05-17T22:42:46Z

@cla-bot check

cla-bot · 2024-05-17T22:42:50Z

The cla-bot has been summoned, and re-checked this pull request!

crates/languages/src/go/runnables.scm

osiewicz · 2024-05-21T23:22:54Z

crates/languages/src/go.rs

+        let snapshot = location.buffer.read(cx).snapshot();
+        let point_range = location.range.to_point(&snapshot);
+        let end = Point::new(point_range.start.row + 1, 0);
+        let line = snapshot.text_for_range(point_range.start..end).peek();


I think it would make sense for build_context function to get access to context built up so far; that way, it could inspect variables set by the captures and you wouldn't have to extract function name from current line, which in my opinion isn't ideal. You really had no better tool at your disposal though.
Tomorrow I'll wire through the changes that will allow us to simplify that function a bit.

@osiewicz Unfortunately, I haven't found a better way to get the line. As you said, It would be better if I could just access viabuild_context more easily. I would appreciate your assistance 🙏

No worries at all! It's not like there was a better way either.
I've opened a PR in #12134; once that lands,
you'll be able to capture the first parameter (that's a string) as a hidden variable, process it (stripping "Run" prefix) and then save it as the SUBTASK_NAME.

d1y · 2024-05-22T05:24:15Z

@mrnugget #12110 Are these two duplicates?

aexvir · 2024-05-22T06:10:00Z

@mrnugget #12110 Are these two duplicates?

they do seem to overlap, yep, although this one adds support for subtests (and benchmarks) so I hope it can be rebased and merged still
most of the tests I usually run have subtests so I find it extremely convenient to be able to run specific cases

mrnugget · 2024-05-22T10:11:16Z

Woops, sorry! Didn't see this one here, I'll try to help with the rebase.

anilsenay · 2024-05-22T10:18:02Z

Woops, sorry! Didn't see this one here, I'll try to help with the rebase.

Ahaha its okey 🙏 I haven't rebase it to avoid overlap your code. You may review my code and change as you like while doing rebase.

mrnugget · 2024-05-22T11:30:12Z

Alright, I rebased and combined the code. Clicking on subtests works, but there's something fishy going on that stops them from showing up in the modal. They only show up when on first character of the line that contains t.Run. @osiewicz is looking into it.

anilsenay · 2024-05-22T11:43:38Z

Alright, I rebased and combined the code. Clicking on subtests works, but there's something fishy going on that stops them from showing up in the modal. They only show up when on first character of the line that contains t.Run. @osiewicz is looking into it.

Thank you @mrnugget 🎉

Alright, he would also look for a better way to get the selected line. Then I'll leave it to him. I'll also look at what I can do.

mrnugget · 2024-05-22T11:54:25Z

Yeah, I also pushed a commit to use the whole line.

anilsenay · 2024-05-22T13:57:48Z

crates/languages/src/go.rs

+        let line = snapshot.text_for_range(start..end).peek();

        let go_subtest_variable = extract_subtest_name(line.unwrap_or(""))


I think peek() does not return the whole line. I debugged line and it has not contain all line content. I tried to use collect and then got the whole line. Could you take a look for this issue?

let line = snapshot.text_for_range(start..end).collect::<String>(); let go_subtest_variable = extract_subtest_name(line.as_str())

It does for me.

[crates/languages/src/go.rs:492:9] &line = Some( "\tt.Run(\"Foobar\", func(t *testing.T) {\n", ) [crates/languages/src/go.rs:492:9] &line = Some( "\t\tfmt.Println(\"subtest\")\n", )

anilsenay · 2024-05-22T23:33:00Z

Yes, that's exactly the problem. It is because we're parsing the current line to look for name of subtest, but it may actually reside on a different line.

Honestly though, that's totally not your fault. It is a systemic problem (so to speak); right now we always use your current cursor position to run task context evaluation, but here it's not gonna cut it, as your context is sourced elsewhere. E.g: If you're in the middle of a test, then that test is your context. Then there's also a matter of nesting. Even though you're in a _sub_test, we should also be grabbing the context from the func TestSomething as well.

Got it, thank you for explanation 🙏
I was focusing on rendering run indicators, i wasn't aware of this modal problem until you mention it, lol.
I guess this is beyond me but I'll try to help if I figure out something via suggestion or something.

thnt · 2024-05-23T02:12:35Z

Since MR #12110 has been merged, I'm reporting here:
Because #10224 has been resolved, I expect this also works for nested folder without go.work

I also want to able to configure custom test args, ex:

{
 "languages": {
    "Go": {
      "test_args": ["-v", "-cover", "-failfast"]
    }
  }
}

anilsenay · 2024-05-23T13:44:44Z

Since MR #12110 has been merged, I'm reporting here: Because #10224 has been resolved, I expect this also works for nested folder without go.work

I also want to able to configure custom test args, ex:
{
 "languages": {
    "Go": {
      "test_args": ["-v", "-cover", "-failfast"]
    }
  }
}

Oh right, our way to set GO_PACKAGE_TASK_VARIABLE is not working for submodules. I think we should fix it.
Some solutions came in mind:

Finding go module name of running test(from go.mod) and run test in that directory (i think it can be done with cwd) as {module_name}/{the_package_name_of_test}

for example, in a working directory /a and /b is different modules which has different go.mod file. When running a test such as /a/pkg/boo_test.go,

cwd should be /a,
command should be go test module-a/pkg -run TestXxx or path can be used without module name: go test ./pkg -run TestXxx

I guess vscode-go extension doing similar like this. In addition, module names are listed in the Testing tab. So getting module name of a go file may be useful for us even later.

Easily, using file's absolute path instead of GO_PACKAGE_TASK_VARIABLE like go test /Users/user/my-app/a/b/foo_test.go - run TestXxx

@mrnugget @osiewicz

I agree that configurable test args could be helpful. For example I changed test timeout argument in vscode. I can be done by injecting user's custom arg list to task's args list maybe. Do you have any opinion or future plans for this suggestion @mrnugget @osiewicz ?

osiewicz · 2024-05-23T14:14:14Z

First off, if the user has a task in their tasks.json that's bound to your tag, that's gonna take the precedence over language-provided tasks, so there's already a way for users to override tasks from language.
I think this is a valid use case though; we need a way for user to configure language tasks.
I imagine that we could have a section in settings.json where an user could set their own value for a specific task variable. Then, in build_context you could set that task variable only if there's no user-supplied value.

In #12003 we found ourselves in need for precise region tracking in which a given runnable has an effect in order to grab variables from it. This PR makes it so that in task modal all task variables from queries overlapping current cursor position. However, in the process of working on that I've found that we cannot always use a top-level capture to represent the full match range of runnable (which has been my assumption up to this point). Tree-sitter captures cannot capture sibling groups; we did just that in Rust queries. Thankfully, none of the extensions are affected as in them, a capture is always attached to single node. This PR adds annotations to them nonetheless; we'll be able to get rid of top-level captures in extension runnables.scm once this PR is in stable version of Zed.

osiewicz · 2024-05-24T12:22:19Z

Ok, I've opened last PR (#12237) that should unblock work on this one.
I'd say we should focus on getting this merged and then - if you fancy it - we can tackle @thnt s request in a follow-up PR.

crates/languages/src/go/runnables.scm

osiewicz · 2024-05-24T12:25:07Z

crates/languages/src/go/runnables.scm

-        (#eq? @run "main"))
-) @go-main
+    (call_expression function: (_) @run @_name
+        (#match? @_name "^t.Run.*"))


Is it safe to assume that the variable we're calling Run on is always going to be called t? I get that it's a convention, but is there some precedent in Go community for breaking it sometimes?

Ok, I think we can rework this pattern altogether to simplify it a bit:
With the following query:

; `t.Run` ( (call_expression function: ( selector_expression field: _ @run @_name (#eq? @_name "Run") ) arguments: ( argument_list . (interpreted_string_literal) @_subtest_name ) ) (#set! tag go-subtest) )

In build_context the task_variables argument is gonna have the value of first argument (string literal) in _subtest_name key. You won't have to parse the line then.
So, tl;dr: in build_context you can get the value of this capture with:
variables.get(&VariableName::Custom(Cow::Borrowed("_subtest_name")));

You still need to escape the subtest name (I guess?) and set it in the context being returned from build_context (captures with _ prefix are removed after a call to build_context so that end users can't reference it).

Yes, its a convention and actually I did not want to cover an edge case for now 😄

It seems logical; I'm gonna try your new suggestion when I am free or maybe you gonna try it already.

Chipping in to say it's a fairly strong convention and good starting point. I've seen some codebases use test helpers that break the t.Run pattern but VSCode and GoLand don't match those subtests either.

Ah, that's fair. With that other query (#12003 (comment)) we don't even need to match on variable name though, as we'll capture just the name of the function being called; so the whole dispute about whether we should match t. is kinda moot. ;p

Would your example above match an unrelated Run() call like

client.Run(ctx) // should not match t.Run("client returns OK", func(t testing.T) {...}) // should match

No, it would not. We also expect the first argument to be a string literal. We could perhaps constrain it further to also require a second parameter that's (I guess?) an anonymous function taking a single param of type testing T

crates/languages/src/go/runnables.scm

Co-authored-by: Piotr Osiewicz <[email protected]>

anilsenay · 2024-05-24T13:01:18Z

Ok, I've opened last PR (#12237) that should unblock work on this one. I'd say we should focus on getting this merged and then - if you fancy it - we can tackle @thnt s request in a follow-up PR.

If you talked about submodule issue, I will open an issue for that not to be forgotten after this PR merged.

crates/languages/src/go/runnables.scm

…odal (#12237) In #12003 we found ourselves in need for precise region tracking in which a given runnable has an effect in order to grab variables from it. This PR makes it so that in task modal all task variables from queries overlapping current cursor position. However, in the process of working on that I've found that we cannot always use a top-level capture to represent the full match range of runnable (which has been my assumption up to this point). Tree-sitter captures cannot capture sibling groups; we did just that in Rust queries. Thankfully, none of the extensions are affected as in them, a capture is always attached to single node. This PR adds annotations to them nonetheless; we'll be able to get rid of top-level captures in extension runnables.scm once this PR is in stable version of Zed. Release Notes: - N/A

osiewicz

LGTM :)

Hide @second_argument from user Co-authored-by: Piotr Osiewicz <[email protected]>

…odal (zed-industries/zed#12237) In zed-industries/zed#12003 we found ourselves in need for precise region tracking in which a given runnable has an effect in order to grab variables from it. This PR makes it so that in task modal all task variables from queries overlapping current cursor position. However, in the process of working on that I've found that we cannot always use a top-level capture to represent the full match range of runnable (which has been my assumption up to this point). Tree-sitter captures cannot capture sibling groups; we did just that in Rust queries. Thankfully, none of the extensions are affected as in them, a capture is always attached to single node. This PR adds annotations to them nonetheless; we'll be able to get rid of top-level captures in extension runnables.scm once this PR is in stable version of Zed. Release Notes: - N/A

cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 17, 2024

mikayla-maki assigned mikayla-maki, SomeoneToIgnore and osiewicz and unassigned mikayla-maki May 17, 2024

anilsenay changed the title ~~Add runnables for Go~~ go: Add runnables for Go May 18, 2024

filipwiech mentioned this pull request May 19, 2024

Is there a way to trigger execution of a class (ScalaTest usecase) scalameta/metals-zed#1

Open

SomeoneToIgnore removed their assignment May 21, 2024

osiewicz reviewed May 21, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

osiewicz reviewed May 21, 2024

View reviewed changes

anilsenay and others added 6 commits May 22, 2024 12:31

Add runnables for Go

6c3d35b

Fix subtest names

c9db1dd

Fix failing workflow jobs

43952f1

Add task names into labels

9e65011

Update pattern to capture the function name as @_name

71ec940

Combine GoContextProvider after rebase

6ab7614

mrnugget force-pushed the go-runnables branch from f48f6b5 to 6ab7614 Compare May 22, 2024 11:25

Fix Clippy error

e5eb7f4

Use whole line and fix label

196b638

anilsenay commented May 22, 2024

View reviewed changes

osiewicz mentioned this pull request May 24, 2024

JavaScript: Add runnable tests #12118

Merged

osiewicz mentioned this pull request May 24, 2024

tasks: Provide task variables from matching runnable ranges in task modal #12237

Merged

osiewicz reviewed May 24, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

osiewicz reviewed May 24, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

osiewicz reviewed May 24, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

anilsenay and others added 3 commits May 24, 2024 15:47

Use eq instead of match for selecting main function

06e7049

Co-authored-by: Piotr Osiewicz <[email protected]>

Update crates/languages/src/go/runnables.scm

1090579

Co-authored-by: Piotr Osiewicz <[email protected]>

Update tagging method for go runnables

6bc9a9c

anilsenay added 2 commits May 24, 2024 19:39

Merge branch 'main' of github.com:anilsenay/zed into go-runnables

4c14ac3

Update runnable scheme for subtests

1db2f07

anilsenay commented May 24, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

anilsenay commented May 24, 2024

View reviewed changes

crates/languages/src/go/runnables.scm Outdated Show resolved Hide resolved

Fix clippy error

13280b6

Merge branch 'main' of github.com:anilsenay/zed into go-runnables

444e262

osiewicz approved these changes May 26, 2024

View reviewed changes

Update crates/languages/src/go/runnables.scm

6248a00

Hide @second_argument from user Co-authored-by: Piotr Osiewicz <[email protected]>

osiewicz merged commit ddb551c into zed-industries:main May 26, 2024
8 checks passed

anilsenay mentioned this pull request May 26, 2024

go: testing submodule support for runnables #12323

Closed

1 task

osiewicz mentioned this pull request Jun 1, 2024

python: Add runnable unittest tasks #12451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go: Add runnables for Go #12003

go: Add runnables for Go #12003

anilsenay commented May 17, 2024 •

edited

Loading

cla-bot bot commented May 17, 2024

anilsenay commented May 17, 2024

cla-bot bot commented May 17, 2024

osiewicz May 21, 2024

anilsenay May 21, 2024 •

edited

Loading

osiewicz May 22, 2024

d1y commented May 22, 2024

aexvir commented May 22, 2024

mrnugget commented May 22, 2024

anilsenay commented May 22, 2024

mrnugget commented May 22, 2024

anilsenay commented May 22, 2024

mrnugget commented May 22, 2024

anilsenay May 22, 2024

mrnugget May 22, 2024

anilsenay commented May 22, 2024

thnt commented May 23, 2024 •

edited

Loading

anilsenay commented May 23, 2024

osiewicz commented May 23, 2024 •

edited

Loading

osiewicz commented May 24, 2024

osiewicz May 24, 2024

osiewicz May 24, 2024 •

edited

Loading

anilsenay May 24, 2024

kisunji May 24, 2024

osiewicz May 24, 2024

kisunji May 24, 2024

osiewicz May 24, 2024

anilsenay commented May 24, 2024

osiewicz left a comment

		let line = snapshot.text_for_range(start..end).peek();

		let go_subtest_variable = extract_subtest_name(line.unwrap_or(""))

go: Add runnables for Go #12003

go: Add runnables for Go #12003

Conversation

anilsenay commented May 17, 2024 • edited Loading

cla-bot bot commented May 17, 2024

anilsenay commented May 17, 2024

cla-bot bot commented May 17, 2024

Choose a reason for hiding this comment

anilsenay May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d1y commented May 22, 2024

aexvir commented May 22, 2024

mrnugget commented May 22, 2024

anilsenay commented May 22, 2024

mrnugget commented May 22, 2024

anilsenay commented May 22, 2024

mrnugget commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilsenay commented May 22, 2024

thnt commented May 23, 2024 • edited Loading

anilsenay commented May 23, 2024

osiewicz commented May 23, 2024 • edited Loading

osiewicz commented May 24, 2024

Choose a reason for hiding this comment

osiewicz May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anilsenay commented May 24, 2024

osiewicz left a comment

Choose a reason for hiding this comment

anilsenay commented May 17, 2024 •

edited

Loading

anilsenay May 21, 2024 •

edited

Loading

thnt commented May 23, 2024 •

edited

Loading

osiewicz commented May 23, 2024 •

edited

Loading

osiewicz May 24, 2024 •

edited

Loading