cmd/screentest: reject negative retrypixels values

Both the global -retrypixels flag and the per-test retrypixels directive
accepted negative values. The comparison later converted the configured
int to uint64, so -1 became math.MaxUint64 and made every same-sized
difference retryable.

Reject negative values at both input paths, document the constraint, and
add regression tests.

Fixes golang/go#80898

Change-Id: I876acc9a3aec69ff07d34105fe8d703ed30b2da0
Reviewed-on: https://go-review.googlesource.com/c/website/+/816040
Auto-Submit: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: David Chase <drchase@google.com>

cmd/screentest/main.go

diff --git a/cmd/screentest/main.go b/cmd/screentest/main.go
index 158ab58..0277f4d 100644
--- a/cmd/screentest/main.go
+++ b/cmd/screentest/main.go
@@ -30,7 +30,7 @@
 	-retrypixels N
 	  If the difference is no more than N pixels, take another screenshot. Repeat up to 3
 	  times. N should be small. This is a last-resort method for handling small, inexplicable
-	  output variations.
+	  output variations. N must be non-negative.
 	-run REGEXP
 	  Run only tests matching regexp.
 	-o
@@ -114,6 +114,7 @@
 	status 404
 
 Use retrypixels N to override the value of the -retrypixels flag for this test.
+N must be non-negative.
 
 	retrypixels 80
 
@@ -184,7 +185,7 @@
 	flag.StringVar(&flags.outputDirURL, "o", "", "path for output: file path or URL with 'file' or 'gs' scheme")
 	flag.StringVar(&flags.headers, "headers", "", "HTTP headers: comma-separated list of name:value")
 	flag.StringVar(&flags.filterRegexp, "run", "", "regexp to match test")
-	flag.IntVar(&flags.retryPixels, "retrypixels", 0, "repeat up to 3 times if diff is <= this value")
+	flag.IntVar(&flags.retryPixels, "retrypixels", 0, "repeat up to 3 times if diff is <= this non-negative value")
 }
 
 // options are the options for the program.

cmd/screentest/screentest.go

diff --git a/cmd/screentest/screentest.go b/cmd/screentest/screentest.go
index ada5ccf..f2dcd4d 100644
--- a/cmd/screentest/screentest.go
+++ b/cmd/screentest/screentest.go
@@ -254,6 +254,10 @@
 
 // commonValues returns values common to all test files.
 func commonValues(ctx context.Context, testURL, wantURL string, opts options) (c common, err error) {
+	if opts.retryPixels < 0 {
+		return common{}, fmt.Errorf("-retrypixels must be non-negative, got %d", opts.retryPixels)
+	}
+
 	// The test/want image readers/writers are relative to the test/want URLs, so
 	// they are common to all files. See test/wantPath for the file- and test-relative components.
 	// They may be nil if a URL has an http or https scheme.
@@ -396,10 +400,14 @@
 			if test == nil {
 				return nil, errors.New("directive must be in a test")
 			}
-			test.retryPixels, err = strconv.Atoi(args)
+			retryPixels, err := strconv.Atoi(args)
 			if err != nil {
 				return nil, fmt.Errorf("strconv.Atoi(%q): %w", args, err)
 			}
+			if retryPixels < 0 {
+				return nil, fmt.Errorf("RETRYPIXELS must be non-negative, got %d", retryPixels)
+			}
+			test.retryPixels = retryPixels
 
 		case "PATH":
 			if test == nil {

cmd/screentest/screentest_test.go

diff --git a/cmd/screentest/screentest_test.go b/cmd/screentest/screentest_test.go
index 000fa14..a389a26 100644
--- a/cmd/screentest/screentest_test.go
+++ b/cmd/screentest/screentest_test.go
@@ -17,6 +17,7 @@
 	"path"
 	"path/filepath"
 	"slices"
+	"strings"
 	"testing"
 	"time"
 
@@ -38,6 +39,7 @@
 		opts             options
 		want             []*testcase
 		wantErr          bool
+		wantErrText      string
 	}{
 		{
 			name:    "readtests",
@@ -212,6 +214,11 @@
 			wantErr: true,
 		},
 		{
+			name:        "readtests-negative-retrypixels",
+			wantErr:     true,
+			wantErrText: "RETRYPIXELS must be non-negative, got -1",
+		},
+		{
 			name: "readtests-filter",
 			opts: options{
 				filterRegexp: `foo \d`,
@@ -248,6 +255,9 @@
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("readTests() error = %v, wantErr %v", err, tt.wantErr)
 			}
+			if err != nil && tt.wantErrText != "" && !strings.Contains(err.Error(), tt.wantErrText) {
+				t.Fatalf("readTests() error = %q, want error containing %q", err, tt.wantErrText)
+			}
 			if err != nil {
 				return
 			}
@@ -266,6 +276,17 @@
 	}
 }
 
+func TestCommonValuesRejectsNegativeRetryPixels(t *testing.T) {
+	const want = "-retrypixels must be non-negative, got -1"
+	_, err := commonValues(context.Background(), "", "", options{retryPixels: -1})
+	if err == nil {
+		t.Fatalf("commonValues() error = nil, want %q", want)
+	}
+	if got := err.Error(); got != want {
+		t.Fatalf("commonValues() error = %q, want %q", got, want)
+	}
+}
+
 func TestRun(t *testing.T) {
 	// Skip this test if Google Chrome is not installed.
 	_, err := exec.LookPath("google-chrome")

cmd/screentest/testdata/readtests-negative-retrypixels.txt

diff --git a/cmd/screentest/testdata/readtests-negative-retrypixels.txt b/cmd/screentest/testdata/readtests-negative-retrypixels.txt
new file mode 100644
index 0000000..9700827
--- /dev/null
+++ b/cmd/screentest/testdata/readtests-negative-retrypixels.txt
@@ -0,0 +1,2 @@
+test negative retrypixels
+retrypixels -1