Introduce first test for browser-side behavior. #798

ghemawat · 2023-08-16T16:33:20Z

Added a test that uses chromedp to drive web page interactions and check the resulting display.

Caveat: just adding a single test initially (it checks sorting of the "Top" view table). Other tests will be added later.

Caveat: coverage of browser Javascript is not gathered, so we won't be able to rely on coverage to measure quality/changes in Javascript testing.

codecov-commenter · 2023-08-16T22:09:01Z

Codecov Report

Merging #798 (2b14af0) into main (2861d24) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #798   +/-   ##
=======================================
  Coverage   66.99%   66.99%           
=======================================
  Files          45       45           
  Lines        9861     9861           
=======================================
  Hits         6606     6606           
  Misses       2795     2795           
  Partials      460      460

aalexand

One thing with integration style tests is how flaky they are, but I guess it's hard to know as we don't have bazel-style --runs_per_test flag to use.

aalexand · 2023-08-16T22:55:02Z

.github/workflows/ci.yaml

@@ -92,10 +92,11 @@ jobs:
          ./test.sh

      - name: Check to make sure that tests also work in GOPATH mode
+        if: matrix.go != 'tip'  # Is GO111MODULE support going away in go1.21?


nit: 1.22, not 1.21 (1.21 is out already). Same below.

aalexand · 2023-08-16T22:55:32Z

.github/workflows/ci.yaml

@@ -137,6 +138,14 @@ jobs:
        with:
          path: ${{ env.WORKING_DIR }}

+      # Limit to just Linux for now since this is expensive and the


aalexand · 2023-08-16T22:57:16Z

internal/driver/browser_test.go

+		if err != nil {
+			return fmt.Errorf("text %s: %v", query, err)
+		}
+		t.Logf("text %s:\n%s", query, value)


Was this for debugging or intended to stay?

I like leaving things like this in because if the test breaks, it is something useful for the fixer to look at -- they will see the extracted HTML.

A plain "go test" shows t.Logf() output only for failing runs. We should consider dropping the "-v" argument from the "go test" call in test.sh to reduce the clutter from our workflow runs.

Sounds good, including removing "-v". I think having the verbose output was helpful a couple of times, but I agree in most cases it's more likely to get in the way rather than be useful (often takes a lot of scrolling to see the error message).

aalexand · 2023-08-16T22:57:25Z

internal/driver/browser_test.go

+		if err != nil {
+			return fmt.Errorf("text %s: %v", query, err)
+		}
+		t.Logf("text %s:\n%s", query, value)


Ditto: debugging leftover?

aalexand · 2023-08-16T22:59:33Z

internal/driver/webui_test.go

@@ -43,6 +43,7 @@ func makeTestServer(t testing.TB, prof *profile.Profile) *httptest.Server {
 	creator := func(a *plugin.HTTPServerArgs) error {
 		server = httptest.NewServer(http.HandlerFunc(
 			func(w http.ResponseWriter, r *http.Request) {
+				t.Logf("serving URL %q", r.URL.Path)


nit: serving -> Serving

https://google.github.io/styleguide/go/decisions#error-strings

Dropping this one since I had added this just for temporary debugging.

aalexand · 2023-08-16T23:26:13Z

Will merge once tests complete.

Added a test that uses chromedp to drive web page interactions and check the resulting display. Disable "GOPATH" mode tests since "go get" support for that was deleted from Go on Aug 16 by golang/go@de4d503 Drop the "-v" from "go test" since it clutters logs. We will still get verbose info for failing tests, so diagnosing failures should not get any harder. Caveat: just adding a single test initially (it checks sorting of the "Top" view table). Other tests will be added later. Caveat: coverage of browser Javascript is not gathered, so we won't be able to rely on coverage to measure quality/changes in Javascript testing.

ghemawat · 2023-08-17T13:55:56Z

I sync-ed with other changes, dropped the unnecessary action to setup chrome (it is already available by default in github runners), and dropped the "-v" from the "go test" invocation.

This should now be fine to merge.

aalexand · 2023-08-17T17:45:57Z

internal/driver/browser_test.go

+	if _, err := exec.LookPath("chrome"); err == nil {
+		return
+	}
+	t.Skip("chrome not available")


FWIW I paused here for a second wondering if this should fail instead of skip so that we know early if the CI gets misconfigured somehow and starts silently skipping the test. But you added a chrome check in the CI configuration so probably we are good.

I think we indeed will need to skip here instead of failing the test, because otherwise the test will always fail in environments that do not have chrome, e.g. windows, mac (where we currently skip this test).

There is an earlier check that Skips unless we are on Linux+amd64. So we could easily change this one to Fail() which will reduce the long distance coupling to the workflow definition.

Louis-Ye · 2023-08-17T18:12:05Z

Marking #791 as fixed

Added a test that uses chromedp to drive web page interactions and check the resulting display. Disable "GOPATH" mode tests since "go get" support for that was deleted from Go on Aug 16 by golang/go@de4d503 Drop the "-v" from "go test" since it clutters logs. We will still get verbose info for failing tests, so diagnosing failures should not get any harder. Caveat: just adding a single test initially (it checks sorting of the "Top" view table). Other tests will be added later. Caveat: coverage of browser Javascript is not gathered, so we won't be able to rely on coverage to measure quality/changes in Javascript testing.

pohly · 2024-05-22T10:28:05Z

go.mod

+	github.com/gobwas/pool v0.2.1 // indirect
+	github.com/gobwas/ws v1.2.1 // indirect
+	github.com/josharian/intern v1.0.0 // indirect
+	github.com/mailru/easyjson v0.7.7 // indirect


github.com/mailru/easyjson is considered unmaintained in Kubernetes. Updating pprof to the latest version in Kubernetes triggers a check about this new dependency:

024/05/22 09:09:54 Unwanted module "github.com/mailru/easyjson" marked in /home/prow/go/src/k8s.io/kubernetes/hack/unwanted-dependencies.json is referenced by new dependants: 2024/05/22 09:09:54 github.com/google/pprof 2024/05/22 09:09:54 !!! Avoid updating referencing modules to versions that reintroduce use of unwanted dependencies

As this is presumable now a dependency only because of some integration test, do you see a chance to move this dependency into a separate package inside this repo?

github.com/chromedp/chromedp seems like pretty heavy dependency for a simple package like pprof.

Perhaps worth filing a separate issue for this so that we discuss it there.

ghemawat force-pushed the webtest branch 10 times, most recently from 3c5aee1 to 9218c12 Compare August 16, 2023 22:04

ghemawat force-pushed the webtest branch 2 times, most recently from aadbe0d to 6d2f13e Compare August 16, 2023 22:22

Louis-Ye mentioned this pull request Aug 16, 2023

Add tests for javascript code #791

Open

ghemawat force-pushed the webtest branch from 6d2f13e to b73ef43 Compare August 16, 2023 22:36

ghemawat requested a review from aalexand August 16, 2023 22:36

aalexand previously approved these changes Aug 16, 2023

View reviewed changes

ghemawat dismissed aalexand’s stale review via 0b24ea8 August 16, 2023 23:06

ghemawat force-pushed the webtest branch 2 times, most recently from 0b24ea8 to ca21896 Compare August 16, 2023 23:08

aalexand previously approved these changes Aug 16, 2023

View reviewed changes

ghemawat dismissed aalexand’s stale review via b7b1e34 August 17, 2023 13:19

ghemawat force-pushed the webtest branch 2 times, most recently from b7b1e34 to 719f4ce Compare August 17, 2023 13:22

ghemawat force-pushed the webtest branch from 719f4ce to 2b14af0 Compare August 17, 2023 13:37

aalexand approved these changes Aug 17, 2023

View reviewed changes

aalexand merged commit 7a8ec2a into google:main Aug 17, 2023
49 checks passed

pohly reviewed May 22, 2024

View reviewed changes

pohly mentioned this pull request May 22, 2024

extended dependencies because of browser test #862

Closed

ghemawat deleted the webtest branch July 16, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce first test for browser-side behavior. #798

Introduce first test for browser-side behavior. #798

ghemawat commented Aug 16, 2023

codecov-commenter commented Aug 16, 2023 •

edited

Loading

aalexand left a comment

aalexand Aug 16, 2023

ghemawat Aug 16, 2023

aalexand Aug 16, 2023

aalexand Aug 16, 2023

ghemawat Aug 16, 2023

aalexand Aug 16, 2023

aalexand Aug 16, 2023

aalexand Aug 16, 2023

ghemawat Aug 16, 2023

aalexand commented Aug 16, 2023

ghemawat commented Aug 17, 2023

aalexand Aug 17, 2023

Louis-Ye Aug 17, 2023

ghemawat Aug 17, 2023

Louis-Ye commented Aug 17, 2023

pohly May 22, 2024

aalexand May 22, 2024

pohly May 22, 2024

Introduce first test for browser-side behavior. #798

Introduce first test for browser-side behavior. #798

Conversation

ghemawat commented Aug 16, 2023

codecov-commenter commented Aug 16, 2023 • edited Loading

Codecov Report

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Aug 16, 2023

ghemawat commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Louis-Ye commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 16, 2023 •

edited

Loading