cmd/screentest: turn deprecations into errors

This is the first CL that makes breaking changes to screentest.

Breaking changes to commands are permitted by the Go compatibility
guarantee. Furthermore, this repo is at major version 0, and I found
no uses of screentest anywhere in the open source ecosystem or Google's
internal codebase, other than in this repo and x/pkgsite.

The changes include:

- Formerly deprecated script features, like the compare, output
  and header directives, are now errors.

- The test and want URLs are now required, so they are arguments
  instead of flags.

- Instead of taking a glob which is expanded by the program, screentest
  now behaves like a normal Unix command and accepts a list of files,
  leaving glob expansion to the shell. (The internal globbing was a
  remnant of modeling the internal/screentest package on
  internal/webtest, where globbing makes sense.)

Change-Id: Ie851705a5c5e37c1934c164360fb523f62888455
Reviewed-on: https://go-review.googlesource.com/c/website/+/627060
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

4 files changed, 89 insertions, 121 deletions

diff --git a/cmd/golangorg/screentest.sh b/cmd/golangorg/screentest.sh
index 7e67ff7b..3ffbd1dd 100755
--- a/cmd/golangorg/screentest.sh
+++ b/cmd/golangorg/screentest.sh
@@ -3,7 +3,7 @@
 # Use of this source code is governed by a BSD-style
 # license that can be found in the LICENSE file.
 
-go run ./cmd/screentest \
-	-test http://localhost:6060/go.dev \
-	-want https://go.dev \
-	'./cmd/golangorg/testdata/screentest/*'
+# This script compares web pages generated by a golangorg web server running locally
+# with the pages on the production go.dev server.
+
+go run ./cmd/screentest http://localhost:6060/go.dev https://go.dev ./cmd/golangorg/testdata/screentest/*
diff --git a/cmd/screentest/main.go b/cmd/screentest/main.go
index 2cd55f06..8536f6bf 100644
--- a/cmd/screentest/main.go
+++ b/cmd/screentest/main.go
@@ -9,15 +9,14 @@ The comparisons are driven by a script file in a format described below.
 
 # Usage
 
-	screentest [flags] [glob]
+	screentest [flags] testURL wantURL file ...
+
+The first two arguments are the URLs being tested and the URL of the desired result, respectively.
+The remaining arguments are script file pathnames.
 
 The flags are:
 
-	-test URL
-		  URL or path being tested. Required.
-	-want URL
-		  URL or path for expected results. Required.
-	-c
+	 -c
 		  Number of test cases to run concurrently.
 	-d
 		  URL of a Chrome websocket debugger. If omitted, screentest tries to find the
@@ -92,9 +91,17 @@ Use wait SELECTOR to wait for an element to appear.
 
 	wait [role="treeitem"][aria-expanded="true"]
 
+Use eval JS to evaluate JavaScript snippets to hide elements or prepare the page in
+some other way.
+
+	eval 'document.querySelector(".selector").remove();'
+	eval 'window.scrollTo({top: 0});'
+
 Use capture [SIZE] [ARG] to create a test case with the properties
-defined in the test case. If present, the first argument to capture should be one of
-'fullscreen', 'viewport' or 'element'.
+defined in the test case. If present, the first argument to capture must be one of
+'fullscreen', 'viewport' or 'element'. The optional second argument provides
+a viewport size. The defaults are 'viewport' with dimensions specified by the windowsize
+directive.
 
 	capture fullscreen 540x1080
 
@@ -102,13 +109,7 @@ When taking an element screenshot provide a selector.
 
 	capture element header
 
-Use eval JS to evaluate JavaScript snippets to hide elements or prepare the page in
-some other way.
-
-	eval 'document.querySelector(".selector").remove();'
-	eval 'window.scrollTo({top: 0});'
-
-Each capture command to creates a new test case for a single page.
+Each capture directive creates a new test case for a single page.
 
 	windowsize 1536x960
 
@@ -132,15 +133,12 @@ import (
 	"fmt"
 	"log"
 	"os"
-	"path/filepath"
 	"runtime"
 )
 
 var flags options
 
 func init() {
-	flag.StringVar(&flags.testURL, "test", "", "URL or file path to test")
-	flag.StringVar(&flags.wantURL, "want", "", "URL or file path with expected results")
 	flag.BoolVar(&flags.update, "u", false, "update cached screenshots")
 	flag.StringVar(&flags.vars, "v", "", "variables provided to script templates as comma separated KEY:VALUE pairs")
 	flag.IntVar(&flags.maxConcurrency, "c", (runtime.NumCPU()+1)/2, "number of test cases to run concurrently")
@@ -151,10 +149,8 @@ func init() {
 }
 
 // options are the options for the program.
-// See the top command and the flag.XXXVar calls above for documentation.
+// See the top of this file and the flag.XXXVar calls above for documentation.
 type options struct {
-	testURL        string
-	wantURL        string
 	update         bool
 	vars           string
 	maxConcurrency int
@@ -166,22 +162,19 @@ type options struct {
 
 func main() {
 	flag.Usage = func() {
-		fmt.Printf("Usage: screentest [flags] [glob]\n")
+		fmt.Printf("usage: screentest [flags] testURL wantURL path ...\n")
+		fmt.Printf("\ttestURL is the URL or file path to be tested\n")
+		fmt.Printf("\twantURL is the URL or file path to compare it to\n")
+		fmt.Printf("\teach path is a script file to execute\n")
+
 		flag.PrintDefaults()
 	}
 	flag.Parse()
-	args := flag.Args()
-	// Require testdata glob when invoked as an installed command.
-	if len(args) != 1 && os.Args[0] == "screentest" {
+	if flag.NArg() < 3 {
 		flag.Usage()
-		os.Exit(1)
+		os.Exit(2)
 	}
-	glob := filepath.Join("cmd", "screentest", "testdata", "*")
-	if len(args) == 1 {
-		glob = args[0]
-	}
-
-	if err := run(context.Background(), glob, flags); err != nil {
+	if err := run(context.Background(), flag.Arg(0), flag.Arg(1), flag.Args()[2:], flags); err != nil {
 		log.Fatal(err)
 	}
 }
diff --git a/cmd/screentest/screentest.go b/cmd/screentest/screentest.go
index 8882faf6..d5cf3e6c 100644
--- a/cmd/screentest/screentest.go
+++ b/cmd/screentest/screentest.go
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+// TODO(jba): sleep directive
+// TODO(jba): specify percent of image that may differ
+// TODO(jba): remove ints function in template (see cmd/golangorg/testdata/screentest/relnotes.txt)
+
 package main
 
 import (
@@ -36,21 +40,22 @@ import (
 	"google.golang.org/api/iterator"
 )
 
-// run runs the test scripts matched by glob. If any errors are
-// encountered, run returns an error listing the problems.
-func run(ctx context.Context, glob string, opts options) error {
+// run compares testURL and wantURL using the test scripts in files and the options in opts.
+func run(ctx context.Context, testURL, wantURL string, files []string, opts options) error {
 	if opts.maxConcurrency < 1 {
 		opts.maxConcurrency = 1
 	}
 
 	now := time.Now()
 
-	files, err := filepath.Glob(glob)
-	if err != nil {
-		return fmt.Errorf("filepath.Glob(%q): %w", glob, err)
+	if testURL == "" {
+		return errors.New("missing URL or path to test")
+	}
+	if wantURL == "" {
+		return errors.New("missing URL or path with expected results")
 	}
 	if len(files) == 0 {
-		return fmt.Errorf("no files match %q", glob)
+		return errors.New("no files to run")
 	}
 	var cancel context.CancelFunc
 	if opts.debuggerURL != "" {
@@ -62,9 +67,10 @@ func run(ctx context.Context, glob string, opts options) error {
 		)...)
 	}
 	defer cancel()
+
 	var buf bytes.Buffer
 	for _, file := range files {
-		tests, err := readTests(file, opts)
+		tests, err := readTests(file, testURL, wantURL, opts)
 		if err != nil {
 			return fmt.Errorf("readTestdata(%q): %w", file, err)
 		}
@@ -240,7 +246,7 @@ func (t *testcase) String() string {
 }
 
 // readTests parses the testcases from a text file.
-func readTests(file string, opts options) ([]*testcase, error) {
+func readTests(file, testURL, wantURL string, opts options) ([]*testcase, error) {
 	tmpl := template.New(filepath.Base(file)).Funcs(template.FuncMap{
 		"ints": func(start, end int) []int {
 			var out []int
@@ -281,20 +287,23 @@ func readTests(file string, opts options) ([]*testcase, error) {
 	if err != nil {
 		return nil, fmt.Errorf("os.UserCacheDir(): %w", err)
 	}
-	if opts.testURL != "" {
-		originA = opts.testURL
-		if strings.HasSuffix(originA, cacheSuffix) {
-			originA = strings.TrimSuffix(originA, cacheSuffix)
-			cacheA = true
-		}
+	originA = testURL
+	if strings.HasSuffix(originA, cacheSuffix) {
+		originA = strings.TrimSuffix(originA, cacheSuffix)
+		cacheA = true
 	}
-	if opts.wantURL != "" {
-		originB = opts.wantURL
-		if strings.HasSuffix(originB, cacheSuffix) {
-			originB = strings.TrimSuffix(originB, cacheSuffix)
-			cacheB = true
-		}
+	if _, err := url.Parse(originA); err != nil {
+		return nil, fmt.Errorf("url.Parse(%q): %w", originA, err)
+	}
+	originB = wantURL
+	if strings.HasSuffix(originB, cacheSuffix) {
+		originB = strings.TrimSuffix(originB, cacheSuffix)
+		cacheB = true
+	}
+	if _, err := url.Parse(originB); err != nil {
+		return nil, fmt.Errorf("url.Parse(%q): %w", originB, err)
 	}
+
 	headers := map[string]any{}
 	hs, err := splitList(opts.headers)
 	if err != nil {
@@ -331,8 +340,8 @@ func readTests(file string, opts options) ([]*testcase, error) {
 		line = strings.TrimRight(line, " \t")
 		field, args := splitOneField(line)
 		field = strings.ToUpper(field)
-		if testName == "" && !slices.Contains([]string{"", "COMPARE", "TEST", "HEADER", "OUTPUT", "WINDOWSIZE"}, field) {
-			log.Printf("%s:%d: DEPRECATED: %q should only occur in a test", file, lineNo, strings.ToLower(field))
+		if testName == "" && !slices.Contains([]string{"", "TEST", "BLOCK", "WINDOWSIZE"}, field) {
+			return nil, fmt.Errorf("%s:%d: the %q directive should only occur in a test", file, lineNo, strings.ToLower(field))
 		}
 		switch field {
 		case "":
@@ -340,49 +349,11 @@ func readTests(file string, opts options) ([]*testcase, error) {
 			testName, pathname = "", ""
 			tasks = nil
 			status = http.StatusOK
-		case "COMPARE":
-			log.Printf("%s:%d: DEPRECATED: instead of 'compare', set the -test and -want flags", file, lineNo)
-			if originA != "" || originB != "" {
-				log.Printf("%s:%d: DEPRECATED: multiple 'compare's", file, lineNo)
-			}
-			origins := strings.Split(args, " ")
-			originA, originB = origins[0], origins[1]
-			cacheA, cacheB = false, false
-			if strings.HasSuffix(originA, cacheSuffix) {
-				originA = strings.TrimSuffix(originA, cacheSuffix)
-				cacheA = true
-			}
-			if strings.HasSuffix(originB, cacheSuffix) {
-				originB = strings.TrimSuffix(originB, cacheSuffix)
-				cacheB = true
-			}
-			if _, err := url.Parse(originA); err != nil {
-				return nil, fmt.Errorf("url.Parse(%q): %w", originA, err)
-			}
-			if _, err := url.Parse(originB); err != nil {
-				return nil, fmt.Errorf("url.Parse(%q): %w", originB, err)
-			}
-		case "HEADER":
-			log.Printf("%s:%d: DEPRECATED: instead of 'header', set the -headers flag", file, lineNo)
-			parts := strings.SplitN(args, ":", 2)
-			if len(parts) != 2 {
-				return nil, fmt.Errorf("invalid header %s on line %d", args, lineNo)
-			}
-			headers[strings.TrimSpace(parts[0])] = strings.TrimSpace(parts[1])
 		case "STATUS":
 			status, err = strconv.Atoi(args)
 			if err != nil {
 				return nil, fmt.Errorf("strconv.Atoi(%q): %w", args, err)
 			}
-		case "OUTPUT":
-			log.Printf("DEPRECATED: 'output': provide -o on the command line")
-			if strings.HasPrefix(args, gcsScheme) {
-				gcsBucket = true
-			}
-			out, err = outDir(args, file)
-			if err != nil {
-				return nil, err
-			}
 		case "WINDOWSIZE":
 			width, height, err = splitDimensions(args)
 			if err != nil {
@@ -421,9 +392,6 @@ func readTests(file string, opts options) ([]*testcase, error) {
 		case "BLOCK":
 			blockedURLs = append(blockedURLs, strings.Fields(args)...)
 		case "CAPTURE":
-			if originA == "" || originB == "" {
-				return nil, fmt.Errorf("missing compare for capture on line %d", lineNo)
-			}
 			if pathname == "" {
 				return nil, fmt.Errorf("missing pathname for capture on line %d", lineNo)
 			}
@@ -475,6 +443,10 @@ func readTests(file string, opts options) ([]*testcase, error) {
 				test.name += fmt.Sprintf(" %s", args)
 				test.screenshotType = elementScreenshot
 				test.screenshotElement = args
+			case "":
+				// nothing to do
+			default:
+				return nil, fmt.Errorf("first argument to capture must be 'fullscreen', 'viewport' or 'element'")
 			}
 			outfile := filepath.Join(out, sanitize(test.name))
 			if gcsBucket {
@@ -533,7 +505,7 @@ func splitOneField(text string) (field, rest string) {
 	if i < 0 {
 		return text, ""
 	}
-	return text[:i], strings.TrimLeft(text[i:], " \t")
+	return text[:i], strings.TrimSpace(text[i:])
 }
 
 // splitDimensions parses a window dimension string into int values
diff --git a/cmd/screentest/screentest_test.go b/cmd/screentest/screentest_test.go
index e4baba02..2a096a6d 100644
--- a/cmd/screentest/screentest_test.go
+++ b/cmd/screentest/screentest_test.go
@@ -20,7 +20,8 @@ import (
 
 func TestReadTests(t *testing.T) {
 	type args struct {
-		filename string
+		testURL, wantURL string
+		filename         string
 	}
 	d, err := os.UserCacheDir()
 	if err != nil {
@@ -37,12 +38,12 @@ func TestReadTests(t *testing.T) {
 		{
 			name: "readtests",
 			args: args{
+				testURL:  "https://go.dev",
+				wantURL:  "http://localhost:6060",
 				filename: "testdata/readtests.txt",
 			},
 			opts: options{
-				vars:    "Authorization:Bearer token",
-				testURL: "https://go.dev",
-				wantURL: "http://localhost:6060",
+				vars: "Authorization:Bearer token",
 			},
 			want: []*testcase{
 				{
@@ -139,11 +140,11 @@ func TestReadTests(t *testing.T) {
 		{
 			name: "readtests2",
 			args: args{
+				testURL:  "https://pkg.go.dev::cache",
+				wantURL:  "http://localhost:8080",
 				filename: "testdata/readtests2.txt",
 			},
 			opts: options{
-				testURL:   "https://pkg.go.dev::cache",
-				wantURL:   "http://localhost:8080",
 				headers:   "Authorization:Bearer token",
 				outputURL: "gs://bucket/prefix",
 			},
@@ -186,7 +187,7 @@ func TestReadTests(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := readTests(tt.args.filename, tt.opts)
+			got, err := readTests(tt.args.filename, tt.args.testURL, tt.args.wantURL, tt.opts)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("readTests() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -207,11 +208,12 @@ func TestRun(t *testing.T) {
 	// Skip this test if Google Chrome is not installed.
 	_, err := exec.LookPath("google-chrome")
 	if err != nil {
-		t.Skip()
+		t.Skip("google-chrome not installed")
 	}
 	type args struct {
-		glob   string
-		output string
+		testURL, wantURL string
+		output           string
+		file             string
 	}
 	d, err := os.UserCacheDir()
 	if err != nil {
@@ -228,19 +230,20 @@ func TestRun(t *testing.T) {
 		{
 			name: "pass",
 			args: args{
-				glob: "testdata/pass.txt",
-			},
-			opts: options{
 				testURL: "https://go.dev",
 				wantURL: "https://go.dev",
+				file:    "testdata/pass.txt",
 			},
+			opts:    options{},
 			wantErr: false,
 		},
 		{
 			name: "fail",
 			args: args{
-				output: filepath.Join(cache, "fail-txt"),
-				glob:   "testdata/fail.txt",
+				testURL: "https://go.dev",
+				wantURL: "https://pkg.go.dev",
+				file:    "testdata/fail.txt",
+				output:  filepath.Join(cache, "fail-txt"),
 			},
 			wantErr: true,
 			wantFiles: []string{
@@ -252,12 +255,12 @@ func TestRun(t *testing.T) {
 		{
 			name: "cached",
 			args: args{
-				output: "testdata/screenshots/cached",
-				glob:   "testdata/cached.txt",
+				testURL: "https://go.dev::cache",
+				wantURL: "https://go.dev::cache",
+				file:    "testdata/cached.txt",
+				output:  "testdata/screenshots/cached",
 			},
 			opts: options{
-				testURL:   "https://go.dev::cache",
-				wantURL:   "https://go.dev::cache",
 				outputURL: "testdata/screenshots/cached",
 			},
 			wantFiles: []string{
@@ -268,7 +271,7 @@ func TestRun(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := run(context.Background(), tt.args.glob, tt.opts); (err != nil) != tt.wantErr {
+			if err := run(context.Background(), tt.args.testURL, tt.args.wantURL, []string{tt.args.file}, tt.opts); (err != nil) != tt.wantErr {
 				t.Fatalf("CheckHandler() error = %v, wantErr %v", err, tt.wantErr)
 			}
 			if len(tt.wantFiles) != 0 {