From 282e1428bc1ceb935288986bb89424e4a501db3a Mon Sep 17 00:00:00 2001
From: Abhinav Gupta <mail@abhinavg.net>
Date: Thu, 10 Nov 2022 18:51:06 -0800
Subject: [PATCH] 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",
+				"//- - - - - - - - -//",
+				"<p>world</p>",
+				"//= = = = = = = = = = = = = = = = = = = = = = = =//",
+			}, "\n"),
+			errMsg: "line 1: invalid case No",
+		},
+		{
+			desc: "bad number/description",
+			give: strings.Join([]string{
+				"1 not a number:description",
+				"//- - - - - - - - -//",
+				"world",
+				"//- - - - - - - - -//",
+				"<p>world</p>",
+				"//= = = = = = = = = = = = = = = = = = = = = = = =//",
+			}, "\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",
+				"//- - - - - - - - -//",
+				"<p>world</p>",
+				"//= = = = = = = = = = = = = = = = = = = = = = = =//",
+			}, "\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)
+		}
+	})
+}