amadan

carlos / platform Public

test: short approved copy must be a whole string literal, not text in a comment (nearest-region Task 25 fix 1)

The short-text check searched raw bytes for the quoted form, so a quoted
comment such as // Previous label: "Region" kept C1 passing after the
approved literal was gone. A short text in a Go file now needs a string
token (the scanner skips comments) whose decoded value equals the text.
U1 to U3 keep substring matching inside a decoded literal. A new test runs
the check on in-memory sources: quoted comments and unrelated identifiers
fail, the real literal and a fragment inside a larger literal pass.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 7311890c6cec561e81810eabdb6d2895a1e9635e parent e34f596
1 file changed, +59 −13
  • approved_copy_test.go +59 −13
diff --git a/approved_copy_test.go b/approved_copy_test.go
index 90dd14401..508daa7ab 100644
--- a/approved_copy_test.go
+++ b/approved_copy_test.go
@@ -49,8 +49,7 @@ var approvedCopyFragments = map[string]bool{"U1": true, "U2": true, "U3": true}
// shortCopyBytes is the length under which a text is a word or a flag name.
// A short text could appear by accident (the word "Region" in a comment), so
-// in a Go file it must appear as a whole string literal, in its Go-quoted
-// form or its backtick form.
+// in a Go file it must be the whole decoded value of a string literal.
const shortCopyBytes = 25
// Every string Paul approved in the nearest-region copy review is present,
@@ -122,27 +121,27 @@ func checkApprovedCopy(t *testing.T, raw []byte, root string) {
}
// copyApplied reports whether body holds the approved text. Docs, scripts and
-// long Go texts match raw. A short text in a Go file must be a whole string
-// literal, so a word cannot pass by appearing elsewhere in the file.
+// long Go texts match raw. A short text in a Go file must be the whole decoded
+// value of a string literal token, so a word cannot pass by appearing in a
+// comment, an identifier or a longer literal. U1 to U3 are pieces of a larger
+// literal, so they need only be inside some literal's decoded value.
func copyApplied(id, file, text string, body []byte) bool {
if !strings.HasSuffix(file, ".go") {
return bytes.Contains(body, []byte(text))
}
if approvedCopyFragments[id] {
- return goLiteralContains(body, text)
+ return goLiteralMatches(body, func(v string) bool { return strings.Contains(v, text) })
}
if len(text) >= shortCopyBytes {
return bytes.Contains(body, []byte(text))
}
- if bytes.Contains(body, []byte(strconv.Quote(text))) {
- return true
- }
- return !strings.Contains(text, "`") && bytes.Contains(body, []byte("`"+text+"`"))
+ return goLiteralMatches(body, func(v string) bool { return v == text })
}
-// goLiteralContains reports whether the value of any string literal in the Go
-// source contains text.
-func goLiteralContains(src []byte, text string) bool {
+// goLiteralMatches reports whether the decoded value of any string literal in
+// the Go source satisfies match. The scanner skips comments, so text that
+// appears only in a comment never matches.
+func goLiteralMatches(src []byte, match func(string) bool) bool {
fset := token.NewFileSet()
file := fset.AddFile("", fset.Base(), len(src))
var s scanner.Scanner
@@ -155,8 +154,55 @@ func goLiteralContains(src []byte, text string) bool {
if tok != token.STRING {
continue
}
- if v, err := strconv.Unquote(lit); err == nil && strings.Contains(v, text) {
+ if v, err := strconv.Unquote(lit); err == nil && match(v) {
return true
}
}
}
+
+// The check itself, run on in-memory sources: the cases that must not pass
+// and the ones that must.
+func TestApprovedCopyCheckRejectsFalsePasses(t *testing.T) {
+ const real = "package p\n\nvar c = struct{ PickerLabel string }{\n\tPickerLabel: \"Region\",\n}\n"
+ cases := []struct {
+ name string
+ id string
+ text string
+ src string
+ want bool
+ }{
+ {"short: the real literal", "C1", "Region", real, true},
+ {"short: a raw string literal", "C1", "Region", "package p\n\nvar c = `Region`\n", true},
+ {"short: a quoted comment only", "C1", "Region",
+ "package p\n\nvar c = \"Zone\" // Previous label: \"Region\"\n", false},
+ {"short: a quoted block comment only", "C1", "Region",
+ "package p\n\nvar c = \"Zone\"\n/* was \"Region\" */\n", false},
+ {"short: a backtick comment only", "C1", "Region",
+ "package p\n\nvar c = \"Zone\" // was `Region`\n", false},
+ {"short: an unrelated identifier only", "C1", "Region",
+ "package p\n\nvar Region = \"Zone\"\n", false},
+ {"short: a longer literal that contains it", "C1", "Region",
+ "package p\n\nvar c = \"Region: %s\"\n", false},
+ {"short: a literal that differs by case", "C1", "Region",
+ "package p\n\nvar c = \"region\"\n", false},
+ {"short: non-ASCII text as a literal", "C23-ca-central-1", "Canada · Montréal",
+ "package p\n\nvar c = \"Canada · Montréal\"\n", true},
+ {"fragment: inside a larger literal", "U1", " [--region <code>]",
+ "package p\n\nvar u = \"apps create [--region <code>] [--account <a>]\"\n", true},
+ {"fragment: in a comment only", "U1", " [--region <code>]",
+ "package p\n\nvar u = \"apps create\" // [--region <code>]\n", false},
+ {"fragment: in an identifier-free comment block", "U3", "[--region <code>]",
+ "package p\n\nvar u = `canary`\n/* [--region <code>] */\n", false},
+ {"long: a text of 25 bytes or more matches raw", "C2", "Where this app's instances run, near you.",
+ "package p\n\nvar c = \"Where this app's instances run, near you.\"\n", true},
+ }
+ for _, c := range cases {
+ if got := copyApplied(c.id, "x.go", c.text, []byte(c.src)); got != c.want {
+ t.Errorf("%s: copyApplied = %v, want %v", c.name, got, c.want)
+ }
+ }
+ // Docs keep raw matching.
+ if !copyApplied("D4", "x.md", "Without `--region`, a new instance starts", []byte("a Without `--region`, a new instance starts b")) {
+ t.Error("a docs text is matched raw")
+ }
+}