design/2981: simplify design

Address comments by adg and aclements

Change-Id: I7a1d8f640a06804f7809e977758d301198839f04
Reviewed-on: https://go-review.googlesource.com/29157
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/design/2981-go-test-json.md b/design/2981-go-test-json.md
index 7393bda..d37720a 100644
--- a/design/2981-go-test-json.md
+++ b/design/2981-go-test-json.md
@@ -4,7 +4,7 @@
 
 _With initial input by Russ Cox, Caleb Spare, Andrew Gerrand and Minux Ma._
 
-Last updated: 2015-10-16
+Last updated: 2016-09-14
 
 Discussion at https://golang.org/issue/2981.
 
@@ -12,7 +12,7 @@
 *  [Background](#background)
 *  [Proposal](#proposal)
    *  [`testing` package](#testing-package)
-   *  [`go test`](#go-test)
+   *  [Example output](#example-output)
 *  [Rationale](#rationale)
 *  [Compatibility](#compatibility)
 *  [Implementation](#implementation)
@@ -32,37 +32,29 @@
 
 Currently, under certain conditions, `go test` streams test/benchmark results
 so a user can see them as they happen.
+Also streaming prevents losing data if `go test` crashes.
 This proposal attempts to preserve streaming capability in the `-json` mode, so
 third party tools interpreting `go test` output can stream results too.
 
 `-json` flag was originally proposed by Russ Cox in
-https://golang.org/issue/2981 in 2012.
-This proposal differs from the original:
-
-* supports streaming
-* `go test` JSON output contains unrecognized test binary output.
-* minimal changes in `testing` package.
-* output is not indented
+https://golang.org/issue/2981 in 2012. This proposal has several differences.
 
 ## Proposal
 
 I propose the following user-visible changes:
 
 *   add `-json` flag to `go test`
-    *   `-json`: all `go test` stdout is JSON objects containing
-        test binary artifacts, separated by newline.
+    *   `-json`: `go test` stdout is a valid [JSON Text Sequence][rfc7464]
+        of JSON objects containing test binary artifacts.
         Format below.
-    *   `-json -v`: verbose messages are printed to stderr,
-        so stdout contains only JSON.
+    *   `-json -v`: verbose messages are printed to stderr, so stdout contains
+        only JSON.
     *   `-json -n`: not supported
     *   `-json -x`: not supported
 *   In `testing` package
-    *   Add `type State` which is enum
-    *   Add `Name`, `Log`, `State` and `Procs` fields to `BenchmarkResult`.
-    *   Add `type CoverageResult`.
-    *   In type `Cover`, change `CoveredPackages` field type from `string` to
-        `[]string`. This type is not covered by Go 1 compatibility guidelines.
-    *   Add `type JSONResult` for JSON output.
+    *   Add `type State int` with constants to describe test/benchmark states.
+    *   Add type `JSONResult` for JSON output.
+    *   Change `Cover.CoveredPackages` field type from `string` to `[]string`.
 
 Type definitions and details below.
 
@@ -75,267 +67,277 @@
 type State int
 
 const (
-    RUN State = iota
+    // RUN means a test/benchmark execution has started
+    RUN State = iota + 1
     PASS
     FAIL
     SKIP
 )
 
-type BenchmarkResult struct {
-    Name  string
-    State State
-    Procs int    // The value of runtime.GOMAXPROCS for this benchmark run.
-    Log   string // The log created by calling (*B).Log and (*B).Logf.
-
-    // existing fields
-    // make them `json:",omitempty"`
-}
-
-// CoverageResult is aggregated code coverage info.
-// It is used for `go test` JSON output.
-// To get full coverage info, use -coverprofile flag in go test.
-type CoverageResult struct {
-    Mode             string
-    TotalStatements  int64
-    ActiveStatements int64
-    CoveredPackages  []string
-}
-
-// JSONResult is used for test binary JSON output format.
-//
-// Each time a test/benchmark completes, the test binary emits one result
-// in JSON format to stdout, surrounded by '\n'.
+// JSONResult structs encoded in JSON are emitted by `go test` if -json flag is
+// specified.
 type JSONResult struct {
-  // BenchmarkResult contains fields used by both benchmarks and tests,
-  // such as Name and State.
-  BenchmarkResult
+    // Configuration is metadata produced by test/benchmark infrastructure.
+    // The interpretation of a key/value pair is up to tooling, but the key/value
+    // pair describes all test/benchmark results that follow,
+    // until overwritten by a JSONResult with a non-empty Configuration field.
+    //
+    // The key begins with a lowercase character (as defined by unicode.IsLower),
+    // contains no space characters (as defined by unicode.IsSpace)
+    // nor upper case characters (as defined by unicode.IsUpper).
+    // Conventionally, multiword keys are written with the words separated by hyphens,
+    // as in cpu-speed.
+    Configuration map[string]string
 
-  Coverage  *CoverageResult  `json:",omitempty"`
+    // Package is a full name of the package containing the test/benchmark.
+    // It is zero iff Name is zero.
+    Package string  `json:",omitempty"
+    // Name is the name of the test/benchmark that this JSONResult is about.
+    // It can be empty if JSONResult describes global state, such as
+    // Configuration or Stdout/Stderr.
+    Name    string  `json:",omitempty"
+    // State is the current state of the test/benchmark.
+    // It is non-zero iff Name is non-zero.
+    State   State   `json:",omitempty"
+    // Procs is the value of runtime.GOMAXPROCS for this test/benchmark run.
+    // It is specified only in the first JSONResult of a test/benchmark.
+    Procs   int     `json:",omitempty"
+    // Log is log created by calling Log or Logf functions of *T or *B.
+    // A JSONResult with Log is emitted by go test as soon as possible.
+    // First occurrence of test/benchmark does not contain logs.
+    Log     string  `json:",omitempty"
+
+    // Benchmark contains benchmark-specific details.
+    // It is emitted in the final JSONResult of a benchmark with a terminal
+    // State if the benchmark does not have sub-benchmarks.
+    Benchmark *BenchmarkResult  `json:",omitempty"
+
+    // CoverageMode is coverage mode that was used to run these tests.
+    CoverageMode     string    `json:",omitempty"
+    // TotalStatements is the number of statements checked for coverage.
+    TotalStatements  int64     `json:",omitempty"
+    // ActiveStatements is the number of statements covered by tests, examples
+    // or benchmarks.
+    ActiveStatements int64     `json:",omitempty"
+    // CoveragedPackages is full names of packages included in coverage.
+    CoveredPackages  []string  `json:",omitempty"
+
+    // Stdout is text written by the test binary directly to os.Stdout.
+    // If this field is non-zero, all others are zero.
+    Stdout string  `json:",omitempty"
+    // Stderr is text written by test binary directly to os.Stderr.
+    // If this field is non-zero, all others are zero.
+    Stderr string  `json:",omitempty"
 }
 ```
 
-Example of a test binary stdout (JSON output is made indented for the
-convenience of the reader. It will be unindented in fact):
+### Example output
+
+Here is an example of `go test -json` output.
+It is simplified and commented for the convenience of the reader;
+in practice it will be unindented and will contain JSON Text Sequence separators
+and no comments.
 
 ```json
-
+// go test emits environment configuration
 {
+    "Configuration": {
+        "commit": "7cd9055",
+        "commit-time": "2016-02-11T13:25:45-0500",
+        "goos": "darwin",
+        "goarch": "amd64",
+        "cpu": "Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz",
+        "cpu-count": "8",
+        "cpu-physical-count": "4",
+        "os": "Mac OS X 10.11.3",
+        "mem": "16 GB"
+    }
+}
+// TestFoo started
+{
+    "Package": "github.com/user/repo",
     "Name": "TestFoo",
     "State": "RUN",
+    "Procs": 4
 }
-Random string written directly to os.Stdout.
+// A line was written directly to os.Stdout
 {
+    "Package": "github.com/user/repo",
+    "Stderr": "Random string written directly to os.Stdout\n"
+}
+// TestFoo passed
+{
+    "Package": "github.com/user/repo",
     "Name": "TestFoo",
     "State": "PASS",
-    "T": 1000000
 }
-
+// TestBar started
 {
+    "Package": "github.com/user/repo",
     "Name": "TestBar",
     "State": "RUN",
+    "Procs": 4
 }
-
+// TestBar logged a line
 {
+    "Package": "github.com/user/repo",
     "Name": "TestBar",
-    "State": "PASS",
-    "T": 1000000,
-    "Log": "some test output\n"
+    "State": "RUN",
+    "Log": "some test output"
 }
-
+// TestBar failed
 {
+    "Package": "github.com/user/repo",
+    "Name": "TestBar",
+    "State": "FAIL"
+}
+// TestQux started
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestQux",
+    "State": "RUN",
+    "Procs": 4
+}
+// TestQux calls T.Fatal("bug")
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestBar",
+    "State": "RUN",
+    "Log": "bug"
+}
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestQux",
+    "State": "FAIL"
+}
+// TestComposite started
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestComposite",
+    "State": "RUN",
+    "Procs": 4
+}
+// TestComposite/A=1 subtest started
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestComposite/A=1",
+    "State": "RUN",
+    "Procs": 4
+}
+// TestComposite/A=1 passed
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestComposite/A=1",
+    "State": "PASS",
+}
+// TestComposite passed
+{
+    "Package": "github.com/user/repo",
+    "Name": "TestComposite",
+    "State": "PASS",
+}
+// Example1 started
+{
+    "Package": "github.com/user/repo",
     "Name": "Example1",
     "State": "RUN",
+    "Procs": 4
 }
-
+// Example1 passed
 {
+    "Package": "github.com/user/repo",
     "Name": "Example1",
-    "State": "PASS",
-    "T": 1000000,
+    "State": "PASS"
 }
-
+// BenchmarkRun started
 {
+    "Package": "github.com/user/repo",
     "Name": "BenchmarkBar",
     "State": "RUN",
+    "Procs": 4
 }
-
+// BenchmarkRun passed
 {
+    "Package": "github.com/user/repo",
     "Name": "BenchmarkBar",
     "State": "PASS",
-    "T": 1000000,
-    "N": 1000,
-    "Bytes": 100,
-    "MemAllocs": 10,
-    "MemBytes": 10
-}
-
-{
-    "Coverage": {
-        "Mode": "set",
-        "TotalStatements":  1000,
-        "ActiveStatements": 900,
-        "CoveredPackages": [
-            "example.com/foobar"
-        ]
-    }
-}
-```
-
-### `go test`
-
-`go test` JSON output format:
-
-```go
-// TestResult contains one output line of a test binary.
-type TestResult struct {
-  Package string // package of the test binary.
-
-  Result    *testing.JSONResult  `json:",omitempty"`
-  Stdout    string               `json:",omitempty"` // Unrecognized stdout of the test binary.
-  Stderr    string               `json:",omitempty"` // Stderr output line of the test binary.
-}
-```
-
-Example `go test -json` output:
-
-```json
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "TestFoo",
-        "State": "Run",
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Stdout": "Random string written directly to os.Stdout."
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "TestFoo",
-        "State": "PASS",
-        "T": 1000000
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "TestBar",
-        "State": "Run",
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "TestBar",
-        "State": "PASS",
-        "T": 1000000,
-        "Log": "some test output\n"
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "Example1",
-        "State": "Run",
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "Example1",
-        "State": "PASS",
-        "T": 1000000
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "BenchmarkBar",
-        "State": "Run",
-    }
-}
-{
-    "Package": "example.com/foobar",
-    "Result": {
-        "Name": "BenchmarkBar",
-        "State": "PASS",
-        "Procs": 8,
+    "Benchmark": {
         "T": 1000000,
         "N": 1000,
+        "Bytes": 100,
+        "MemAllocs": 10,
+        "MemBytes": 10
     }
 }
+// BenchmarkComposite started
 {
-    "Package": "example.com/foobar",
-    "Result": {
-        "Coverage": {
-            "Mode": "set",
-            "TotalStatements":  1000,
-            "ActiveStatements": 900,
-            "CoveredPackages": [
-                "example.com/foobar"
-            ]
-        }
+    "Package": "github.com/user/repo",
+    "Name": "BenchmarkComposite",
+    "State": "RUN",
+    "Procs": 4
+}
+// BenchmarkComposite/A=1 started
+{
+    "Package": "github.com/user/repo",
+    "Name": "BenchmarkComposite/A=1",
+    "State": "RUN",
+    "Procs": 4
+}
+// BenchmarkComposite/A=1 passed
+{
+    "Package": "github.com/user/repo",
+    "Name": "BenchmarkComposite/A=1",
+    "State": "PASS",
+    "Benchmark": {
+        "T": 1000000,
+        "N": 1000,
+        "Bytes": 100,
+        "MemAllocs": 10,
+        "MemBytes": 10
     }
 }
-
+// BenchmarComposite passed
+{
+    "Package": "github.com/user/repo",
+    "Name": "BenchmarComposite",
+    "State": "PASS"
+}
+// Total coverage information in the end.
+{
+    "CoverageMode": "set",
+    "TotalStatements":  1000,
+    "ActiveStatements": 900,
+    "CoveredPackages": [
+        "github.com/user/repo"
+    ]
+}
 ```
 
 ## Rationale
 
-*   A test binary surrounds `testing.JSONResult` JSON with `\n` to handle the
-    situation when a string without a trailing `\n` is printed directly to
-    `os.Stdout`.
-*   A test binary always streams results so we don't loose them if the binary
-    panics.
-
 Alternatives:
 
 *   Add `-format` and `-benchformat` flags proposed in
-    https://github.com/golang/go/issues/12826. This is simpler to implement
-    by moving the burden of output parsing to third party programs.
+    https://github.com/golang/go/issues/12826.
+    While this is simpler to implement, users will have to do more work to
+    specify format and then parse it.
 
 Trade offs:
 
-*   `testing.JSONResult` is used for tests, examples and benchmarks.
-    A third party tool would have to determine the type of the result by the
-    prefix of `"Name"` property, e.g. tests always start with `"Test"`.
-    This is a trade off for simplicity of `testing` package API.
-
-    Alternatives:
-
-    *   add `type TestResult`, which together with
-        `BenchmarkResult` would have duplicated fields, such as `Name`,
-        `State`, `Procs`, `T`, `Log`.
-        We cannot add `type CommonResult` with common fields and embed it in
-        `TestResult` and `BenchmarkResult` because it would break backwards
-        compatibility of `BenchmarkResult`.
-    *   Duplicate fields in `TestResult` but make `TestResult` and any other
-        JSON-output-related types internal.
-        The problem is that third party tool authors would have to write structs
-        for JSON parsing themselves.
-
 *   I propose to make `-json` mutually exclusive with `-n` and `-x` flags.
-    This is a trade off for `go test` output format simplicity.
-    Supporting `-json` with `-n`/`-x` flags would require a new field in
-    `TestResult` that would contain commands that have been run. Note that
-    we cannot print commands to stdout because stdout must be valid JSON.
+    These flags belong to `go build` subcommand while this proposal is scoped
+    to `go test`.
+    Supporting the flags would require adding JSON output knowledge to
+    `go/build.go`.
+*   `JSONResult.Benchmark.T` provides duration of a benchmark run, but there is
+    not an equivalent for a test run.
+    This is a trade off for `JSONResult` simplicity.
+    We don't have to define `TestResult` because `JSONResult` is enough to
+    describe a test result.
 
-    Supporting `-json` with `-n`/`-x` flags would also raise the question
-    whether the field must be specific to commands or it should contain anything
-    `build.go` prints to stdout.
-    At this time `-n` and `-x` are the only flags that cause `build.go`
-    to print to stdout, so we can avoid the problem for now.
+    Currently `go test` does not provide test timing info, so the proposal is
+    consistent with the current `go test` output.
 
-    If we add more output to `build.go` in future, we can add
-    `BuildOutput string` field to `TestResult` for arbitrary `build.go` output.
-
-    I propose not to add `BuildOutput` now because `-n` affects `go test` too.
-    For example, `go test -n` prints a command to run the test binary, which
-    should not be a part of `BuildOutput` (because it is not build).
-*   `go test` always streams and does not aggregate results into one JSON
-    object.
-    This is a trade off for `go test -json` output format simplicity.
 
 ## Compatibility
 
@@ -347,22 +349,8 @@
 
 Most of the work would be done by the author of this proposal.
 
-Implementation steps:
-
-1.  Add `type State` and add new fields to `testing.BenchmarkResult`.
-    Modify `testing.(*B).launch` to fill the new fields.
-1.  Add `-test.json` flag, `type CoverageResult` and `type JSONResult` to the
-    `testing` package.
-    Modify `(*T).report`, `RunBenchmarks`, `coverReport` and `runExample`
-    functions to print JSON if `-test.json` is specified.
-    If `-test.verbose` is specified, print verbose messages to stderr.
-1.  Add `-json` flag to `go test`.
-    If specified, pass `-test.json` to test binaries.
-
-    For each line in a test binary output, try to parse it as
-    `testing.JSONResult`, and print a `TestResult`.
-
 The goal is to get agreement on this proposal and to complete the work
-before the 1.6 freeze date.
+before the 1.8 freeze date.
 
 [testStreamOutput]: https://github.com/golang/go/blob/0b248cea169a261cd0c2db8c014269cca5a170c4/src/cmd/go/test.go#L361-L369
+[rfc7464]: https://tools.ietf.org/html/rfc7464