[testify-expert] Improve Test Quality: pkg/workflow/time_delta_test.go

[github-actions]

Overview

The test file pkg/workflow/time_delta_test.go (1284 lines, 11 test functions) has been selected for quality improvement by the Testify Uber Super Expert. This file has zero testify usage — it relies entirely on raw t.Errorf / t.Error calls, manual equality checks, and a custom containsString helper. Converting to testify would significantly improve readability, error messages, and consistency with the rest of the codebase.

Current State

• Test File: pkg/workflow/time_delta_test.go
• Source File: pkg/workflow/time_delta.go
• Test Functions: 11 test functions
• Lines of Code: 1284 lines (test) / 499 lines (source)
• Testify Usage: ❌ None — no assert or require imported

Test Quality Analysis

Strengths ✅

• Comprehensive table-driven tests with t.Run() throughout — this is the right pattern
• Good coverage of all exported/internal functions (11 test functions for 10 source functions)
• Excellent edge case coverage (empty strings, duplicates, boundary values, ordinal numbers)

🎯 Areas for Improvement

1. No Testify — Manual Assertions Everywhere

The file does not import github.com/stretchr/testify/assert or require. Every assertion uses raw t.Errorf with manual guards and early returns.

Current pattern (repeated ~40+ times):

if tt.expectError {
    if err == nil {
        t.Errorf("parseTimeDelta(%q) expected error but got none", tt.input)
        return
    }
    if tt.errorMsg != "" && !containsString(err.Error(), tt.errorMsg) {
        t.Errorf("parseTimeDelta(%q) error = %v, want to contain %v", tt.input, err.Error(), tt.errorMsg)
    }
} else {
    if err != nil {
        t.Errorf("parseTimeDelta(%q) unexpected error: %v", tt.input, err)
        return
    }
    if result == nil {
        t.Errorf("parseTimeDelta(%q) returned nil result", tt.input)
        return
    }
    if result.Days != tt.expected.Days || result.Hours != tt.expected.Hours || result.Minutes != tt.expected.Minutes {
        t.Errorf("parseTimeDelta(%q) = {Days: %d, Hours: %d, Minutes: %d}, ...")
    }
}

Recommended (testify):

if tt.expectError {
    require.Error(t, err, "parseTimeDelta(%q) should return an error", tt.input)
    if tt.errorMsg != "" {
        assert.Contains(t, err.Error(), tt.errorMsg, "error message should contain expected substring")
    }
} else {
    require.NoError(t, err, "parseTimeDelta(%q) should not return an error", tt.input)
    require.NotNil(t, result, "parseTimeDelta(%q) should return a non-nil result", tt.input)
    assert.Equal(t, tt.expected, result, "parseTimeDelta(%q) result mismatch", tt.input)
}

This reduces ~12 lines to ~6 and gives far better diff output on failure via assert.Equal.

2. Custom containsString Helper — Replace with assert.Contains

The file defines its own helper at line 730:

func containsString(s, substr string) bool {
    return strings.Contains(s, substr)
}

This is used in 5 places. With testify, it can be eliminated entirely:

// ❌ CURRENT
if tt.errorMsg != "" && !containsString(err.Error(), tt.errorMsg) {
    t.Errorf("...")
}

// ✅ IMPROVED
assert.Contains(t, err.Error(), tt.errorMsg, "error should contain expected message")

3. Struct Field Comparison — Replace with assert.Equal

Multiple tests manually compare individual struct fields instead of comparing the whole struct:

// ❌ CURRENT — verbose, misses Weeks/Months fields in some places
if result.Days != tt.expected.Days || result.Hours != tt.expected.Hours || result.Minutes != tt.expected.Minutes {
    t.Errorf("parseTimeDelta(%q) = {Days: %d, Hours: %d, Minutes: %d}, want {Days: %d, Hours: %d, Minutes: %d}",
        tt.input, result.Days, result.Hours, result.Minutes, tt.expected.Days, tt.expected.Hours, tt.expected.Minutes)
}

// ✅ IMPROVED — simpler, automatically compares all fields, better diff
assert.Equal(t, tt.expected, result, "parseTimeDelta(%q) result mismatch", tt.input)

Note: TestParseTimeDelta only compares Days, Hours, Minutes but not Weeks or Months — assert.Equal would catch regressions in those fields automatically.

4. Missing require.* for Critical Guards

When an error is not expected and err != nil, the test uses t.Errorf + return instead of require.NoError. The require pattern is more idiomatic and avoids the manual return:

// ❌ CURRENT
if err != nil {
    t.Errorf("parseAbsoluteDateTime(%q) unexpected error: %v", tt.input, err)
    return
}

// ✅ IMPROVED — stops subtest immediately on failure, no manual return
require.NoError(t, err, "parseAbsoluteDateTime(%q) should not error", tt.input)

5. TestParseAbsoluteDateTime — Repeated Validation Logic

The result parsing in TestParseAbsoluteDateTime is repeated inline for each subtest (parse result, check day, check month, check year separately). This could use a helper or assert.Equal on a struct:

// ✅ IMPROVED — parse once, assert once
require.NoError(t, err, "should parse absolute date time")
parsed, parseErr := time.Parse("2006-01-02 15:04:05", result)
require.NoError(t, parseErr, "result should be a valid timestamp: %s", result)
assert.Equal(t, tt.expectedYear, parsed.Year(), "year mismatch")
assert.Equal(t, tt.expectedMonth, parsed.Month(), "month mismatch")
assert.Equal(t, tt.expectedDay, parsed.Day(), "day mismatch")

📋 Implementation Guidelines

Steps

1. Add testify imports:

   import (
       "testing"
       "time"
   
       "github.com/stretchr/testify/assert"
       "github.com/stretchr/testify/require"
   )

2. Remove the containsString helper function (line 730)

3. Replace all t.Errorf / t.Error patterns with assert.* / require.* throughout all 11 test functions

4. Replace manual struct field comparisons with assert.Equal(t, tt.expected, result, ...)

5. Replace !containsString(err.Error(), tt.errorMsg) with assert.Contains(t, err.Error(), tt.errorMsg, ...)

Priority Order

1. High: Add testify imports, remove containsString
2. High: Replace manual error guards with require.NoError / require.Error
3. Medium: Replace field-by-field struct comparison with assert.Equal
4. Low: Add descriptive assertion messages where missing

Testing Commands

# Run tests for this package
go test -v ./pkg/workflow/ -run "TestParseTimeDelta|TestTimeDeltaString|TestIsRelativeStopTime|TestParseAbsoluteDateTime|TestResolveStopTime|TestIsRelativeDate|TestParseRelativeDate|TestResolveRelativeDate|TestParseRelativeTimeSpec|TestParseExpiresFromConfig"

# Run all unit tests
make test-unit

Reference

• Repository testing guidelines: scratchpad/testing.md
• Testify docs: https://github.com/stretchr/testify

Acceptance Criteria

• github.com/stretchr/testify/assert and require imported
• Custom containsString helper removed
• All t.Errorf / t.Error patterns replaced with assert.* / require.*
• Struct comparisons use assert.Equal(t, tt.expected, result, ...) instead of field-by-field checks
• Error containment checks use assert.Contains instead of !containsString(...)
• All assertions include helpful messages
• Tests pass: make test-unit

---

Priority: Medium
Effort: Medium (1284-line file, 11 functions, ~40+ assertion sites)
Expected Impact: Shorter test code, richer failure output, consistency with codebase standards

Files Involved:

• Test file: pkg/workflow/time_delta_test.go
• Source file: pkg/workflow/time_delta.go

References: §25636010680

Generated by Daily Testify Uber Super Expert · ● 13M · ◷

• expires on May 12, 2026, 6:16 PM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot

I've identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ● 178.7K · ◷