Skip to content

fix(reverse): remove test-only shortcut that corrupted "Test<digits>" input - #34

Open
dualfroz wants to merge 1 commit into
gobeam:masterfrom
dualfroz:dualfroz/reverse-test-hack
Open

fix(reverse): remove test-only shortcut that corrupted "Test<digits>" input#34
dualfroz wants to merge 1 commit into
gobeam:masterfrom
dualfroz:dualfroz/reverse-test-hack

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Problem

Reverse() returns a wrong result for any string that starts with Test
followed by digits:

stringy.New("Test123").Reverse() // "123seT", want "321tseT"
stringy.New("Test42").Reverse()  // "42seT",  want "24tseT"
stringy.New("Test0").Reverse()   // "0seT",   want "0tseT"

The digit portion is not reversed at all, and the produced string is simply
incorrect for real-world input.

Root cause

stringy.go, Reverse:

// Special handling for TestN format in the concurrency test
if strings.HasPrefix(input, "Test") && len(input) > 4 {
	numPart := input[4:]
	allDigits := true
	for _, c := range numPart {
		if !unicode.IsDigit(c) {
			allDigits = false
			break
		}
	}
	if allDigits {
		// For TestN format in tests, return the expected format for test
		return numPart + "seT"
	}
}

This branch was added purely to satisfy a concurrency test and short-circuits
the real reversal, returning numPart + "seT" (the digits unreversed) instead
of reversing the whole string. It leaks test logic into the library and
produces incorrect output for legitimate inputs matching the pattern.

Fix

Remove the shortcut and always run the normal rune-based reversal:

input := getInput(*i)

r := []rune(input)
for i, j := 0, len(r)-1; i < len(r)/2; i, j = i+1, j-1 {
	r[i], r[j] = r[j], r[i]
}
return string(r)

Test

  • Added TestInput_Reverse_TestDigits asserting correct reversal for
    Test123, Test42, Test0, and Hello123.
  • Updated the concurrency test in TestConcurrentAccess (case 4), which
    previously pinned the defect by expecting fmt.Sprintf("%dseT", id). The new
    expectation reverses the entire "Test<id>" string independently of the
    implementation, per the standard "the old expected value encoded the bug"
    exception.

Gates:

  • go test ./... -> ok (PASS)
  • go vet ./... -> exit 0
  • go build ./... -> exit 0
  • gofmt -l stringy.go stringy_test.go -> clean

Counterfactual: restoring the shortcut makes TestInput_Reverse_TestDigits
fail with Reverse("Test123") = "123seT", want "321tseT".

Reverse() short-circuited any string matching Test followed by digits and
returned the digits unreversed plus seT, producing wrong output for real
input (e.g. Reverse("Test123") gave "123seT" instead of "321tseT").
Remove the shortcut so the normal rune reversal always runs, and correct the
concurrency test that had pinned the defective expectation.
@dualfroz
dualfroz force-pushed the dualfroz/reverse-test-hack branch from 41444e5 to dfccede Compare September 5, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant