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: don't treat TestMain as a test #482

Closed
zmb3 opened this issue Aug 6, 2020 · 5 comments
Closed

codelens: don't treat TestMain as a test #482

zmb3 opened this issue Aug 6, 2020 · 5 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@zmb3
Copy link
Contributor

zmb3 commented Aug 6, 2020

What version of Go, VS Code & VS Code Go extension are you using?

  • Run go version to get version of Go
    • 1.14.6
  • Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders
1.48.0-insider
cfbd1999769f4f08dce29629fb92fdc0fac53829
x64
  • Check your installed extensions to get the version of the VS Code Go extension
    • 0.16.1

Describe the bug

The feature that puts clickable "run test | debug test" on top of test functions identifies TestMain as a runnable test.

image

TestMain is not something that works with go test -run ..., so clicking the "run test" button results in the following slightly confusing message.

Running tool: /usr/local/bin/go test -timeout 30s -run ^TestMain$

testing: warning: no tests to run
PASS
ok  	my/pkg/here	0.153s

Would it be more clear if we simply didn't offer the run/debug button for TestMain?

Steps to reproduce the behavior:

  1. Create a TestMain
  2. Click on "run test"
  3. See error
@hyangah hyangah added NeedsFix The path to resolution is known, but the work has not been done. HelpWanted Issues that are not prioritized by the maintainers. Help is requested from community contributors. labels Aug 6, 2020
@hyangah
Copy link
Contributor

hyangah commented Aug 6, 2020

Needs adjustment in the filters in getTestFunctions, etc.

--
FYI we are also working with gopls so gopls provides those codelenses in the future.
@mcjcloud can you please help checking if gopls is generating codelens for TestMain?

@hyangah hyangah added this to the Backlog milestone Aug 7, 2020
@hyangah hyangah added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. HelpWanted Issues that are not prioritized by the maintainers. Help is requested from community contributors. labels Aug 7, 2020
@suzmue
Copy link
Contributor

suzmue commented Aug 7, 2020

I looked into this issue a bit (thanks @zmb3 for the easy repro) and just wanted to update this with some things I found and thoughts on a fix:

So it looks like 'TestMain' is actually run successfully using go test -run ... and the error is coming from the testing package from the call to m.Run(). The message is definitely confusing as the TestMain function did successfully run, m just had no tests to run.

Since it is actually possible to run this test using go test -run ... I think it would be valid to leave the 'run test, debug test' buttons on this and some people may have a use for doing so (debugging the setup or teardown code, etc).

func TestMain(t *testing.T) is considered a regular test, so a correct fix would need to check the function signature, too.

@zmb3
Copy link
Contributor Author

zmb3 commented Aug 8, 2020

Since it is actually possible to run this test using go test -run ...

I agree that it's possible to run the TestMain func using go test -run, but I'm not sure I'd call it a test. It will always say "no tests to run" and there will never be any tests that pass or fail when clicking that button, so is it the button actually useful?

If someone wants to debug the setup or teardown code, they can still put breakpoints in TestMain and click "debug test" on an actual test.

@hyangah
Copy link
Contributor

hyangah commented Aug 10, 2020

since func TestMain(t *testing.T) is a regular test, yes we need to check the function signature. A heuristic would be to read the line (of the symbol's range) and see if it's including testing.M. But that's till a heuristic.

Looking at the gopls source code,
it seems that it already does do the right filtering.
We are currently in the process of delegating the code lens computation to the language server.
How about waiting for the transition to complete?

@hyangah hyangah changed the title don't treat TestMain as a test codelens: don't treat TestMain as a test Sep 3, 2020
@hyangah
Copy link
Contributor

hyangah commented Sep 17, 2020

I will close this issue in favor of #654.

@hyangah hyangah closed this as completed Sep 17, 2020
@golang golang locked and limited conversation to collaborators Sep 17, 2021
@polinasok polinasok self-assigned this Mar 30, 2022
gopherbot pushed a commit that referenced this issue May 18, 2022
…estMain(*testing.M)

<img width="673" alt="image" src="https://user-images.githubusercontent.com/51177946/160227163-0c38a63d-32a5-43cc-883f-39730df5d421.png">
<img width="674" alt="image" src="https://user-images.githubusercontent.com/51177946/160227174-0ed987ff-f587-4085-b47a-2b13d86e3dbe.png">
<img width="672" alt="image" src="https://user-images.githubusercontent.com/51177946/160227189-59d572fe-8847-4d62-b1ce-b0355fe4fca7.png">
<img width="670" alt="image" src="https://user-images.githubusercontent.com/51177946/160227203-3a8bcc4d-bf0d-486b-8ec1-68821ecdeb36.png">

<img width="871" alt="image" src="https://user-images.githubusercontent.com/51177946/160894934-f772660d-f100-495f-95c0-6d0f88ea0005.png">
<img width="874" alt="image" src="https://user-images.githubusercontent.com/51177946/160895017-c5493dab-99f3-4567-a8f4-b822d2616ad4.png">

Fixes #482
Fixes #2039

Change-Id: I9b69d762b8a968f94931455d771c03ff6e73182e
GitHub-Last-Rev: 650fb12
GitHub-Pull-Request: #2129
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/394136
Reviewed-by: Polina Sokolova <polina@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Polina Sokolova <polina@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
5 participants