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

proposal: testing: flag to not truncate file names when logging #55976

Closed
firelizzard18 opened this issue Sep 30, 2022 · 7 comments
Closed

proposal: testing: flag to not truncate file names when logging #55976

firelizzard18 opened this issue Sep 30, 2022 · 7 comments

Comments

@firelizzard18
Copy link
Contributor

I propose to add a flag to go test, such as -logabspath, that will print out the absolute path of the location of a call to (*testing.T).Log and friends instead of just the file name.

*testing.T methods that log (via (*testing.T).log) print out the location of the call, truncating the filename to just the basename. This may be helpful to users but it is problematic for tools like the VSCode Go extension. The Go extension's test support uses that location information to attach a log event to the source location. The extension assumes that such calls are made from files in the same package as the test, so this works in that case. However, it fails if those methods are called from a location outside of the test's package (if that location does not also call (*testing.T).Helper(). My proposal would provide an opt-in way for tooling such as the extension to change this behavior.

@gopherbot gopherbot added this to the Proposal milestone Sep 30, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: src/testing: flag to not truncate file names when logging proposal: testing: flag to not truncate file names when logging Sep 30, 2022
@apparentlymart
Copy link

apparentlymart commented Oct 4, 2022

Whenever I think about a possible new option I also like to think about whether it might be possible to change the default behavior in a reasonable way to avoid adding the new option. 😀

In this case, I wonder: would it be reasonable to change the default behavior so that files in the current package will stay as relative paths but files in any other package will always be printed as absolute paths, to avoid the ambiguity?

My sense is that a test helper that fails to call t.Helper() is probably either some old code (from before that facility existed) or a mistake, and so most of the time the author would want to update it to include t.Helper() and therefore report a more useful path. Under that assumption it could be reasonable to print a possibly-obnoxiously-long path in the unusual case where a test helper isn't marked as such, making that oversight more obvious in the testing output. (And as a bonus it avoids the need for an option!)

(I may well be missing a situation where it's desirable to intentionally produce test logging attributed to a line that isn't in the current package, in which case I'd withdraw the above suggestion.)

@firelizzard18
Copy link
Contributor Author

I'd be happy with anything that allows the VSCode Go extension to unambiguously identify the correct file. I suggested a flag because that means the default is no change in behavior.

Regardless of why t.Helper() is not called, incorrect/ambiguous paths is a bad experience for the user. For log traces that come from the current package, the user can ctrl/cmd-click on the log to open the file and line. For log traces from other packages, the user tries to ctrl/cmd-click on the log file but it fails because the path is wrong. As an extension developer (I wrote the test explorer integration), I want to provide a good user experience even if the developer made a mistake (e.g. forgot to call t.Helper()).

In my own work, there is an unavoidable case where t.Helper() does not help: code running in a goroutine. Some of the tooling I use for tests runs asynchronously (and lives in a different package), so there is no current package file in the stack trace for that code. In other cases, the tooling is used for other purposes beyond just Go unit tests such that I do not want it to depend on the testing package, so errors are passed to a callback (which may call t.Log()) but it is not appropriate for the tooling to call t.Helper().

@firelizzard18
Copy link
Contributor Author

In case it matters, if this proposal is accepted I am happy to submit the CL myself.

@dnephin
Copy link
Contributor

dnephin commented Dec 17, 2022

Thanks for creating this proposal!

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.

@seankhliao
Copy link
Member

Duplicate of #37708

@seankhliao seankhliao marked this as a duplicate of #37708 Dec 17, 2022
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Dec 17, 2022
@dnephin
Copy link
Contributor

dnephin commented Dec 17, 2022

@seankhliao thanks for the link!

This issue is labeled as a proposal, where as the other one seems to be stuck in the backlog as "needs investigation".

Is there some way we can make that one a proposal as well, so that it gets some attention? Or can we keep this one since this one is correctly labelled?

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants