Add initial flamegraph web interface test. #802

ghemawat · 2023-09-01T19:57:17Z

We now do some rudimentary testing of the flamegraph web interface. The test code is written in Javascript and uses a simple test fixture class.

codecov-commenter · 2023-09-01T20:29:10Z

Codecov Report

Merging #802 (f76c90e) into main (20cde90) will not change coverage.
The diff coverage is n/a.

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

internal/driver/testdata/testflame.js

aalexand · 2023-09-06T05:57:05Z

internal/driver/testdata/testfixture.js

+  setContext(name, ...args) {
+    let str = name + "(";
+    let sep = "";
+    for (const arg of args) {


Can we use args.join(",")?

Good idea. Done.

aalexand · 2023-09-06T06:01:55Z

internal/driver/testdata/testfixture.js

+  }
+
+  log(...args) {
+    this.result.push(this.toArray("LOG", this.context, ...args));


I don't know JS that well, but poking around a bit I think this can be just this.result.push("LOG", this.context, ...args);.

Thanks. You were close. I needed:

this.result.push(["LOG", this.context, ...args]);

aalexand · 2023-09-06T06:06:08Z

internal/driver/testdata/testflame.js

@@ -0,0 +1,86 @@
+(function () {


nit: I would maybe give the function a name and then call it explicitly at the end. This syntax with parens gave me a bit of a pause when reading the code.

aalexand · 2023-09-06T06:11:08Z

internal/driver/testdata/testflame.js

+      test.err("not above");
+    }
+    // TODO: Allow checking boxes above pivots.
+    if (rb.left < ra.left || rb.right > rb.right) {


rb.right comparison should be rb.right > ra.right?

Good catch. Fixed.

aalexand · 2023-09-06T06:11:45Z

internal/driver/testdata/testflame.js

+    }
+    // TODO: Allow checking boxes above pivots.
+    if (rb.left < ra.left || rb.right > rb.right) {
+      test.err("horizontal span is not nested inside horizontal span of");


The "inside horizontal span of" part reads unfinished. Or maybe I misread it.

aalexand · 2023-09-06T06:12:42Z

internal/driver/testdata/testflame.js

+  // rect gets the bounding box for box with text t.
+  function rect(t) {
+    const elem = boxMap.get(t);
+    if (!elem) test.err("did not find", t);


Should we return here? Otherwise it seems we'd be dereferencing a null in the call in the next line.

ghemawat

Thanks. Suggested changes made.

ghemawat · 2023-09-06T15:09:13Z

internal/driver/testdata/testfixture.js

+  setContext(name, ...args) {
+    let str = name + "(";
+    let sep = "";
+    for (const arg of args) {


Good idea. Done.

ghemawat · 2023-09-06T15:10:46Z

internal/driver/testdata/testfixture.js

+  }
+
+  log(...args) {
+    this.result.push(this.toArray("LOG", this.context, ...args));


Thanks. You were close. I needed:

this.result.push(["LOG", this.context, ...args]);

ghemawat · 2023-09-06T15:13:30Z

internal/driver/testdata/testflame.js

@@ -0,0 +1,86 @@
+(function () {


ghemawat · 2023-09-06T15:14:26Z

internal/driver/testdata/testflame.js

+  // rect gets the bounding box for box with text t.
+  function rect(t) {
+    const elem = boxMap.get(t);
+    if (!elem) test.err("did not find", t);


ghemawat · 2023-09-06T15:15:17Z

internal/driver/testdata/testflame.js

+      test.err("not above");
+    }
+    // TODO: Allow checking boxes above pivots.
+    if (rb.left < ra.left || rb.right > rb.right) {


Good catch. Fixed.

ghemawat · 2023-09-06T15:16:27Z

internal/driver/testdata/testflame.js

+    }
+    // TODO: Allow checking boxes above pivots.
+    if (rb.left < ra.left || rb.right > rb.right) {
+      test.err("horizontal span is not nested inside horizontal span of");


We now do some rudimentary testing of the flamegraph web interface. The test code is written in Javascript and uses a simple test fixture class.

ghemawat · 2023-09-06T21:02:40Z

Cleaned up branch history by moving all changes made here into a single commit at the end.

aalexand · 2023-09-06T21:19:44Z

internal/driver/testdata/testflame.js

+    test.setContext("checkWidth", t, fraction);
+    const r = rect(t);
+    if (!r) return;
+    const expect = (chartRect.width - 4) * fraction;


nit: a named constant for the 4 might be good, it's a bit obscure as is.

That's a good point. Done, and thanks.

Question: I added the change as a separate commit. Do you prefer I amend the previous commit instead during code review and force push to github, or will you squash when you merge into main?

My 2 cents: personally I think amending the commit is better than squashing it into one, because it would be easier to relate the comments and CI runs to code in the PR.

We will squash them when merging into main.

I think you meant "adding a new commit" instead of "amending the commit"?

I agree with the decision to not squash until merging into main. Also makes it easier for reviewers to see the changes since the last round of reviews.

Thanks.

Right, I think "adding a new commit" is better than "amending" (in between reviews), and is much better than "squashing" all previous commits into one.

Thanks!

I don't have strong preference, separate commits are easier so probably the default choice. One added advantage is being able to move out a specific part of the change into a separate PR if requested by a reviewer - easier to do if it's a separate commit. Doesn't happen that often though, at least for this repo.

Louis-Ye

LGTM

ghemawat requested a review from aalexand September 1, 2023 20:07

aalexand reviewed Sep 5, 2023

View reviewed changes

internal/driver/testdata/testflame.js Show resolved Hide resolved

aalexand reviewed Sep 6, 2023

View reviewed changes

ghemawat commented Sep 6, 2023

View reviewed changes

Add initial flamegraph web interface test.

196b82f

We now do some rudimentary testing of the flamegraph web interface. The test code is written in Javascript and uses a simple test fixture class.

ghemawat force-pushed the flametest branch from 55d20c0 to 196b82f Compare September 6, 2023 21:01

aalexand previously approved these changes Sep 6, 2023

View reviewed changes

Add named PADDING constant and use it in test.

f76c90e

ghemawat dismissed aalexand’s stale review via f76c90e September 7, 2023 15:38

Louis-Ye approved these changes Sep 7, 2023

View reviewed changes

aalexand merged commit d3ddc79 into google:main Sep 7, 2023

ghemawat deleted the flametest branch September 8, 2023 00:21

yktakaha4 mentioned this pull request Nov 24, 2023

webhtml: fill search input from url parameter #820

Open

Add initial flamegraph web interface test. #802

Add initial flamegraph web interface test. #802

Uh oh!

Conversation

ghemawat commented Sep 1, 2023

Uh oh!

codecov-commenter commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghemawat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghemawat commented Sep 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Louis-Ye Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Louis-Ye left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Sep 1, 2023 •

edited

Loading

Louis-Ye Sep 7, 2023 •

edited

Loading