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: store test artifacts #71287

Open
neild opened this issue Jan 15, 2025 · 15 comments
Open

proposal: testing: store test artifacts #71287

neild opened this issue Jan 15, 2025 · 15 comments
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jan 15, 2025

This is an offshoot of #43936.

Some tests produce output files which the user may want to examine. For example, a test might produce files which are compared to some reference. If the comparison fails, the user will want to examine the generated files. Or a test might produce a packet capture or similar trace which can be used to debug failures.

We call these output files "test artifacts".

This is a proposal to add support for storing test artifacts to the testing package.

We add a new method to testing.TB:

package testing

// OutputDir returns a directory for the test to store output files in.
// When the -outputdir flag is provided, this directory will be located
// under that directory. Otherwise, OutputDir returns a temporary directory
// which is removed after the test completes.
//
// Each test or subtest has a unique artifact directory.
// Repeated calls to OutputDir in the same test or subtest return the same directory.
// Subtest outputs are not located under the parent test's output directory.
func (t *testing.T) OutputDir() string

The -outputdir flag already exists, and is currently used to specify a location to put output files from profiling. We're adding an additional meaning to it here: It's now where all your saved test outputs go.

When -outputdir is specified, the first call to OutputDir in a test or subtest will emit a line to the test output consisting of "=== ARTIFACTS ", the test name, and the test artifact directory, separated by spaces:

=== ARTIFACTS TestName/subtest_name /path/to/root/artifact/dir/TestName__subtest_name

When -json is specified, this will appear as an entry with an Action of "artifacts", the usual Time, Package, and Test keys, and a "Path" key containing the artifact directory:

{"Time":"2025-01-15T13:39:27.75235-08:00","Action":"artifacts","Package":"path","Test":"TestName","Path":"/path/to/root/artifact/dir/TestName"}

That's the proposal.

A few points on the design:

  • I'm reusing the existing -outputdir flag, on the theory that output files from profiling are just another test artifact. If we don't like that reuse, then we could add a new -artifactdir flag and rename TB.OutputDir to TB.ArtifactDir for consistency.

  • The test output uses the word "ARTIFACTS" because the JSON already has "output" events.

  • TB.OutputDir returns a directory, same as TB.TempDir. This seems simpler than asking the testing package to copy files around.

  • TB.OutputDir returns a directory even if we aren't saving artifacts so test behavior doesn't change depending on the presence or absence of the -outputdir flag.

In simple interactive use, users can pass -outputdir to store test artifacts when debugging a failing test.

Test frameworks that collect artifacts can arrange to pass -outputdir to the tests they run and collect any artifacts after the fact.

As a concrete use case, within Google our testing infrastructure sets an environment variable to the location of a directory. Tests can write files into this directory, and those files will be stored and associated with the test run. If we implement this proposal, we can arrange for the test infrastructure to also pass this directory as an -outputdir flag, and any test using TB.OutputDir will automatically use the right location.

@mknyszek
Copy link
Contributor

mknyszek commented Jan 15, 2025

Speaking from the perspective of Go's CI infrastructure, it would be helpful to have some standard convention to indicate output artifacts at the standard library level, so I'm in support of this proposal (or something like it).

We share the tooling for processing Go tests with several other teams, and without a standard for this sort of thing, we would be stuck having to define our own amongst ourselves. That may turn out to be OK in practice, but having a single way to do it would save us all the time of trying to define our own convention, since it wouldn't even be a question. As far as test metadata goes, I think this would be one of the most valuable forms, for the Go team anyway, and IMO more valuable than Attr on its own due the well-defined convention. I can think of several use-cases for exactly this:

  • Execution trace tests and pprof tests would emit artifacts when they encounter broken traces produced by the runtime, for debugging.
  • Crashes in CI would result in core dumps that can be easily downloaded and dissected. (So many more issues would be possible to debug!)
  • Integration tests for binary format serialization/deserialization could dump intermediate artifacts when things go wrong.

I don't have many thoughts on the specifics of how this should look in the output or in the testing package, but from my perspective this doesn't seem like much of an overreach into framework territory. It's a fairly common and simple piece of functionality, and again, essentially just defining a convention.

@gabyhelp gabyhelp added the LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool label Jan 15, 2025
@seankhliao
Copy link
Member

How would this interact with fuzzing which currently stores new inputs that cause crashes in testdata/fuzz/ ?

@neild
Copy link
Contributor Author

neild commented Jan 15, 2025

No changes to fuzzing, unless I'm missing something. Is there some interaction that should happen there?

@seankhliao
Copy link
Member

I was thinking they're similar in that they generate test artifacts, and if you're running in some remote CI system, you may want a (relatively) easy way to extract the new additions from the other fuzz inputs, such as redirecting them to a specified directory as in this proposal.

@neild
Copy link
Contributor Author

neild commented Jan 16, 2025

I don't think we can have -outputdir stop us from writing fuzz crashes to testdata/fuzz; anyone using fuzzing now is presumably depending on the current behavior and won't want it changed by an orthogonal feature.

Perhaps we could write fuzz crashes to both testdata/fuzz and -outputdir. But I think simpler would be to have a separate -fuzzoutputdir flag to set where new fuzz crashes get written. If you want everything in the same place, set both -fuzzoutputdir and -outputdir.

I think a -fuzzoutputdir flag is probably a separate proposal, though.

@earthboundkid
Copy link
Contributor

Instead of returning a string, could it return an os.Root?

@neild
Copy link
Contributor Author

neild commented Jan 16, 2025

Instead of returning a string, could it return an os.Root?

It could, but that'd be a gratuitous divergence from TB.TempDir. If you want a Root, it's easy enough to open the directory as one. And sometimes you do want just the directory name (when passing it as a flag to some other process, say).

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jan 16, 2025
@willfaught
Copy link
Contributor

willfaught commented Jan 16, 2025

If I run such tests for an unfamiliar package with just go test, will my system accrue these output artifacts permanently without my knowing? How do I clean these, or prevent them entirely?

Where will the paths likely be? Under /tmp, under the package, under GOPATH?

Do tests get unique paths over multiple test runs, or does one test run overwrite the artifacts of another for the same test? If overwrite, are all previous artifacts deleted first?

I was thinking they're similar in that they generate test artifacts, and if you're running in some remote CI system, you may want a (relatively) easy way to extract the new additions from the other fuzz inputs, such as redirecting them to a specified directory as in this proposal.

Perhaps fuzz artifacts should be thought of as generated inputs, not outputs, so they shouldn't be included in this.

@neild
Copy link
Contributor Author

neild commented Jan 16, 2025

If I run such tests for an unfamiliar package with just go test, will my system accrue these output artifacts permanently without my knowing? How do I clean these, or prevent them entirely?

If you don't pass the --outputdir flag to "go test", artifacts get written to a temporary directory (exactly the same as if they were written to a t.TempDir) and are deleted after the test finishes.

If you do pass --outputdir, artifacts get written to that directory and you can decide what to do with them from there. Persistent artifacts are only produced when you ask for them, in the location you specify.

Do tests get unique paths over multiple test runs, or does one test run overwrite the artifacts of another for the same test? If overwrite, are all previous artifacts deleted first?

That is an excellent question. I'm going to have to think about it.

@neild
Copy link
Contributor Author

neild commented Jan 21, 2025

When a test using T.OutputDir is run multiple times (-count=N), I propose that each run gets a separate, empty output directory. We store all the outputs from all runs.

I see three possible behaviors here:

  1. Each run gets the same output directory. Nothing is cleaned up between runs, so a test may start with files in its output directory. I don't like this option at all. Test runs should be as independent of each other as we can make them, and leaving around old outputs works against that.
  2. Each run gets a clean output directory. We delete most of them and preserve only a few. (Maybe the outputs from the first run to succeed and the first to fail?) Keeps the total size of the outputs down, but we need to decide on which runs to keep. And what if someone wants all those outputs?
  3. Store everything. Simple. Easy to describe. Outputs might get large, but disks are big and the user can run go test multiple times with a smaller -count if they want to limit the total output size per invocation.

@aclements
Copy link
Member

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

@aclements aclements moved this from Incoming to Active in Proposals Jan 23, 2025
@aclements
Copy link
Member

I'm reusing the existing -outputdir flag, on the theory that output files from profiling are just another test artifact.

This is currently documented as

	-outputdir directory
	    Place output files from profiling in the specified directory,
	    by default the directory in which "go test" is running.

This will muddy the "default" value. I think under this proposal, if -outputdir isn't specified, profiles would still go to the current directory, but artifacts would go to a temporary directory.

There's also an implementation issue with using -outputdir. Currently, go test always passes -test.outputdir if any test profiling flags are passed. It has to do this because go test can change the current directory before running the test binary and needs to communicate the "original" output directory.

One option would be for go test to rewrite the paths to the profiling flags to always include the full path, and pass -test.outputdir if and only if the user passed -outputdir. Then the test would be able to tell if the user set -outputdir.

When a test using T.OutputDir is run multiple times (-count=N), I propose that each run gets a separate, empty output directory. We store all the outputs from all runs.

This sounds reasonable. If we give each iteration the same output directory, there's bound to be a confusing mix of fixed file names that get overwritten each time and generated file names that accumulate.

@neild
Copy link
Contributor Author

neild commented Jan 27, 2025

If muddying the default value for -outputdir is a concern, we can add a separate -artifactdir flag instead. If we do that, I think the method should be T.ArtifactDir for consistency.

Another option might be to add -profiledir and -artifactdir flags, and say that -outputdir sets the default for both.

@rsc
Copy link
Contributor

rsc commented Feb 5, 2025

For profiling, there are two separate concerns: "enable profiling" and "where do profiles get written" (outputdir).
The current artifact proposal tries to merge them into one concern "where do artifacts get written" (unfortunately also called outputdir).

It seems reasonable to have one outputdir and just have a flag for "write artifacts" (default off). Then we can still keep the new OutputDir method, which may be useful regardless, but also have a "Artifacts() bool" method or something like that. Maybe it's not a method but is a top-level function like testing.Short. So you'd do

go test -artifacts

to drop them in the current directory, and by default you wouldn't get them at all.

Although I'm not sure why it's one flag to enable all possible artifacts. It seems like you might want each artifact writer to define its own flag, or at least have some kind of sub-selection. In the explanation:

Some tests produce output files which the user may want to examine. For example, a test might produce files which are compared to some reference. If the comparison fails, the user will want to examine the generated files. Or a test might produce a packet capture or similar trace which can be used to debug failures.

If one test is failing, it doesn't seem like you want to save all the artifacts from all the tests. Maybe -artifacts takes a test pattern? Or something else?

artifacts is also such a long string to type. It would be nice if we had a shorter name for these.

@neild
Copy link
Contributor Author

neild commented Feb 5, 2025

If you don't want artifacts from all tests, you can just run the tests that you want artifacts for, no? "Run these tests, but only save the artifacts from those tests" seems overly complicated.

A separate "write artifacts (to the output dir, which might be the current dir)" flag sounds reasonable to me.

I don't really like an Artifacts() bool method. That implies that a test running in save-artifacts mode behaves differently than one running in the default configuration. A test might fail in one configuration and not the other, and there will be extra test complexity around deciding whether and where to put files.

For example, one place I might use artifact storage is in a test for a code generator, like the protobuf compiler. These tests often compare the output of the generator to a golden reference. Storing the current output in the test artifacts is useful when debugging changes. With the current proposal, that's a simple matter of putting the files in t.OutputDir rather than t.TempDir directory. With an Artifacts() bool method it gets more complicated; do you copy the files after the test runs, do you change which directory you use based on the bool?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Projects
Status: Active
Development

No branches or pull requests

9 participants