From eb17108b432315d961a4def4c11a82339c6db69d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Nov 2022 18:39:48 -0800 Subject: [PATCH 1/4] testutil: Add ParseTestCaseFile This moves the logic for parsing a test case file into a separate function named ParseTestCaseFile. This will make it easier for extensions to use the same format for test files, even if they want a different strategy to run them. The function does not do filtering like DoTestCaseFile; that logic has been left inside DoTestCaseFile. Minus that, this function does not modify any logic. The next commits do. --- testutil/testutil.go | 69 +++++++++++++++++++++++++-- testutil/testutil_test.go | 99 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 163 insertions(+), 5 deletions(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index f7cb312..d343639 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -62,8 +62,10 @@ type MarkdownTestCaseOptions struct { Trim bool } -const attributeSeparator = "//- - - - - - - - -//" -const caseSeparator = "//= = = = = = = = = = = = = = = = = = = = = = = =//" +const ( + attributeSeparator = "//- - - - - - - - -//" + caseSeparator = "//= = = = = = = = = = = = = = = = = = = = = = = =//" +) var optionsRegexp *regexp.Regexp = regexp.MustCompile(`(?i)\s*options:(.*)`) @@ -84,8 +86,51 @@ func ParseCliCaseArg() []int { return ret } -// DoTestCaseFile runs test cases in a given file. -func DoTestCaseFile(m goldmark.Markdown, filename string, t TestingT, no ...int) { +// ParseTestCaseFile parses the contents of the given test case file +// and reurns the test cases found inside. +// +// The file should contain zero or more test cases, each in the form: +// +// NUM[:DESC] +// [OPTIONS] +// //- - - - - - - - -// +// INPUT +// //- - - - - - - - -// +// OUTPUT +// //= = = = = = = = = = = = = = = = = = = = = = = =// +// +// Where, +// +// - NUM is a test case number +// - DESC is an optional description +// - OPTIONS, if present, is a JSON object +// - INPUT is the input Markdown +// - OUTPUT holds the expected result from the processor. +// +// Basic example: +// +// 3 +// //- - - - - - - - -// +// Hello, **world**. +// //- - - - - - - - -// +//

Hello, world

+// //= = = = = = = = = = = = = = = = = = = = = = = =// +// +// Example of a description: +// +// 3: supports bold text +// //- - - - - - - - -// +// Hello, **world**. +// [..] +// +// Example of options: +// +// 3: supports bold text +// OPTIONS: {"trim": true} +// //- - - - - - - - -// +// Hello, **world**. +// [..] +func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { fp, err := os.Open(filename) if err != nil { panic(err) @@ -158,6 +203,22 @@ func DoTestCaseFile(m goldmark.Markdown, filename string, t TestingT, no ...int) if len(c.Expected) != 0 { c.Expected = c.Expected + "\n" } + + cases = append(cases, c) + } + + return cases, nil +} + +// DoTestCaseFile runs test cases in a given file. +func DoTestCaseFile(m goldmark.Markdown, filename string, t TestingT, no ...int) { + allCases, err := ParseTestCaseFile(filename) + if err != nil { + panic(err) + } + + cases := allCases[:0] + for _, c := range allCases { shouldAdd := len(no) == 0 if !shouldAdd { for _, n := range no { diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index 2000a00..87e0975 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -1,7 +1,104 @@ package testutil -import "testing" +import ( + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) // This will fail to compile if the TestingT interface is changed in a way // that doesn't conform to testing.T. var _ TestingT = (*testing.T)(nil) + +func TestParseTestCaseFile(t *testing.T) { + tests := []struct { + desc string + give string // contents of the test file + want []MarkdownTestCase + }{ + { + desc: "empty", + give: "", + want: []MarkdownTestCase{}, + }, + { + desc: "simple", + give: strings.Join([]string{ + "1", + "//- - - - - - - - -//", + "input", + "//- - - - - - - - -//", + "output", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + want: []MarkdownTestCase{ + { + No: 1, + Markdown: "input", + Expected: "output\n", + }, + }, + }, + { + desc: "description", + give: strings.Join([]string{ + "2:check something", + "//- - - - - - - - -//", + "hello", + "//- - - - - - - - -//", + "

hello

", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + want: []MarkdownTestCase{ + { + No: 2, + Description: "check something", + Markdown: "hello", + Expected: "

hello

\n", + }, + }, + }, + { + desc: "options", + give: strings.Join([]string{ + "3", + `OPTIONS: {"trim": true}`, + "//- - - - - - - - -//", + "world", + "//- - - - - - - - -//", + "

world

", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + want: []MarkdownTestCase{ + { + No: 3, + Options: MarkdownTestCaseOptions{Trim: true}, + Markdown: "world", + Expected: "

world

\n", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + filename := filepath.Join(t.TempDir(), "give.txt") + if err := os.WriteFile(filename, []byte(tt.give), 0o644); err != nil { + t.Fatal(err) + } + + got, err := ParseTestCaseFile(filename) + if err != nil { + t.Fatalf("could not parse: %v", err) + } + + if !reflect.DeepEqual(tt.want, got) { + t.Errorf("output did not match:") + t.Errorf(" got = %#v", got) + t.Errorf("want = %#v", tt.want) + } + }) + } +} From 282e1428bc1ceb935288986bb89424e4a501db3a Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Nov 2022 18:51:06 -0800 Subject: [PATCH 2/4] ParseTestCaseFile: Don't panic Instead of panicking, ParseTestCaseFile now reports errors. The errors take the form, line $line: $msg: $cause For example, line 12: invalid case No: parse error As a result of this change, we no longer discard the error returned by strconv.Atoi or json.Marshal when we reject the test file, and include it in the error message instead. Note that the errors do not include the file name because the file name is always the same so the caller can add that if necessary (which it will, in the next commit). --- testutil/testutil.go | 32 ++++++++++-- testutil/testutil_test.go | 101 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 5 deletions(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index d343639..4e529f3 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -86,6 +86,19 @@ func ParseCliCaseArg() []int { return ret } +type testCaseParseError struct { + Line int + Err error +} + +func (e *testCaseParseError) Error() string { + return fmt.Sprintf("line %v: %v", e.Line, e.Err) +} + +func (e *testCaseParseError) Unwrap() error { + return e.Err +} + // ParseTestCaseFile parses the contents of the given test case file // and reurns the test cases found inside. // @@ -133,7 +146,7 @@ func ParseCliCaseArg() []int { func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { fp, err := os.Open(filename) if err != nil { - panic(err) + return nil, err } defer fp.Close() @@ -147,6 +160,15 @@ func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { } cases := []MarkdownTestCase{} line := 0 + + // Builds a testCaseParseError for the curent line. + parseErrorf := func(msg string, args ...interface{}) error { + return &testCaseParseError{ + Line: line, + Err: fmt.Errorf(msg, args...), + } + } + for scanner.Scan() { line++ if util.IsBlank([]byte(scanner.Text())) { @@ -162,23 +184,23 @@ func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { c.No, err = strconv.Atoi(scanner.Text()) } if err != nil { - panic(fmt.Sprintf("%s: invalid case No at line %d", filename, line)) + return nil, parseErrorf("invalid case No: %w", err) } if !scanner.Scan() { - panic(fmt.Sprintf("%s: invalid case at line %d", filename, line)) + return nil, parseErrorf("invalid case: expected content after case No") } line++ matches := optionsRegexp.FindAllStringSubmatch(scanner.Text(), -1) if len(matches) != 0 { err = json.Unmarshal([]byte(matches[0][1]), &c.Options) if err != nil { - panic(fmt.Sprintf("%s: invalid options at line %d", filename, line)) + return nil, parseErrorf("invalid options: %w", err) } scanner.Scan() line++ } if scanner.Text() != attributeSeparator { - panic(fmt.Sprintf("%s: invalid separator '%s' at line %d", filename, scanner.Text(), line)) + return nil, parseErrorf("invalid separator %q", scanner.Text()) } buf := []string{} for scanner.Scan() { diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index 87e0975..541a993 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -1,6 +1,7 @@ package testutil import ( + "errors" "os" "path/filepath" "reflect" @@ -102,3 +103,103 @@ func TestParseTestCaseFile(t *testing.T) { }) } } + +func TestParseTestCaseFile_Errors(t *testing.T) { + tests := []struct { + desc string + give string // contents of the test file + errMsg string + }{ + { + desc: "bad number/no description", + give: strings.Join([]string{ + "1 not a number", + "//- - - - - - - - -//", + "world", + "//- - - - - - - - -//", + "

world

", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + errMsg: "line 1: invalid case No", + }, + { + desc: "bad number/description", + give: strings.Join([]string{ + "1 not a number:description", + "//- - - - - - - - -//", + "world", + "//- - - - - - - - -//", + "

world

", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + errMsg: "line 1: invalid case No", + }, + { + desc: "eof after number", + give: strings.Join([]string{ + "1", + }, "\n"), + errMsg: "line 1: invalid case: expected content after", + }, + { + desc: "bad options", + give: strings.Join([]string{ + "3", + `OPTIONS: {not valid JSON}`, + "//- - - - - - - - -//", + "world", + "//- - - - - - - - -//", + "

world

", + "//= = = = = = = = = = = = = = = = = = = = = = = =//", + }, "\n"), + errMsg: "line 2: invalid options:", + }, + { + desc: "bad separator", + give: strings.Join([]string{ + "3", + "// not the right separator //", + }, "\n"), + errMsg: `line 2: invalid separator "// not the right separator //"`, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + filename := filepath.Join(t.TempDir(), "give.txt") + if err := os.WriteFile(filename, []byte(tt.give), 0o644); err != nil { + t.Fatal(err) + } + + cases, err := ParseTestCaseFile(filename) + if err == nil { + t.Fatalf("expected error, got:\n%#v", cases) + } + + if got := err.Error(); !strings.Contains(got, tt.errMsg) { + t.Errorf("unexpected error message:") + t.Errorf(" got = %v", got) + t.Errorf("does not contain = %v", tt.errMsg) + } + }) + } +} + +func TestTestCaseParseError(t *testing.T) { + wrapped := errors.New("great sadness") + err := &testCaseParseError{Line: 42, Err: wrapped} + + t.Run("Error", func(t *testing.T) { + want := "line 42: great sadness" + got := err.Error() + if want != got { + t.Errorf("Error() = %q, want %q", got, want) + } + }) + + t.Run("Unwrap", func(t *testing.T) { + if !errors.Is(err, wrapped) { + t.Errorf("error %#v should unwrap to %#v", err, wrapped) + } + }) +} From 86794e96eea9852ad78c7c543b97c47b817b8398 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Nov 2022 19:05:20 -0800 Subject: [PATCH 3/4] DoTestCaseFile: Don't panic Instead of panicking, report the error in the form, $file: $msg So an error might look like, foo.txt: line 12: invalid case No: [..] And then kill the test with FailNow. This is the same as calling `t.Fatalf(...)`. --- testutil/testutil.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index 4e529f3..3404a5a 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -236,7 +236,8 @@ func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { func DoTestCaseFile(m goldmark.Markdown, filename string, t TestingT, no ...int) { allCases, err := ParseTestCaseFile(filename) if err != nil { - panic(err) + t.Errorf("%v: %v", filename, err) + t.FailNow() } cases := allCases[:0] From 8fe644f01ff69e020cd4da3aa3524cf25f5a9e42 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 10 Nov 2022 23:45:31 -0800 Subject: [PATCH 4/4] Add io.Reader variant Move core logic to ParseTestCases, which operates on an io.Reader, but keep the file-based function around for convenience. --- testutil/testutil.go | 27 ++++++++++++++++----------- testutil/testutil_test.go | 31 ++++++++++++++++--------------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index 3404a5a..b969525 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "io" "os" "regexp" "runtime/debug" @@ -99,10 +100,7 @@ func (e *testCaseParseError) Unwrap() error { return e.Err } -// ParseTestCaseFile parses the contents of the given test case file -// and reurns the test cases found inside. -// -// The file should contain zero or more test cases, each in the form: +// ParseTestCases parses test cases from a source in the following format: // // NUM[:DESC] // [OPTIONS] @@ -143,13 +141,7 @@ func (e *testCaseParseError) Unwrap() error { // //- - - - - - - - -// // Hello, **world**. // [..] -func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { - fp, err := os.Open(filename) - if err != nil { - return nil, err - } - defer fp.Close() - +func ParseTestCases(fp io.Reader) ([]MarkdownTestCase, error) { scanner := bufio.NewScanner(fp) c := MarkdownTestCase{ No: -1, @@ -169,6 +161,7 @@ func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { } } + var err error for scanner.Scan() { line++ if util.IsBlank([]byte(scanner.Text())) { @@ -232,6 +225,18 @@ func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { return cases, nil } +// ParseTestCaseFile reads test cases as described by [ParseTestCases] +// from an external file. +func ParseTestCaseFile(filename string) ([]MarkdownTestCase, error) { + fp, err := os.Open(filename) + if err != nil { + return nil, err + } + defer fp.Close() + + return ParseTestCases(fp) +} + // DoTestCaseFile runs test cases in a given file. func DoTestCaseFile(m goldmark.Markdown, filename string, t TestingT, no ...int) { allCases, err := ParseTestCaseFile(filename) diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index 541a993..d6d147c 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -3,7 +3,6 @@ package testutil import ( "errors" "os" - "path/filepath" "reflect" "strings" "testing" @@ -13,7 +12,7 @@ import ( // that doesn't conform to testing.T. var _ TestingT = (*testing.T)(nil) -func TestParseTestCaseFile(t *testing.T) { +func TestParseTestCases(t *testing.T) { tests := []struct { desc string give string // contents of the test file @@ -85,12 +84,7 @@ func TestParseTestCaseFile(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - filename := filepath.Join(t.TempDir(), "give.txt") - if err := os.WriteFile(filename, []byte(tt.give), 0o644); err != nil { - t.Fatal(err) - } - - got, err := ParseTestCaseFile(filename) + got, err := ParseTestCases(strings.NewReader(tt.give)) if err != nil { t.Fatalf("could not parse: %v", err) } @@ -104,7 +98,7 @@ func TestParseTestCaseFile(t *testing.T) { } } -func TestParseTestCaseFile_Errors(t *testing.T) { +func TestParseTestCases_Errors(t *testing.T) { tests := []struct { desc string give string // contents of the test file @@ -166,12 +160,7 @@ func TestParseTestCaseFile_Errors(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - filename := filepath.Join(t.TempDir(), "give.txt") - if err := os.WriteFile(filename, []byte(tt.give), 0o644); err != nil { - t.Fatal(err) - } - - cases, err := ParseTestCaseFile(filename) + cases, err := ParseTestCases(strings.NewReader(tt.give)) if err == nil { t.Fatalf("expected error, got:\n%#v", cases) } @@ -185,6 +174,18 @@ func TestParseTestCaseFile_Errors(t *testing.T) { } } +func TestParseTestCaseFile_Error(t *testing.T) { + cases, err := ParseTestCaseFile("does_not_exist.txt") + if err == nil { + t.Fatalf("expected error, got:\n%#v", cases) + } + + if !errors.Is(err, os.ErrNotExist) { + t.Errorf(" unexpected error = %v", err) + t.Errorf("expected unwrap to = %v", os.ErrNotExist) + } +} + func TestTestCaseParseError(t *testing.T) { wrapped := errors.New("great sadness") err := &testCaseParseError{Line: 42, Err: wrapped}