Code cleanup; update comments and TODOs

main.go

@@ -5,7 +5,6 @@ import (
 	"flag"
 	"fmt"
 	"io"
-	"log"
 	"os"
 	"sort"
 	"strings"
@@ -34,11 +33,11 @@ var usage = `Usage:
 Options:
 	-h		Show help.
 	-v		Show version.
-	-all		Display all event types: pass, skip and fail. (Failed items are always displayed)
-	-pass		Display all passed tests.
-	-skip		Display all skipped tests.
-	-notests	Display packages with no tests in summary.
-	-dump		Dump raw go test output; enables recovering original go test output in non-JSON format.
+	-all		Display table event for pass, skip and fail. (Failed items are always displayed)
+	-pass		Display table for passed tests.
+	-skip		Display table for passed skipped tests.
+	-notests	Display packages containing no test files in summary.
+	-dump		Enables recovering initial go test output in non-JSON format following Summary and Test tables.
 `
 
 func main() {
@@ -53,8 +52,6 @@ func main() {
 		os.Exit(0)
 	}
 
-	log.SetFlags(0)
-
 	r, err := getReader()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "Error: %v\n\n", err)
@@ -63,23 +60,34 @@ func main() {
 	defer r.Close()
 
 	pkgs, err := parse.Start(r)
+	// TODO(mf): no matter what error we get, we should always allow the user to retrieve
+	// whatever we could read in Start with -dump. Currently it only gets called way below.
 	if err != nil {
 		switch err := errors.Cause(err).(type) {
 		case *json.SyntaxError:
 			fmt.Fprint(os.Stderr, "Error: must call go test with -json flag\n\n")
 			flag.Usage()
 		case *parse.PanicErr:
+			// Just return the package name, test name and debug info from the panic.
 			err.PrintPanic()
 			os.Exit(1)
 		default:
+			// TODO(mf):
+			// - Does it make sense to display error and usage
+			// back to the user when there is a scan error?
 			fmt.Fprintf(os.Stderr, "Error: %v\n\n", err)
 			flag.Usage()
 		}
 	}
 
+	if len(pkgs) == 0 {
+		parse.RawDump()
+		os.Exit(0)
+	}
+
 	// Prints packages summary table.
 	// TODO: think about using functional options?
-	pkgs.Print(*noTestsPtr)
+	pkgs.PrintSummary(*noTestsPtr)
 
 	// Print all failed tests per package (if any).
 	for _, p := range pkgs {
@@ -162,7 +170,7 @@ func getReader() (io.ReadCloser, error) {
 			return nil, err
 		}
 
-		// check file mode bits to test for named pipe as stdin
+		// Check file mode bits to test for named pipe as stdin.
 		if finfo.Mode()&os.ModeNamedPipe != 0 {
 			return os.Stdin, nil
 		}

parse/event.go

@@ -11,15 +11,6 @@ import (
 	"github.com/pkg/errors"
 )
 
-func NewEvent(data []byte) (*Event, error) {
-	var ev Event
-	if err := json.Unmarshal(data, &ev); err != nil {
-		return nil, errors.Wrap(err, "failed to unmarshal test event")
-	}
-
-	return &ev, nil
-}
-
 // Event represents a single line of json output from go test with the -json flag.
 //
 // For more info see, https://golang.org/cmd/test2json and
@@ -51,16 +42,24 @@ type Event struct {
 	Elapsed float64
 }
 
+func NewEvent(data []byte) (*Event, error) {
+	var ev Event
+	if err := json.Unmarshal(data, &ev); err != nil {
+		return nil, errors.Wrap(err, "failed to unmarshal test event")
+	}
+
+	return &ev, nil
+}
+
 // Events groups emitted events by test name. All events must belong to a single test
 // and thus a single package.
 type Events []*Event
 
-// Discard reports whether an "output":
-// 1. has no test name (with the exception of the skip line)
-// 2. has test name but is an update: RUN, PAUSE, CONT.
+// Discard reports whether an "output" action:
+// 1. has test name but is an update action: RUN, PAUSE, CONT.
+// 2. has no test name
 //
-// It might be possible folks want to know how often parallel
-// tests are switched (potential feature request?).
+// If output is not one of the above return false.
 func (e *Event) Discard() bool {
 	u := []string{
 		"=== RUN",

parse/package.go

@@ -1,97 +1,9 @@
 package parse
 
 import (
-	"bufio"
-	"bytes"
-	"fmt"
-	"io"
-	"os"
-	"strconv"
 	"strings"
-
-	"github.com/olekukonko/tablewriter"
-	"github.com/pkg/errors"
 )
 
-var rawdump bytes.Buffer
-
-// RawDump returns original lines of output from go test.
-func RawDump() {
-	sc := bufio.NewScanner(&rawdump)
-	for sc.Scan() {
-		e, err := NewEvent(sc.Bytes())
-		if err != nil {
-			fmt.Fprintf(os.Stderr, "tparse new event error: %v\n", err)
-			continue
-		}
-		fmt.Fprint(os.Stderr, e.Output)
-	}
-	if err := sc.Err(); err != nil {
-		fmt.Fprintf(os.Stderr, "tparse scan error: %v\n", err)
-		return
-	}
-}
-
-// Packages is a collection of packages being tested.
-// TODO: consider changing this to a slice of packages instead of a map?
-// - would make it easier sorting the summary box by elapsed time
-// - would make it easier adding functional options.
-type Packages map[string]*Package
-
-func (p Packages) Print(skipNoTests bool) {
-	if len(p) == 0 {
-		return
-	}
-
-	tbl := tablewriter.NewWriter(os.Stdout)
-
-	tbl.SetHeader([]string{
-		"Status",
-		"Elapsed",
-		"Package",
-		"Cover",
-		"Pass",
-		"Fail",
-		"Skip",
-	})
-
-	for name, pkg := range p {
-
-		if pkg.NoTest {
-			if !skipNoTests {
-				continue
-			}
-
-			tbl.Append([]string{
-				Yellow("SKIP"),
-				"0.00s",
-				name + "\n[no test files]",
-				fmt.Sprintf(" %.1f%%", pkg.Coverage),
-				"0", "0", "0",
-			})
-
-			continue
-		}
-
-		if pkg.Cached {
-			name += " (cached)"
-		}
-
-		tbl.Append([]string{
-			pkg.Summary.Action.WithColor(),
-			strconv.FormatFloat(pkg.Summary.Elapsed, 'f', 2, 64) + "s",
-			name,
-			fmt.Sprintf(" %.1f%%", pkg.Coverage),
-			strconv.Itoa(len(pkg.TestsByAction(ActionPass))),
-			strconv.Itoa(len(pkg.TestsByAction(ActionFail))),
-			strconv.Itoa(len(pkg.TestsByAction(ActionSkip))),
-		})
-	}
-
-	tbl.Render()
-	fmt.Printf("\n")
-}
-
 // Package is the representation of a single package being tested. The
 // summary field is an event that contains all relevant information about the
 // package, namely Package (name), Elapsed and Action (big pass or fail).
@@ -111,8 +23,8 @@ type Package struct {
 	Coverage float64
 }
 
-// AddTestEvent adds the event to a test based on test name.
-func (p *Package) AddTestEvent(event *Event) {
+// AddEvent adds the event to a test based on test name.
+func (p *Package) AddEvent(event *Event) {
 	for _, t := range p.Tests {
 		if t.Name == event.Test {
 			t.Events = append(t.Events, event)
@@ -129,88 +41,6 @@ func (p *Package) AddTestEvent(event *Event) {
 	p.Tests = append(p.Tests, t)
 }
 
-// Start will return PanicErr on the first package that reports a test containing a panic.
-func Start(r io.Reader) (Packages, error) {
-
-	pkgs := Packages{}
-
-	var scan bool
-	var badLine int
-
-	rd := io.TeeReader(r, &rawdump)
-
-	sc := bufio.NewScanner(rd)
-	for sc.Scan() {
-
-		// We'll "prescan" up-to 50 lines for a parsable event. If we do get a parsable event
-		// we expect no errors to follow until EOF. For each unparsable event we will dump back
-		// the up to 50 lines to stderr.
-		e, err := NewEvent(sc.Bytes())
-		if err != nil {
-			if scan || badLine >= 50 {
-				return nil, err
-			}
-
-			fmt.Fprintln(os.Stderr, sc.Text())
-			badLine++
-			continue
-		}
-		scan = true
-
-		pkg, ok := pkgs[e.Package]
-		if !ok {
-			pkg = &Package{Summary: &Event{}}
-			pkgs[e.Package] = pkg
-		}
-
-		if e.SkipLine() {
-			pkg.Summary = &Event{Action: ActionPass}
-			pkg.NoTest = true
-		}
-
-		if e.IsCached() {
-			pkg.Cached = true
-		}
-
-		cover, ok := e.Cover()
-		if ok {
-			pkg.Cover = true
-			pkg.Coverage = cover
-		}
-
-		if e.Summary() {
-			pkg.Summary = e
-			continue
-		}
-
-		// We don't need to save these line.
-		if e.Discard() {
-			continue
-		}
-
-		pkg.AddTestEvent(e)
-
-	}
-
-	if err := sc.Err(); err != nil {
-		// TODO: FIXME: something went wrong scanning. We may want to fail? and dump
-		// what we were able to read.
-		// E.g., store events in strings.Builder and dump the output lines,
-		// or return a structured error with context and events we were able to read.
-		return nil, errors.Wrap(err, "bufio scanner error")
-	}
-
-	// Panic means end of the world, don't return a summary, no table tests, etc.
-	// Just return the package, test and debug info from the panic inside the error.
-	for _, pkg := range pkgs {
-		if err := pkg.HasPanic(); err != nil {
-			return nil, err
-		}
-	}
-
-	return pkgs, nil
-}
-
 // HasPanic reports whether a package contains a test that panicked.
 // A PanicErr is returned if a test contains a panic.
 func (p *Package) HasPanic() error {

parse/packages.go

@@ -0,0 +1,62 @@
+package parse
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+
+	"github.com/olekukonko/tablewriter"
+)
+
+// Packages is a collection of packages being tested.
+type Packages map[string]*Package
+
+func (p Packages) PrintSummary(skipNoTests bool) {
+	tbl := tablewriter.NewWriter(os.Stdout)
+
+	tbl.SetHeader([]string{
+		"Status",
+		"Elapsed",
+		"Package",
+		"Cover",
+		"Pass",
+		"Fail",
+		"Skip",
+	})
+
+	for name, pkg := range p {
+
+		if pkg.NoTest {
+			if !skipNoTests {
+				continue
+			}
+
+			tbl.Append([]string{
+				Yellow("SKIP"),
+				"0.00s",
+				name + "\n[no test files]",
+				fmt.Sprintf(" %.1f%%", pkg.Coverage),
+				"0", "0", "0",
+			})
+
+			continue
+		}
+
+		if pkg.Cached {
+			name += " (cached)"
+		}
+
+		tbl.Append([]string{
+			pkg.Summary.Action.WithColor(),
+			strconv.FormatFloat(pkg.Summary.Elapsed, 'f', 2, 64) + "s",
+			name,
+			fmt.Sprintf(" %.1f%%", pkg.Coverage),
+			strconv.Itoa(len(pkg.TestsByAction(ActionPass))),
+			strconv.Itoa(len(pkg.TestsByAction(ActionFail))),
+			strconv.Itoa(len(pkg.TestsByAction(ActionSkip))),
+		})
+	}
+
+	tbl.Render()
+	fmt.Printf("\n")
+}