[testify-expert] Improve Test Quality: pkg/timeutil/format_test.go

[github-actions]

Current State

• File: pkg/timeutil/format_test.go (170 lines, 1 test function: TestFormatDuration)
• Source pair: pkg/timeutil/format.go (53 lines, exports FormatDuration, FormatDurationMs, FormatDurationNs)
• Test count: 1 table-driven test with 24 sub-cases, all for FormatDuration only.
• Sibling file pkg/timeutil/spec_test.go covers all three exported functions using testify/assert, but with far fewer edge cases (9, 6, and 4 cases respectively) and it exists as a separate "spec" test, not part of this file.

Strengths

• TestFormatDuration is well-organized as a table-driven test with clear boundary coverage (999ns→1μs, 59.999s→60.0s, etc.) and uses t.Parallel() correctly at both parent and subtest level.

Prioritized Improvements

1. Missing / high-value tests (coverage gap)

format_test.go tests zero cases for FormatDurationMs and FormatDurationNs, even though they are exported, non-trivial (rounding, negative/zero guard, minute+second composition) functions in the same source file. Coverage for these currently lives only in spec_test.go, which is documentation-driven and misses several edge cases:

• FormatDurationMs: negative input (currently falls into the ms < 1000 branch and would print e.g. "-500ms" — unverified behavior), ms == 1000 boundary, ms == 60000 boundary (minute rollover), and multi-minute values like 125000 → "2m5s".
• FormatDurationNs: rounding behavior near half-second boundaries (e.g. 1_500_000_000 ns → should round via time.Duration.Round), and very large durations (multi-hour).

Suggested additional test function

func TestFormatDurationMs(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name     string
		ms       int
		expected string
	}{
		{name: "sub-second", ms: 500, expected: "500ms"},
		{name: "exactly 1000ms boundary", ms: 1000, expected: "1.0s"},
		{name: "seconds with decimal", ms: 1500, expected: "1.5s"},
		{name: "exactly 60000ms boundary", ms: 60000, expected: "1m0s"},
		{name: "minutes and seconds", ms: 90000, expected: "1m30s"},
		{name: "multi-minute", ms: 125000, expected: "2m5s"},
		{name: "zero", ms: 0, expected: "0ms"},
		{name: "negative value", ms: -500, expected: "-500ms"}, // documents current (possibly unintended) behavior
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			assert.Equal(t, tt.expected, FormatDurationMs(tt.ms))
		})
	}
}

func TestFormatDurationNs(t *testing.T) {
	t.Parallel()
	tests := []struct {
		name     string
		ns       int64
		expected string
	}{
		{name: "zero returns em-dash", ns: 0, expected: "—"},
		{name: "negative returns em-dash", ns: -1, expected: "—"},
		{name: "rounds to nearest second", ns: 1_500_000_000, expected: "2s"},
		{name: "multi-hour duration", ns: 3*60*60*1_000_000_000 + 30*60*1_000_000_000, expected: "3h30m0s"},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			assert.Equal(t, tt.expected, FormatDurationNs(tt.ns))
		})
	}
}

If negative ms should not be silently accepted, this test would also surface a latent bug in FormatDurationMs for confirmation/fix.

2. Testify assertion upgrades

The existing test uses manual comparison and t.Errorf:

result := FormatDuration(tt.duration)
if result != tt.expected {
	t.Errorf("FormatDuration(%v) = %q, want %q", tt.duration, result, tt.expected)
}

Every other test file in pkg/timeutil (spec_test.go) and the majority of the repo's test suite use testify/assert/require. Replace with:

assert.Equal(t, tt.expected, result, "FormatDuration(%v) mismatch", tt.duration)

This requires adding "github.com/stretchr/testify/assert" to the import block.

3. Table-driven refactors

Already table-driven — no changes needed here beyond merging the new FormatDurationMs/FormatDurationNs tables described above, ideally consolidated into this file (format_test.go) rather than split across spec_test.go, since both target the same source file format.go.

4. Organization / readability

• Consider moving the doc-oriented spec assertions in spec_test.go to reference this file's more exhaustive tables instead of duplicating a thinner set of cases, to avoid drift between the two files over time.
• Group sub-tests by unit-conversion boundary using t.Run names consistent with the existing style (already followed well in this file).

Acceptance Checklist

• Add TestFormatDurationMs covering boundary values (0, 999, 1000, 59999, 60000, negative) and multi-minute compositions.
• Add TestFormatDurationNs covering zero, negative, rounding-boundary, and multi-hour cases.
• Convert TestFormatDuration (and new tests) to use testify/assert instead of manual if/t.Errorf.
• Confirm/resolve behavior of FormatDurationMs with negative input (either assert current behavior explicitly or fix format.go if it's a bug).
• Run make test-unit and confirm all pkg/timeutil tests pass.

Generated by 🧪 Daily Testify Uber Super Expert · copilot · auto · 29 AIC · ⌖ 5.97 AIC · ⊞ 7.6K · ◷

• expires on Aug 27, 2026, 10:08 AM UTC-08:00

[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 · codex · mai10 · 1.96 AIC · ⌖ 2.79 AIC · ⊞ 17.2K · ◷