Correct errors, test for specific error messages, improve formatting

Signed-off-by: Ben Ridley <benridley29@gmail.com>

timeinterval/timeinterval.go

@@ -181,23 +181,6 @@ func (r *WeekdayRange) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return err
 }
 
-// MarshalYAML implements the yaml.Marshaler interface for WeekdayRange
-func (r WeekdayRange) MarshalYAML() (interface{}, error) {
-	beginStr, ok := daysOfWeekInv[r.Begin]
-	if !ok {
-		return nil, fmt.Errorf("unable to convert %d into weekday string", r.Begin)
-	}
-	if r.Begin == r.End {
-		return interface{}(beginStr), nil
-	}
-	endStr, ok := daysOfWeekInv[r.End]
-	if !ok {
-		return nil, fmt.Errorf("unable to convert %d into weekday string", r.End)
-	}
-	rangeStr := fmt.Sprintf("%s:%s", beginStr, endStr)
-	return interface{}(rangeStr), nil
-}
-
 // UnmarshalYAML implements the Unmarshaller interface for DayOfMonthRange.
 func (r *DayOfMonthRange) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	var str string
@@ -205,23 +188,30 @@ func (r *DayOfMonthRange) UnmarshalYAML(unmarshal func(interface{}) error) error
 		return err
 	}
 	err := stringableRangeFromString(str, r)
+	// Check beginning <= end accounting for negatives day of month indices as well.
+	// Months != 31 days can't be addressed here and are clamped, but at least we can catch blatant errors.
 	if r.Begin == 0 || r.Begin < -31 || r.Begin > 31 {
 		return fmt.Errorf("%d is not a valid day of the month: out of range", r.Begin)
 	}
 	if r.End == 0 || r.End < -31 || r.End > 31 {
 		return fmt.Errorf("%d is not a valid day of the month: out of range", r.End)
 	}
-	// Check Beginning <= End accounting for negatives day of month indices
-	trueBegin := r.Begin
-	trueEnd := r.End
+	// Restricting here prevents errors where begin > end in longer months but not shorter months.
+	if r.Begin < 0 && r.End > 0 {
+		return fmt.Errorf("end day must be negative if start day is negative")
+	}
+	// Check begin <= end. We can't know this for sure when using negative indices
+	// but we can prevent cases where its always invalid (using 28 day minimum length)
+	checkBegin := r.Begin
+	checkEnd := r.End
 	if r.Begin < 0 {
-		trueBegin = 30 + r.Begin
+		checkBegin = 28 + r.Begin
 	}
 	if r.End < 0 {
-		trueEnd = 30 + r.End
+		checkEnd = 28 + r.End
 	}
-	if trueBegin > trueEnd {
-		return errors.New("start day cannot be before end day")
+	if checkBegin > checkEnd {
+		return fmt.Errorf("end day %d is always before start day %d", r.End, r.Begin)
 	}
 	return err
 }
@@ -232,34 +222,15 @@ func (r *MonthRange) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	if err := unmarshal(&str); err != nil {
 		return err
 	}
-	err := stringableRangeFromString(str, r)
+	if err := stringableRangeFromString(str, r); err != nil {
+		return err
+	}
 	if r.Begin > r.End {
-		return errors.New("start month cannot be before end month")
+		begin := monthsInv[r.Begin]
+		end := monthsInv[r.End]
+		return fmt.Errorf("end month %s is before start month %s", end, begin)
 	}
-	if r.Begin < 1 || r.Begin > 12 {
-		return fmt.Errorf("%s is not a valid month: out of range", str)
-	}
-	if r.End < 1 || r.End > 12 {
-		return fmt.Errorf("%s is not a valid month: out of range", str)
-	}
-	return err
-}
-
-// MarshalYAML implements the yaml.Marshaler interface for DayOfMonthRange
-func (r MonthRange) MarshalYAML() (interface{}, error) {
-	beginStr, ok := monthsInv[r.Begin]
-	if !ok {
-		return nil, fmt.Errorf("unable to convert %d into month", r.Begin)
-	}
-	if r.Begin == r.End {
-		return interface{}(beginStr), nil
-	}
-	endStr, ok := monthsInv[r.End]
-	if !ok {
-		return nil, fmt.Errorf("unable to convert %d into month", r.End)
-	}
-	rangeStr := fmt.Sprintf("%s:%s", beginStr, endStr)
-	return interface{}(rangeStr), nil
+	return nil
 }
 
 // UnmarshalYAML implements the Unmarshaller interface for YearRange.
@@ -270,7 +241,7 @@ func (r *YearRange) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	}
 	err := stringableRangeFromString(str, r)
 	if r.Begin > r.End {
-		return errors.New("start day cannot be before end day")
+		return fmt.Errorf("end year %d is before start year %d", r.End, r.Begin)
 	}
 	return err
 }
@@ -286,25 +257,36 @@ func (tr *TimeRange) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	}
 	start, err := parseTime(y.StartTime)
 	if err != nil {
-		return nil
+		return err
 	}
-	End, err := parseTime(y.EndTime)
+	end, err := parseTime(y.EndTime)
 	if err != nil {
 		return err
 	}
-	if start < 0 {
-		return errors.New("start time out of range")
-	}
-	if End > 1440 {
-		return errors.New("End time out of range")
-	}
-	if start >= End {
+	if start >= end {
 		return errors.New("start time cannot be equal or greater than end time")
 	}
-	tr.StartMinute, tr.EndMinute = start, End
+	tr.StartMinute, tr.EndMinute = start, end
 	return nil
 }
 
+// MarshalYAML implements the yaml.Marshaler interface for WeekdayRange
+func (r WeekdayRange) MarshalYAML() (interface{}, error) {
+	beginStr, ok := daysOfWeekInv[r.Begin]
+	if !ok {
+		return nil, fmt.Errorf("unable to convert %d into weekday string", r.Begin)
+	}
+	if r.Begin == r.End {
+		return interface{}(beginStr), nil
+	}
+	endStr, ok := daysOfWeekInv[r.End]
+	if !ok {
+		return nil, fmt.Errorf("unable to convert %d into weekday string", r.End)
+	}
+	rangeStr := fmt.Sprintf("%s:%s", beginStr, endStr)
+	return interface{}(rangeStr), nil
+}
+
 //MarshalYAML implements the yaml.Marshaler interface for TimeRange
 func (tr TimeRange) MarshalYAML() (out interface{}, err error) {
 	startHr := tr.StartMinute / 60
@@ -432,7 +414,7 @@ func (tp TimeInterval) ContainsTime(t time.Time) bool {
 	return true
 }
 
-// Converts a string of the form "HH:MM" into a TimeRange
+// Converts a string of the form "HH:MM" into the number of minutes elapsed in the day
 func parseTime(in string) (mins int, err error) {
 	if !validTimeRE.MatchString(in) {
 		return 0, fmt.Errorf("couldn't parse timestamp %s, invalid format", in)

timeinterval/timeinterval_test.go

@@ -181,6 +181,7 @@ var yamlUnmarshalTestCases = []struct {
 	contains    []string
 	excludes    []string
 	expectError bool
+	err         string
 }{
 	{
 		// Simple business hours test
@@ -262,12 +263,33 @@ var yamlUnmarshalTestCases = []struct {
 			"01 Apr 21 13:00 GMT",
 		},
 	},
+	{
+		// Invalid start time
+		in: `
+---
+- times:
+    - start_time: '01:99'
+      end_time: '23:59'`,
+		expectError: true,
+		err:         "couldn't parse timestamp 01:99, invalid format",
+	},
+	{
+		// Invalid end time
+		in: `
+---
+- times:
+    - start_time: '00:00'
+      end_time: '99:99'`,
+		expectError: true,
+		err:         "couldn't parse timestamp 99:99, invalid format",
+	},
 	{
 		// Start day before End day
 		in: `
 ---
 - weekdays: ['friday:monday']`,
 		expectError: true,
+		err:         "start day cannot be before end day",
 	},
 	{
 		// Invalid weekdays
@@ -276,6 +298,7 @@ var yamlUnmarshalTestCases = []struct {
 - weekdays: ['blurgsday:flurgsday']
 `,
 		expectError: true,
+		err:         "blurgsday is not a valid weekday",
 	},
 	{
 		// 0 day of month
@@ -284,14 +307,25 @@ var yamlUnmarshalTestCases = []struct {
 - days_of_month: ['0']
 `,
 		expectError: true,
+		err:         "0 is not a valid day of the month: out of range",
 	},
 	{
-		// Too early day of month
+		// Start day of month < 0
 		in: `
 ---
 - days_of_month: ['-50:-20']
 `,
 		expectError: true,
+		err:         "-50 is not a valid day of the month: out of range",
+	},
+	{
+		// End day of month > 31
+		in: `
+---
+- days_of_month: ['1:50']
+`,
+		expectError: true,
+		err:         "50 is not a valid day of the month: out of range",
 	},
 	{
 		// Negative indices should work
@@ -307,20 +341,58 @@ var yamlUnmarshalTestCases = []struct {
 		expectError: false,
 	},
 	{
-		// Negative start date before positive End date
+		// End day must be negative if begin day is negative
 		in: `
 ---
 - days_of_month: ['-15:5']
 `,
 		expectError: true,
+		err:         "end day must be negative if start day is negative",
 	},
 	{
-		// Negative End date before positive postive start date
+		// Negative end date before positive postive start date
 		in: `
 ---
 - days_of_month: ['10:-25']
 `,
 		expectError: true,
+		err:         "end day -25 is always before start day 10",
+	},
+	{
+		// Invalid start month
+		in: `
+---
+- months: ['martius:june']
+`,
+		expectError: true,
+		err:         "martius is not a valid month",
+	},
+	{
+		// Invalid end month
+		in: `
+---
+- months: ['march:junius']
+`,
+		expectError: true,
+		err:         "junius is not a valid month",
+	},
+	{
+		// Start month after end month
+		in: `
+---
+- months: ['december:january']
+`,
+		expectError: true,
+		err:         "end month january is before start month december",
+	},
+	{
+		// Start year after end year
+		in: `
+---
+- years: ['2022:2020']
+`,
+		expectError: true,
+		err:         "end year 2020 is before start year 2022",
 	},
 }
 
@@ -333,6 +405,9 @@ func TestYamlUnmarshal(t *testing.T) {
 		} else if err == nil && tc.expectError {
 			t.Errorf("Expected error when unmarshalling %s but didn't receive one", tc.in)
 		} else if err != nil && tc.expectError {
+			if err.Error() != tc.err {
+				t.Errorf("Incorrect error: Want %s, got %s", tc.err, err.Error())
+			}
 			continue
 		}
 		if !reflect.DeepEqual(ti, tc.intervals) {