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

testing: optionally include full (or relative) path name #37708

Closed
stapelberg opened this issue Mar 6, 2020 · 16 comments
Closed

testing: optionally include full (or relative) path name #37708

stapelberg opened this issue Mar 6, 2020 · 16 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@stapelberg
Copy link
Contributor

Previous discussion happened in issue #21119.

Currently, t.Error calls print only the base name of the source file.

This is fine when running a single test, but in larger projects, it is a significant productivity boost to have relative (or full) path names, which make the “jump to next compilation error” feature in popular editors work.

I understand the outcome of #21119 (can’t change the default), but I suggest adding a knob to opt into printing relative or full paths.

Perhaps this could be similar to log.SetFlags, where you can specify Llongfile or Lshortfile?

Let me know if this would be okay, and I’d be happy to send a CL.

@mvdan
Copy link
Member

mvdan commented Mar 6, 2020

@rogpeppe, @myitcv and myself discussed a very similar idea not too long ago.

It's unfortunate that we can't change the default. However, there is some precedent for such an opt-in flag:

$ go tool compile -h 2>&1 | grep -- -L
  -L	show full file names in error messages

I also wonder if go test could transform filename paths transparently, when it buffers output from test binaries. The only case where the test binary output goes straight to the terminal is when go test is run without arguments - and, in that case, the paths are already relative to the current directory, as we're testing the package in the current directory too. Transforming paths should be possible to do, given that test2json does very similar parsing of plaintext test output.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2020
@toothrot toothrot added this to the Backlog milestone Mar 9, 2020
@toothrot
Copy link
Contributor

toothrot commented Mar 9, 2020

also ccing @mpvl

@aryman-glympse
Copy link

Is there anyway we can do this similar to how the log package sets flags for log outputs?

// setting longfile in package log.
log.SetFlags(log.Lmicroseconds | log.Llongfile)

Proposed:

testing.SetFlags(testing.Llongfile)

@dnephin
Copy link
Contributor

dnephin commented Dec 17, 2022

#55976 was closed as a duplicate of this issue, but this one seems to be stuck in the backlog. Can we turn this into a proposal?

Moving my comment here for consideration:

I've noticed occasionally GoLand links to the wrong file when tests are run from the terminal window inside GoLand. I suspect it's for exactly the reason described above. The filename is ambiguous. There can be multiple files with the same name in a project, and when tests are run from the terminal window, it doesn't know which of those files to link to. An absolute path would allow this feature to work reliably.

This problem exists even when t.Helper is used correctly. The tooling that is reading the output doesn't necessarily know which directory is the current working directory because tests can be run from any directory.

I suspect the absolute path is really only useful when the output is being read by another application, and the filename is better when the output is being read by a human. Instead of a new flag to enable this behaviour, the existing -json flag on go test could be used to enable output of absolute paths instead of filenames.

1c72ee7 recently added -test.v=json, which I think allows testing to detect if -json is set or not.

@ianlancetaylor ianlancetaylor changed the title testing: optionally include full (or relative) path name proposal: testing: optionally include full (or relative) path name Dec 21, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Dec 21, 2022
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Dec 21, 2022
@rsc rsc moved this from Incoming to Active in Proposals Dec 21, 2022
@rsc
Copy link
Contributor

rsc commented Dec 21, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@jmhwang7
Copy link

Excited to see that the proposal will be reviewed at the weekly proposal review meeting!

It would be great if there was an option to print the full/relative path of the source file even when the test succeeds.

For my company's repos, I'm working on associating tests with owners (using a CODEOWNERS file within the repo). We have large packages that multiple teams contribute too, so it's impossible to assign ownership based on package unless we take on a large amount of refactoring work. It's fairly easy however to assign ownership by files.

I'm able to assign ownership to a testcase when it fails based on the filename printed, however, I haven't been able to assign ownership for tests that pass. Printing the full/relative path of the source file for every test run would enable this.

@ianlancetaylor
Copy link
Member

I think the most reasonable way to do this would be to add an option to go test. That would then permit cases like go test ... to report full path names for testing log messages, which is convenient if there are a lot of packages with similar file names.

So what should the option be? I suggest -test.fullpath, and perhaps go test should recognize -fullpath and convert it as it does with several other options.

@rsc
Copy link
Contributor

rsc commented Jan 11, 2023

Does anyone object to adding -fullpath as a test option for this?

@rsc
Copy link
Contributor

rsc commented Jan 18, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Jan 18, 2023
@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 25, 2023
@rsc
Copy link
Contributor

rsc commented Jan 25, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: testing: optionally include full (or relative) path name testing: optionally include full (or relative) path name Jan 25, 2023
@rsc rsc modified the milestones: Proposal, Backlog Jan 25, 2023
@stapelberg
Copy link
Contributor Author

Great! Does anyone want to contribute a CL for this? Otherwise I can have a look, but it might be a few weeks until I find the time.

@empire
Copy link
Contributor

empire commented Jan 26, 2023

@stapelberg, I've prepared a CL for this, and I'll send it in the next few days.

@hyangah
Copy link
Contributor

hyangah commented Jan 26, 2023

This will help vscode go extension's job much easier. (vscode runs go test or dlv test from the open file's directory).
Can we extend this flag apply to produce absolute path names for build errors? (-gcflags=all=-L should do it in theory, but it doesn't do it now)
That way, when a test fails due to build error, the full path will be shown with the build errors.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/463837 mentions this issue: testing: add -fullpath to go test

@stapelberg
Copy link
Contributor Author

I can confirm that setting -fullpath works as expected in Emacs’s compilation-mode :)

% ~/upstream-go/bin/go test -fullpath -count=1 -run=/sqlite -v ./...
[…]
=== RUN   TestUpdate/sqlite
2023/02/23 19:27:28 using database: txdb/sqlite
2023/02/23 19:27:28 Ingested image "abcdefg" (matching "scan2drive")
2023/02/23 19:27:28 Setting desired image for machine "scan2drive" to "abcdefg"
    /home/michael/go/src/github.com/gokrazy/gus/internal/gusserver/update_test.go:61: update: diff (-want +got):
          gusapi.UpdateResponse{
          	SbomHash:     "abcdefg",
        - 	RegistryType: "localdiska",
        + 	RegistryType: "localdisk",
          	DownloadLink: "/doesnotexist/disk.gaf",
          }

Pressing M-g n brings me into the correct file and line 🎉

Thanks for getting this implemented :)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/498396 mentions this issue: doc/go1.21: mention new go test -fullpath option

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 26, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 May 26, 2023
gopherbot pushed a commit that referenced this issue May 26, 2023
For #37708

Change-Id: I7b04d6331c15771c7d74ff77afd523c435e0dafe
Reviewed-on: https://go-review.googlesource.com/c/go/+/498396
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Eli Bendersky <eliben@google.com>
Reviewed-by: Eli Bendersky <eliben@google.com>
@rsc rsc removed this from Proposals Mar 1, 2024
@golang golang locked and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests