[testify-expert] Improve Test Quality: pkg/gitutil/gitutil_ctr_formal_test.go

[github-actions]

Current State

• File: pkg/gitutil/gitutil_ctr_formal_test.go (130 LOC, 11 test functions)
• Paired source: pkg/gitutil/gitutil.go (functions ValidateGitRef, ValidateGitPath, plus others)
• Sibling test file: pkg/gitutil/gitutil_test.go already contains table-driven TestValidateGitRef and TestValidateGitPath covering nearly identical cases.

Strengths

• Correct require/assert split (fail-fast on the primary error check, assert for secondary message checks).
• Uses t.Parallel() consistently, including in subtests.
• Uses require.ErrorContains / assert.ErrorContains instead of manual strings.Contains(err.Error(), ...).

Prioritized Improvements

1. Missing / high-value test: ValidateGitPath does not reject NUL bytes

Details

ValidateGitRef explicitly rejects NUL bytes (gitutil.go line 60-61), and this test file covers that case (TestValidateGitRef_NulByteRejected). However ValidateGitPath (gitutil.go line 72-86) has no NUL byte check at all, and there is no test in this file (or in gitutil_test.go) asserting the expected behavior for ValidateGitPath("evil\x00path").

Since ValidateGitPath values ultimately reach exec.Command argument lists, an un-validated NUL byte is a latent gap: Go's os/exec will error at syscall time, but the validator itself gives no actionable message before that point. Add a test asserting current behavior (even if it documents "no NUL check" today), and file a matching source-level test that will start failing once the source is fixed — this creates a safety net for a likely follow-up fix in ValidateGitPath.

func TestValidateGitPath_NulByteBehavior(t *testing.T) {
	t.Parallel()
	p := "evil\x00path"
	err := ValidateGitPath(p)
	// Currently ValidateGitPath does not check for NUL bytes; document this explicitly
	// so a future symmetry fix (mirroring ValidateGitRef) is caught by CI.
	require.Error(t, err, "ValidateGitPath should reject NUL bytes for symmetry with ValidateGitRef")
}

2. Testify assertion upgrades

Details

Several subtests repeat fmt.Sprintf("%q", ref) / fmt.Sprintf("%q", p) to build the expected substring for assert.ErrorContains. This is fragile: if the error format ever changes from %q to something else, every one of these tests silently breaks in lockstep rather than being centralized.

Before:

func TestValidateGitRef_HyphenPrefixRejected(t *testing.T) {
	t.Parallel()
	ref := "-evil"
	err := ValidateGitRef(ref)
	require.Error(t, err)
	require.ErrorContains(t, err, "must not start with '-'")
	assert.ErrorContains(t, err, fmt.Sprintf("%q", ref))
}

After (extract a small helper, reduces duplication across all 6 similarly-shaped tests):

func assertGitValidationError(t *testing.T, err error, value, wantSubstr string) {
	t.Helper()
	require.Error(t, err)
	require.ErrorContains(t, err, wantSubstr)
	assert.ErrorContains(t, err, fmt.Sprintf("%q", value))
}

func TestValidateGitRef_HyphenPrefixRejected(t *testing.T) {
	t.Parallel()
	ref := "-evil"
	assertGitValidationError(t, ValidateGitRef(ref), ref, "must not start with '-'")
}

3. Table-driven refactor / de-duplication with gitutil_test.go

Details

This file duplicates cases already present in gitutil_test.go's TestValidateGitRef / TestValidateGitPath table-driven tests (e.g. hyphen-prefix, NUL byte, traversal, empty-string cases are tested twice under different function names: TestValidateGitRef table rows vs. TestValidateGitRef_HyphenPrefixRejected, etc.).

Recommendation: either

• merge the unique cases from this file (e.g. TestValidateGitRef_ErrorMessagesAreActionable, which asserts the %q-quoted value appears in all invalid-ref error messages) into the existing table in gitutil_test.go as additional table rows / a shared assertion, or
• rename this file to clarify its distinct purpose (e.g. "public API / CTR-focused smoke tests") if it is intentionally a separate contract-test surface, and add a doc comment at the top explaining why duplication with gitutil_test.go is intentional.

Currently there is no comment indicating why two separate test files assert the same behavior, which will confuse future maintainers deciding where to add new validation cases.

4. Organization / readability

Details

• Add a short package/file-level doc comment explaining the intended scope of gitutil_ctr_formal_test.go relative to gitutil_test.go (e.g., "CTR" naming is not self-explanatory to new contributors).
• Group the ValidateGitRef_* and ValidateGitPath_* tests under clearer t.Run subtests within two parent tests instead of 10 top-level Test* functions, to make the parallel test count and reporting more compact.

Acceptance Checklist

• Add ValidateGitPath NUL-byte test case (and correct source behavior if a gap is confirmed).
• Extract shared assertion helper to reduce duplicated fmt.Sprintf("%q", ...) boilerplate.
• Resolve duplication with gitutil_test.go (merge or document intentional overlap).
• Run make test-unit and confirm all pkg/gitutil tests pass.

Generated by 🧪 Daily Testify Uber Super Expert · copilot · auto · 27.1 AIC · ⌖ 8.22 AIC · ⊞ 7.7K · ◷

• expires on Sep 12, 2026, 10:06 AM UTC-08:00