From 274da7ade3b774f79ae610377643f861f0c74cc6 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Thu, 14 Nov 2024 07:48:58 -0500 Subject: [PATCH] cmd/screentest: doc headless-shell; improve output - Document the preference for headless-shell, the stripped-down Chrome binary. It is more reliable. - Clarify output, reduce vertical whitespace. - Fix race condition. Change-Id: I8d879bf4442a089e1b3eeb47074c76fe46e87a53 Reviewed-on: https://go-review.googlesource.com/c/website/+/628056 Reviewed-by: Hyang-Ah Hana Kim LUCI-TryBot-Result: Go LUCI --- cmd/golangorg/README.md | 5 +- cmd/golangorg/testdata/screentest/godev.txt | 17 +- cmd/screentest/main.go | 63 +++++-- cmd/screentest/screentest.go | 192 ++++++++++++++++---- cmd/screentest/screentest_test.go | 3 + cmd/screentest/testdata/readtests2.txt | 1 + 6 files changed, 220 insertions(+), 61 deletions(-) diff --git a/cmd/golangorg/README.md b/cmd/golangorg/README.md index 717231565f..5cb5e27411 100644 --- a/cmd/golangorg/README.md +++ b/cmd/golangorg/README.md @@ -31,8 +31,11 @@ The go.dev web site has a suite of visual checks that can be run with: ./cmd/golangorg/screentest.sh These checks can be run locally and will generate visual diffs of web pages -from the set of testcases in `cmd/golangorg/testdata/screentest/*.txt`, comparing screenshots +from the set of test cases in `cmd/golangorg/testdata/screentest/*.txt`, comparing screenshots of the live server and a locally running instance of cmd/golangorg. +Screentest will start Chrome locally to render the pages, but that can be unreliable. +Prefer Chrome headless-shell. See the documentation at the top of cmd/screenshot/main.go +for more. ## Deploying to go.dev and golang.org diff --git a/cmd/golangorg/testdata/screentest/godev.txt b/cmd/golangorg/testdata/screentest/godev.txt index b33a4b6a8c..ddba5efb31 100644 --- a/cmd/golangorg/testdata/screentest/godev.txt +++ b/cmd/golangorg/testdata/screentest/godev.txt @@ -2,11 +2,16 @@ windowsize 1536x960 test homepage path / +# Wait for the playground to run the sample program. +sleep 4s capture fullscreen capture fullscreen 540x1080 test why go case studies path /solutions/case-studies +# Scrolling to bottom causes lazy-loading images to load. +eval window.scrollTo({top: document.body.scrollHeight}); +sleep 1s capture fullscreen capture fullscreen 540x1080 @@ -15,10 +20,14 @@ path /solutions/use-cases capture fullscreen capture fullscreen 540x1080 -test getting started -path /learn/ -capture fullscreen -capture fullscreen 540x1080 +# This test will fail because the local server +# uses fake download information, so the download button +# will have a different Go version. +# +# test getting started +# path /learn/ +# capture fullscreen +# capture fullscreen 540x1080 test docs path /doc/ diff --git a/cmd/screentest/main.go b/cmd/screentest/main.go index 4b7bfc9244..8ed3c7ecaa 100644 --- a/cmd/screentest/main.go +++ b/cmd/screentest/main.go @@ -19,27 +19,45 @@ can be slash-separated file paths (even on Windows). The flags are: - -c - Number of test cases to run concurrently. + -c + Number of test cases to run concurrently. -d - URL of a Chrome websocket debugger. If omitted, screentest tries to find the - Chrome executable on the system and starts a new instance. + URL of a Chrome websocket debugger. If omitted, screentest uses the + Chrome executable on the command path. It will look first for the + headless-shell binary, which is preferred. -headers - HTTP(S) headers to send with each request, as a comma-separated list of name:value. + HTTP(S) headers to send with each request, as a comma-separated list of name:value. -run REGEXP - Run only tests matching regexp. - -o - URL or slash-separated path for output files. If omitted, files are written - to a subdirectory of the user's cache directory. Each test file is given - its own directory, so test names in two files can be identical. But the directory - name is the basename of the test file with the extension removed. Conflicting - file names will overwrite each other. - -u - Instead of comparing screenshots, use the test screenshots to update the - want screenshots. This only makes sense if wantURL is a storage location - like a file path or GCS bucket. - -v - Variables provided to script templates as comma separated KEY:VALUE pairs. + Run only tests matching regexp. + -o + URL or slash-separated path where output files for failing tests are written. + If omitted, files are written to a subdirectory of the user's cache directory. + At the start of each run, existing files are removed. + Each test file is given its own directory, so test names in two files can be identical, + but the directory name is the basename of the test file with the extension removed, so + files with identical basenames will overwrite each other. + -u + Instead of comparing screenshots, use the test screenshots to update the + want screenshots. This only makes sense if wantURL is a storage location + like a file path or GCS bucket. + -v + Variables provided to script templates as comma-separated KEY:VALUE pairs. + +# Headless Chrome + +Screentest needs a headless Chrome process to render web pages. Although it can use a full +Chrome browser, we have found the headless-shell build of Chrome to be more reliable. +Install headless-shell on your local machine with this command: + + npx @puppeteer/browsers install chrome-headless-shell@VERSION + +Put the binary on your path and screentest will find it. Omit the -d flag in this case. + +You can also run headless-shell in docker. We use this command: + + docker run --detach --rm --network host --shm-size 8G --name headless-shell chromedp/headless-shell:VERSION + +Then pass "-d ws://localhost:9222" to screentest. # Scripts @@ -105,6 +123,11 @@ some other way. eval 'document.querySelector(".selector").remove();' eval 'window.scrollTo({top: 0});' +Use sleep DURATION to pause the browser for the duration. This is a last resort +for deflaking; prefer to wait for an element. + + sleep 50ms + Use capture [SIZE] [ARG] to create a test case with the properties 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 @@ -169,6 +192,8 @@ type options struct { } func main() { + log.SetFlags(0) + log.SetPrefix("screentest: ") flag.Usage = func() { fmt.Printf("usage: screentest [flags] testURL wantURL path ...\n") fmt.Printf("\ttestURL is the URL or file path to be tested\n") @@ -184,5 +209,7 @@ func main() { } if err := run(context.Background(), flag.Arg(0), flag.Arg(1), flag.Args()[2:], flags); err != nil { log.Fatal(err) + } else { + log.Print("PASS") } } diff --git a/cmd/screentest/screentest.go b/cmd/screentest/screentest.go index 5648b9ef25..449c753c88 100644 --- a/cmd/screentest/screentest.go +++ b/cmd/screentest/screentest.go @@ -2,17 +2,14 @@ // 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) -// TODO(jba): write index.html to outdir with a nice view of all the failures -// TODO(jba): debug -run regexp matching package main import ( "bufio" "bytes" + "cmp" "context" "errors" "fmt" @@ -27,6 +24,7 @@ import ( "regexp" "strconv" "strings" + "sync" "text/template" "time" @@ -41,7 +39,7 @@ import ( // 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 { - now := time.Now() + start := time.Now() if testURL == "" { return errors.New("missing URL or path to test") @@ -75,7 +73,10 @@ func run(ctx context.Context, testURL, wantURL string, files []string, opts opti return err } - var buf bytes.Buffer + var ( + summary bytes.Buffer + failedTests []*testcase // tests that failed and wrote diffs + ) for _, file := range files { tests, err := readTests(file, testURL, wantURL, c) if err != nil { @@ -96,27 +97,90 @@ func run(ctx context.Context, testURL, wantURL string, files []string, opts opti if opts.maxConcurrency < 1 { opts.maxConcurrency = 1 } - var hdr bool + + var ( + mu sync.Mutex + hdr bool + ) runConcurrently(len(tests), opts.maxConcurrency, func(i int) { tc := tests[i] if err := tc.run(ctx, opts.update); err != nil { + mu.Lock() if !hdr { - fmt.Fprintf(&buf, "%s\n\n", file) + fmt.Fprintf(&summary, "%s\n", file) hdr = true } - fmt.Fprintf(&buf, "%v\n", err) - fmt.Fprintf(&buf, "inspect diff at %s\n\n", path.Join(tc.failImageWriter.path(), tc.diffPath)) + fmt.Fprintf(&summary, "%v\n", err) + if tc.wroteDiff { + failedTests = append(failedTests, tc) + } + mu.Unlock() } fmt.Println(tc.output.String()) }) } - fmt.Printf("finished in %s\n\n", time.Since(now).Truncate(time.Millisecond)) - if buf.Len() > 0 { - return errors.New(buf.String()) + fmt.Printf("finished in %s\n\n", time.Since(start).Truncate(time.Millisecond)) + if summary.Len() > 0 { + os.Stdout.Write(summary.Bytes()) + if len(failedTests) > 0 { + data := failedImagesPage(failedTests) + if err := c.failImageWriter.writeData(ctx, "index.html", data); err != nil { + return err + } + } + return fmt.Errorf("FAIL. Output at %s", c.failImageWriter.path()) } return nil } +// failedImagesPage builds a web page that displays the images for failed tests. +func failedImagesPage(failedTests []*testcase) []byte { + var buf bytes.Buffer + + p := func(format string, args ...any) { + fmt.Fprintf(&buf, format, args...) + } + + p(` + + + + + +

Screentest Failures

+ `) + for _, tc := range failedTests { + p("

%s

\n", tc.name) + p("\n") + p(` + + + + + + `, tc.testOrigin(), tc.wantOrigin()) + p(` + + + + + + `, tc.testPath, tc.wantPath, tc.diffPath) + p("
Got: %sWant: %sDiff
\n") + } + p(` + + + `) + + return buf.Bytes() +} + const ( browserWidth = 1536 browserHeight = 960 @@ -149,6 +213,7 @@ type testcase struct { testURL, wantURL string // URL to visit if the command-line arg is http/https testPath, wantPath string // slash-separated path to use if the command-line arg is file, gs or a path diffPath string // output path for failed tests + wroteDiff bool // test failed and diffPath was written status int viewportWidth int viewportHeight int @@ -158,6 +223,14 @@ type testcase struct { output bytes.Buffer } +// testOrigin returns the origin of the test image: either an http(s) URL or a +// storage path. +func (tc *testcase) testOrigin() string { return cmp.Or(tc.testURL, tc.testPath) } + +// wantOrigin returns the origin of the want image: either an http(s) URL or a +// storage path. +func (tc *testcase) wantOrigin() string { return cmp.Or(tc.wantURL, tc.wantPath) } + // commonValues returns values common to all test files. func commonValues(ctx context.Context, testURL, wantURL string, opts options) (c common, err error) { // The test/want image readers/writers are relative to the test/want URLs, so @@ -171,6 +244,9 @@ func commonValues(ctx context.Context, testURL, wantURL string, opts options) (c if err != nil { return common{}, err } + if opts.update && c.wantImageReadWriter == nil { + return common{}, fmt.Errorf("cannot update a non-storage wantURL: %s", wantURL) + } outDirPath := opts.outputDirURL if outDirPath == "" { @@ -234,7 +310,9 @@ func readTests(file, testURL, wantURL string, common common) (_ []*testcase, err testNames := map[string]bool{} // to detect duplicates - defer wrapf(&err, "%s:%d", file, lineNo) + defer func() { + wraperr(&err, "%s:%d", file, lineNo) + }() scan := bufio.NewScanner(bytes.NewReader(data)) for scan.Scan() { @@ -244,8 +322,8 @@ func readTests(file, testURL, wantURL string, common common) (_ []*testcase, err continue } line = strings.TrimRight(line, " \t") - directive, args := splitOneField(line) - directive = strings.ToUpper(directive) + origDirective, args := splitOneField(line) + directive := strings.ToUpper(origDirective) switch directive { case "": // An empty line means the end of a test (if one is active). @@ -262,16 +340,22 @@ func readTests(file, testURL, wantURL string, common common) (_ []*testcase, err } case "BLOCK": - blockedURLs = append(blockedURLs, strings.Fields(args)...) + urls := strings.Fields(args) + if test != nil { + test.blockedURLs = append(test.blockedURLs, urls...) + } else { + blockedURLs = append(blockedURLs, urls...) + } case "TEST": if test != nil { return nil, errors.New("no blank lines between tests") } test = &testcase{ - common: common, - name: args, - status: http.StatusOK, + common: common, + name: args, + status: http.StatusOK, + blockedURLs: blockedURLs, } if testNames[test.name] { return nil, fmt.Errorf("duplicate test name %q", test.name) @@ -325,6 +409,19 @@ func readTests(file, testURL, wantURL string, common common) (_ []*testcase, err } test.tasks = append(test.tasks, chromedp.Evaluate(args, nil)) + case "SLEEP": + if test == nil { + return nil, errors.New("directive must be in a test") + } + if ns, err := strconv.Atoi(args); err == nil { + return nil, fmt.Errorf("sleep argument of %d is in nanoseconds; did you mean %[1]ds?", ns) + } + dur, err := time.ParseDuration(args) + if err != nil { + return nil, err + } + test.tasks = append(test.tasks, chromedp.Sleep(dur)) + case "CAPTURE": if test == nil { return nil, errors.New("directive must be in a test") @@ -374,7 +471,7 @@ func readTests(file, testURL, wantURL string, common common) (_ []*testcase, err test = &clone default: - return nil, fmt.Errorf("unknown directive %q", directive) + return nil, fmt.Errorf("unknown directive %q", origDirective) } if directive != "" { lastDirective = directive @@ -480,6 +577,7 @@ func splitDimensions(text string) (width, height int, err error) { // run generates screenshots for a given test case and a diff if the // screenshots do not match. func (tc *testcase) run(ctx context.Context, update bool) (err error) { + defer wraperr(&err, "test %s", tc.name) now := time.Now() fmt.Fprintf(&tc.output, "test %s ", tc.name) var testScreen, wantScreen image.Image @@ -511,10 +609,11 @@ func (tc *testcase) run(ctx context.Context, update bool) (err error) { }) since := time.Since(now).Truncate(time.Millisecond) if result.Equal { - fmt.Fprintf(&tc.output, "(%s)\n", since) + fmt.Fprintf(&tc.output, "(%s)", since) return nil } - fmt.Fprintf(&tc.output, "(%s)\nFAIL %s != %s (%d pixels differ)\n", since, tc.testURL, tc.wantURL, result.DiffPixelsCount) + fmt.Fprintf(&tc.output, "(%s)\n FAIL %s != %s (%d pixels differ)\n", + since, tc.testOrigin(), tc.wantOrigin(), result.DiffPixelsCount) g, gctx := errgroup.WithContext(ctx) g.Go(func() error { return tc.failImageWriter.writeImage(gctx, tc.testPath, testScreen) }) g.Go(func() error { return tc.failImageWriter.writeImage(gctx, tc.wantPath, wantScreen) }) @@ -522,8 +621,9 @@ func (tc *testcase) run(ctx context.Context, update bool) (err error) { if err := g.Wait(); err != nil { return err } - fmt.Fprintf(&tc.output, "wrote diff to %s\n", path.Join(tc.failImageWriter.path(), tc.diffPath)) - return fmt.Errorf("%s != %s", tc.testURL, tc.wantURL) + fmt.Fprintf(&tc.output, " wrote diff to %s", path.Join(tc.failImageWriter.path(), tc.diffPath)) + tc.wroteDiff = true + return fmt.Errorf("%s != %s", tc.testOrigin(), tc.wantOrigin()) } // screenshot gets a screenshot for a testcase url. If reader is non-nil @@ -680,6 +780,7 @@ type imageReader interface { // An imageWriter writes images to slash-separated paths. type imageWriter interface { writeImage(ctx context.Context, path string, img image.Image) error + writeData(ct context.Context, path string, data []byte) error rmdir(ctx context.Context, path string) error path() string // return the slash-separated path that this was created with } @@ -689,8 +790,6 @@ type imageReadWriter interface { imageWriter } -var validSchemes = []string{"file", "gs", "http", "https"} - // newImageReadWriter returns an imageReadWriter for loc. // loc can be a URL with a scheme or a slash-separated file path. func newImageReadWriter(ctx context.Context, loc string) (imageReadWriter, error) { @@ -722,7 +821,7 @@ type dirImageReadWriter struct { func (rw *dirImageReadWriter) readImage(_ context.Context, path string) (_ image.Image, err error) { path = rw.nativePathname(path) - defer wrapf(&err, "reading image from %s", path) + defer wraperr(&err, "reading image from %s", path) f, err := os.Open(path) if err != nil { return nil, err @@ -732,9 +831,17 @@ func (rw *dirImageReadWriter) readImage(_ context.Context, path string) (_ image return img, err } -func (rw *dirImageReadWriter) writeImage(_ context.Context, path string, img image.Image) (err error) { +func (rw *dirImageReadWriter) writeImage(ctx context.Context, path string, img image.Image) error { + var buf bytes.Buffer + if err := png.Encode(&buf, img); err != nil { + return err + } + return rw.writeData(ctx, path, buf.Bytes()) +} + +func (rw *dirImageReadWriter) writeData(_ context.Context, path string, data []byte) (err error) { path = rw.nativePathname(path) - defer wrapf(&err, "writing %s", path) + defer wraperr(&err, "writing %s", path) if err := os.MkdirAll(filepath.Dir(path), os.ModePerm); err != nil { return err @@ -746,7 +853,8 @@ func (rw *dirImageReadWriter) writeImage(_ context.Context, path string, img ima defer func() { err = errors.Join(err, f.Close()) }() - return png.Encode(f, img) + _, err = f.Write(data) + return err } func (rw *dirImageReadWriter) rmdir(_ context.Context, path string) error { @@ -785,7 +893,7 @@ func newGCSImageReadWriter(ctx context.Context, urlstr string) (*gcsImageReadWri } func (rw *gcsImageReadWriter) readImage(ctx context.Context, pth string) (_ image.Image, err error) { - defer wrapf(&err, "reading %s", path.Join(rw.url, pth)) + defer wraperr(&err, "reading %s", path.Join(rw.url, pth)) r, err := rw.bucket.Object(rw.objectName(pth)).NewReader(ctx) if err != nil { @@ -796,13 +904,21 @@ func (rw *gcsImageReadWriter) readImage(ctx context.Context, pth string) (_ imag return img, err } -func (rw *gcsImageReadWriter) writeImage(ctx context.Context, pth string, img image.Image) (err error) { - defer wrapf(&err, "writing %s", path.Join(rw.url, pth)) +func (rw *gcsImageReadWriter) writeImage(ctx context.Context, path string, img image.Image) error { + var buf bytes.Buffer + if err := png.Encode(&buf, img); err != nil { + return err + } + return rw.writeData(ctx, path, buf.Bytes()) +} + +func (rw *gcsImageReadWriter) writeData(ctx context.Context, pth string, data []byte) (err error) { + defer wraperr(&err, "writing %s", path.Join(rw.url, pth)) cctx, cancel := context.WithCancel(ctx) defer cancel() w := rw.bucket.Object(rw.objectName(pth)).NewWriter(cctx) - if err := png.Encode(w, img); err != nil { + if _, err := w.Write(data); err != nil { cancel() _ = w.Close() return err @@ -811,7 +927,7 @@ func (rw *gcsImageReadWriter) writeImage(ctx context.Context, pth string, img im } func (rw *gcsImageReadWriter) rmdir(ctx context.Context, pth string) (err error) { - defer wrapf(&err, "rmdir %s", path.Join(rw.url, pth)) + defer wraperr(&err, "rmdir %s", path.Join(rw.url, pth)) prefix := path.Join(rw.prefix, pth) if !strings.HasSuffix(prefix, "/") { @@ -864,8 +980,8 @@ func runConcurrently(n, max int, f func(int)) { } } -// wrapf prepends a non-nil *errp with the given message, formatted by fmt.Sprintf. -func wrapf(errp *error, format string, args ...any) { +// wraperr prepends a non-nil *errp with the given message, formatted by fmt.Sprintf. +func wraperr(errp *error, format string, args ...any) { if *errp != nil { *errp = fmt.Errorf("%s: %w", fmt.Sprintf(format, args...), *errp) } diff --git a/cmd/screentest/screentest_test.go b/cmd/screentest/screentest_test.go index 9f90edeb29..60187180de 100644 --- a/cmd/screentest/screentest_test.go +++ b/cmd/screentest/screentest_test.go @@ -238,6 +238,9 @@ func TestReadTests(t *testing.T) { t.Run(tt.name, func(t *testing.T) { filename := filepath.Join("testdata", tt.name+".txt") comm, err := commonValues(ctx, tt.testURL, tt.wantURL, tt.opts) + if err != nil { + t.Fatal(err) + } got, err := readTests(filename, tt.testURL, tt.wantURL, comm) if (err != nil) != tt.wantErr { t.Fatalf("readTests() error = %v, wantErr %v", err, tt.wantErr) diff --git a/cmd/screentest/testdata/readtests2.txt b/cmd/screentest/testdata/readtests2.txt index e39c07088f..67a1e6f442 100644 --- a/cmd/screentest/testdata/readtests2.txt +++ b/cmd/screentest/testdata/readtests2.txt @@ -7,5 +7,6 @@ capture test eval path /eval eval console.log('hello, world!') +sleep 2s4ms capture