From e2da1ad98541a56759a35afa743d5e0432041175 Mon Sep 17 00:00:00 2001 From: Michael Fridman Date: Mon, 4 Jul 2022 23:33:25 -0400 Subject: [PATCH] Improve markdown output formatting (#75) --- .github/workflows/ci.yml | 18 +++-- internal/app/console_writer.go | 67 +++++++++++++----- internal/app/table_failed.go | 120 ++++++++++++++++++++++++++------- internal/app/table_summary.go | 27 ++++---- internal/app/table_tests.go | 6 +- 5 files changed, 173 insertions(+), 65 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 60ac83f..f1ce991 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,7 @@ on: branches: - main pull_request: + types: [opened, synchronize, reopened] jobs: build: @@ -29,16 +30,13 @@ jobs: run: go build -v . - name: Run tests with GITHUB_STEP_SUMMARY - run: | - go test -count=1 -race ./... -json -coverpkg github.com/mfridman/tparse/parse | tee output.json | ./tparse -notests -follow -all - NO_COLOR=1 ./tparse -format markdown -file output.json -all -slow 5 > $GITHUB_STEP_SUMMARY + # Note the use of || true. This so the job doesn't fail at that line. We want to preserve -follow + # as part of the test output, but not output it to the summary page, which is done in the proceeding + # command when we parse the output.json file. + run: | + go test -count=1 -race ./... -json -coverpkg github.com/mfridman/tparse/parse \ + | tee output.json | ./tparse -notests -follow -all || true + ./tparse -format markdown -file output.json -all -slow 5 > $GITHUB_STEP_SUMMARY - name: Run tparse w/ std lib run: go test -count=1 fmt strings bytes bufio crypto log mime sort time -json -cover | ./tparse -follow -all - - # - name: Run tparse w/ std lib (GITHUB_STEP_SUMMARY) - # env: - # NO_COLOR: 1 - # run: | - # go test -count=1 fmt strings bytes bufio crypto log mime sort time -json -cover |\ - # ./tparse -format=markdown >> $GITHUB_STEP_SUMMARY diff --git a/internal/app/console_writer.go b/internal/app/console_writer.go index be9c68d..ddd392a 100644 --- a/internal/app/console_writer.go +++ b/internal/app/console_writer.go @@ -2,6 +2,8 @@ package app import ( "io" + "os" + "strconv" "github.com/charmbracelet/lipgloss" "github.com/muesli/termenv" @@ -27,15 +29,26 @@ type consoleWriter struct { yellow colorOptionFunc } -type colorOptionFunc func(s string, bold bool) string +type colorOptionFunc func(s string) string // newColor is a helper function to set the base color. func newColor(color lipgloss.TerminalColor) colorOptionFunc { - return func(text string, bold bool) string { - return lipgloss.NewStyle().Bold(bold).Foreground(color).Render(text) + return func(text string) string { + return lipgloss.NewStyle().Foreground(color).Render(text) } } +// newMarkdownColor is a helper function to set the base color for markdown. +func newMarkdownColor(s string) colorOptionFunc { + return func(text string) string { + return s + " " + text + } +} + +func noColor() colorOptionFunc { + return func(text string) string { return text } +} + func newConsoleWriter(w io.Writer, format OutputFormat, disableColor bool) *consoleWriter { if format == 0 { format = OutputFormatBasic @@ -44,18 +57,42 @@ func newConsoleWriter(w io.Writer, format OutputFormat, disableColor bool) *cons w: w, format: format, } - if disableColor { - cw.red = newColor(lipgloss.NoColor{}) - cw.green = newColor(lipgloss.NoColor{}) - cw.yellow = newColor(lipgloss.NoColor{}) - } else { - // TODO(mf): not sure why I have to do this. It's working just fine locally but in - // CI (GitHub Actions) it is not outputting with colors. - // https://github.com/charmbracelet/lipgloss/issues/74 - lipgloss.SetColorProfile(termenv.TrueColor) - cw.red = newColor(lipgloss.Color("9")) - cw.green = newColor(lipgloss.Color("10")) - cw.yellow = newColor(lipgloss.Color("11")) + cw.red = noColor() + cw.green = noColor() + cw.yellow = noColor() + + if !disableColor { + // NOTE(mf): The GitHub Actions CI env (and probably others) does not have an + // interactive TTY, and tparse will degrade to the "best available option" .. + // which is no colors. We can work around this by setting the color profile + // manually instead of relying on it to auto-detect. + // Ref: https://github.com/charmbracelet/lipgloss/issues/74 + // + // TODO(mf): Should this be an explicit env variable instead? Such as TPARSE_FORCE_COLOR + // + // For now we best-effort the most common CI environments and set this manually. + if isCIEnvironment() { + lipgloss.SetColorProfile(termenv.TrueColor) + } + switch format { + case OutputFormatMarkdown: + cw.green = newMarkdownColor("🟢") + cw.yellow = newMarkdownColor("🟡") + cw.red = newMarkdownColor("🔴") + default: + cw.green = newColor(lipgloss.Color("10")) + cw.yellow = newColor(lipgloss.Color("11")) + cw.red = newColor(lipgloss.Color("9")) + } } return cw } + +func isCIEnvironment() bool { + if s := os.Getenv("CI"); s != "" { + if ok, err := strconv.ParseBool(s); err == nil && ok { + return true + } + } + return false +} diff --git a/internal/app/table_failed.go b/internal/app/table_failed.go index 18408a0..b28a8a2 100644 --- a/internal/app/table_failed.go +++ b/internal/app/table_failed.go @@ -16,7 +16,7 @@ func (c *consoleWriter) printFailed(packages []*parse.Package) { if pkg.HasPanic { // TODO(mf): document why panics are handled separately. A panic may or may // not be associated with tests, so we print it at the package level. - output := prepareStyledPanic(pkg.Summary.Package, pkg.Summary.Test, pkg.PanicEvents) + output := c.prepareStyledPanic(pkg.Summary.Package, pkg.Summary.Test, pkg.PanicEvents) fmt.Fprintln(c.w, output) continue } @@ -24,10 +24,9 @@ func (c *consoleWriter) printFailed(packages []*parse.Package) { if len(failedTests) == 0 { continue } - - styledPackageHeader := styledHeader( - strings.ToUpper(pkg.Summary.Action.String()), - strings.TrimSpace(pkg.Summary.Package), + styledPackageHeader := c.styledHeader( + pkg.Summary.Action.String(), + pkg.Summary.Package, ) fmt.Fprintln(c.w, styledPackageHeader) fmt.Fprintln(c.w) @@ -43,9 +42,29 @@ func (c *consoleWriter) printFailed(packages []*parse.Package) { divider := lipgloss.NewStyle(). BorderStyle(lipgloss.NormalBorder()). BorderTop(true). - Faint(true). + Faint(c.format != OutputFormatMarkdown). Width(96) + /* + Note, some output such as the "--- FAIL: " line is prefixed + with spaces. Unfortunately when dumping this in markdown format + it renders as an code block. + + "To produce a code block in Markdown, simply indent every line of the + block by at least 4 spaces or 1 tab." + Ref. https://daringfireball.net/projects/markdown/syntax + + Example: + --- FAIL: Test (0.05s) + --- FAIL: Test/test_01 (0.01s) + --- FAIL: Test/test_01/sort (0.00s) + + This is why we wrap the entire test output in a code block. + */ + + if c.format == OutputFormatMarkdown { + fmt.Fprintln(c.w, fencedCodeBlock) + } var key string for i, t := range failedTests { // Add top divider to all tests except first one. @@ -54,11 +73,18 @@ func (c *consoleWriter) printFailed(packages []*parse.Package) { fmt.Fprintln(c.w, divider.String()) } key = base - fmt.Fprintln(c.w, prepareStyledTest(t)) + fmt.Fprintln(c.w, c.prepareStyledTest(t)) + } + if c.format == OutputFormatMarkdown { + fmt.Fprint(c.w, fencedCodeBlock+"\n\n") } } } +const ( + fencedCodeBlock string = "```" +) + // copied directly from strings.Cut (go1.18) to support older Go versions. // In the future, replace this with the upstream function. func cut(s, sep string) (before, after string, found bool) { @@ -68,14 +94,15 @@ func cut(s, sep string) (before, after string, found bool) { return s, "", false } -func prepareStyledPanic(packageName, testName string, panicEvents []*parse.Event) string { +func (c *consoleWriter) prepareStyledPanic( + packageName string, + testName string, + panicEvents []*parse.Event, +) string { if testName != "" { packageName = packageName + " • " + testName } - styledPackageHeader := styledHeader( - "PANIC", - packageName, - ) + styledPackageHeader := c.styledHeader("PANIC", packageName) // TODO(mf): can we pass this panic stack to another package and either by default, // or optionally, build human-readable panic output with: // https://github.com/maruel/panicparse @@ -89,20 +116,35 @@ func prepareStyledPanic(packageName, testName string, panicEvents []*parse.Event return lipgloss.JoinVertical(lipgloss.Left, styledPackageHeader, rows.String()) } -// styledHeader styles a header based on the status and package name: -// -// ╭───────────────────────────────────────────────────────────╮ -// │ PANIC package: github.com/pressly/goose/v3/tests/e2e │ -// ╰───────────────────────────────────────────────────────────╯ -// -func styledHeader(status, packageName string) string { +func (c *consoleWriter) styledHeader(status, packageName string) string { + status = c.red(strings.ToUpper(status)) + packageName = strings.TrimSpace(packageName) + + if c.format == OutputFormatMarkdown { + msg := fmt.Sprintf("## %s • %s", status, packageName) + return msg + // TODO(mf): an alternative implementation is to add 2 horizontal lines above and below + // the package header output. + // + // var divider string + // for i := 0; i < len(msg); i++ { + // divider += "─" + // } + // return fmt.Sprintf("%s\n%s\n%s", divider, msg, divider) + } + /* + Need to rethink how to best support multiple output formats across + CI, local terminal development and markdown + + See https://github.com/mfridman/tparse/issues/71 + */ headerStyle := lipgloss.NewStyle(). BorderStyle(lipgloss.ThickBorder()). BorderForeground(lipgloss.Color("103")) statusStyle := lipgloss.NewStyle(). - Foreground(lipgloss.Color("9")). PaddingLeft(3). - PaddingRight(2) + PaddingRight(2). + Foreground(lipgloss.Color("9")) packageNameStyle := lipgloss.NewStyle(). PaddingRight(3) headerRow := lipgloss.JoinHorizontal( @@ -113,7 +155,11 @@ func styledHeader(status, packageName string) string { return headerStyle.Render(headerRow) } -func prepareStyledTest(t *parse.Test) string { +const ( + failLine = "--- FAIL: " +) + +func (c *consoleWriter) prepareStyledTest(t *parse.Test) string { t.SortEvents() var rows, headerRows strings.Builder @@ -124,18 +170,42 @@ func prepareStyledTest(t *parse.Test) string { if e.Action != parse.ActionOutput { continue } - if strings.Contains(e.Output, "--- FAIL: ") { - header := lipgloss.NewStyle().Foreground(lipgloss.Color("1")).Render(e.Output) + if strings.Contains(e.Output, failLine) { + header := strings.TrimSuffix(e.Output, "\n") + // go test prefixes too much padding to the "--- FAIL: " output lines. + // Let's cut the padding by half, being careful to preserve the fail + // line and the proceeding output. + before, after, ok := cut(header, failLine) + var pad string + if ok { + var n int + for _, r := range before { + if r == 32 { + n++ + } + } + for i := 0; i < n/2; i++ { + pad += " " + } + } + header = pad + failLine + after + + // Avoid colorizing markdown output so it renders properly, otherwise add a subtle + // red color to the test headers. + if c.format != OutputFormatMarkdown { + header = lipgloss.NewStyle().Foreground(lipgloss.Color("1")).Render(header) + } headerRows.WriteString(header) continue } + if e.Output != "" { rows.WriteString(e.Output) } } out := headerRows.String() if rows.Len() > 0 { - out += "\n" + rows.String() + out += "\n\n" + rows.String() } return out } diff --git a/internal/app/table_summary.go b/internal/app/table_summary.go index 68238c7..d80cb33 100644 --- a/internal/app/table_summary.go +++ b/internal/app/table_summary.go @@ -46,7 +46,7 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool packageName := pkg.Summary.Package if pkg.HasPanic { row := summaryRow{ - status: c.red("PANIC", true), + status: c.red("PANIC"), elapsed: elapsed, packageName: packageName, cover: "--", pass: "--", fail: "--", skip: "--", @@ -56,7 +56,7 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool } if pkg.HasFailedBuildOrSetup { row := summaryRow{ - status: c.red("FAIL", true), + status: c.red("FAIL"), elapsed: elapsed, packageName: packageName + "\n[" + pkg.Summary.Output + "]", cover: "--", pass: "--", fail: "--", skip: "--", @@ -66,7 +66,7 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool } if pkg.NoTestFiles { row := summaryRow{ - status: c.yellow("NOTEST", true), + status: c.yellow("NOTEST"), elapsed: elapsed, packageName: packageName + "\n[no test files]", cover: "--", pass: "--", fail: "--", skip: "--", @@ -78,7 +78,7 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool // This should capture cases where packages truly have no tests, but empty files. if len(pkg.NoTestSlice) == 0 { row := summaryRow{ - status: c.yellow("NOTEST", true), + status: c.yellow("NOTEST"), elapsed: elapsed, packageName: packageName + "\n[no tests to run]", cover: "--", pass: "--", fail: "--", skip: "--", @@ -94,7 +94,7 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool } packageName := fmt.Sprintf("%s\n[no tests to run]\n%s", packageName, strings.Join(ss, "\n")) row := summaryRow{ - status: c.yellow("NOTEST", true), + status: c.yellow("NOTEST"), elapsed: elapsed, packageName: packageName, cover: "--", pass: "--", fail: "--", skip: "--", @@ -109,14 +109,17 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool coverage := "--" if pkg.Cover { coverage = fmt.Sprintf("%.1f%%", pkg.Coverage) - if pkg.Summary.Action != parse.ActionFail { + // Showing coverage for a package that failed is a bit odd. + // + // Only colorize the coverage when everything passed AND the output is not markdown. + if pkg.Summary.Action == parse.ActionPass && c.format != OutputFormatMarkdown { switch cover := pkg.Coverage; { case cover > 0.0 && cover <= 50.0: - coverage = c.red(coverage, false) + coverage = c.red(coverage) case pkg.Coverage > 50.0 && pkg.Coverage < 80.0: - coverage = c.yellow(coverage, false) + coverage = c.yellow(coverage) case pkg.Coverage >= 80.0: - coverage = c.green(coverage, false) + coverage = c.green(coverage) } } } @@ -124,11 +127,11 @@ func (c *consoleWriter) summaryTable(packages []*parse.Package, showNoTests bool status := strings.ToUpper(pkg.Summary.Action.String()) switch pkg.Summary.Action { case parse.ActionPass: - status = c.green(status, false) + status = c.green(status) case parse.ActionSkip: - status = c.yellow(status, false) + status = c.yellow(status) case parse.ActionFail: - status = c.red(status, false) + status = c.red(status) } row := summaryRow{ status: status, diff --git a/internal/app/table_tests.go b/internal/app/table_tests.go index 9336f4a..7af3691 100644 --- a/internal/app/table_tests.go +++ b/internal/app/table_tests.go @@ -91,11 +91,11 @@ func (c *consoleWriter) testsTable(packages []*parse.Package, option TestTableOp status := strings.ToUpper(t.Status().String()) switch t.Status() { case parse.ActionPass: - status = c.green(status, false) + status = c.green(status) case parse.ActionSkip: - status = c.yellow(status, false) + status = c.yellow(status) case parse.ActionFail: - status = c.red(status, false) + status = c.red(status) } dir, packageName := path.Split(t.Package)