Prototype for viewing a diff on wpt.fyi #310

lukebjerring · 2017-11-28T19:44:16Z

Add-on for #301

Running at https://diff-visualizer-dot-wptdashboard.appspot.com

Example:
https://diff-visualizer-dot-wptdashboard.appspot.com/?before=edge@1db039e5b6&after=edge

lukebjerring · 2017-11-28T19:46:49Z

Considerations:

Browser summary should probably be extracted, display something more useful than Diff@diff
Diff is omitting entries for test-counts with unchanged results, which makes the numbers look strange.
Existing UI uses redness shade inversely proportional to number of differences, which makes no sense
Super slow to load

rwaldron · 2017-12-04T16:10:30Z

@lukebjerring ping for rebase—thanks!

lukebjerring · 2017-12-04T16:19:58Z

@rwaldron sync'd.

rwaldron · 2017-12-04T16:29:56Z

@lukebjerring Github is still reporting that there are conflicts, which I can confirm by pulling this locally. Can you run this in your local branch:

# assuming "upstream" is https://github.com/w3c/wptdashboard.git
git pull --rebase upstream master

Resolve the conflicts, commit, then...

git push HEAD -f

lukebjerring · 2017-12-04T16:33:17Z

Not sure what you're seeing, but I see:

This branch has no conflicts with the base branch
Merging can be performed automatically.

rwaldron · 2017-12-04T16:42:33Z

I just nuked everything local and started fresh and now I cannot reproduce this conflict that Github claims, so we're back on track.

rwaldron

I'm seeing the following warning in the console:

property-accessors.html:341 Polymer::Attributes: couldn't decode Array as JSON:
_deserializeValue @ property-accessors.html:341

And the following Uncaught exception:

Uncaught (in promise) TypeError: Cannot read property 'map' of null
test-file-results.html:107

Is this display intentional?

rwaldron · 2017-12-04T18:28:37Z

components/runs.html

@@ -41,7 +41,7 @@
        async connectedCallback () {
          super.connectedCallback()

-          if (!this.testRuns) {
+          if (this.testRunResources) {


I'm trying to understand this change—I don't see any other changes that involve this.testRunResources. Any details would be appreciated 👍

TestRun is an array of the JSON objects, which can be passed in directly. TestRunResources are URLs to fetch the JSON objects. This PR uses both (the TestRun for the diff is passed as a JSON object).

This will probably change anyway, since the single-result view 'diff' isn't needed, so will probably keep 'diff' as a separate property on the Polymer element.

Got it, thanks for the details!

rwaldron · 2017-12-04T18:31:56Z

components/runs.html

@@ -57,7 +57,7 @@
                .reduce((sum, item) => {
                  return Array.isArray(item) ? sum.concat(item) : [...sum, item]
                }, [])
-            this.testRuns = flattened
+            this.testRuns = this.testRuns ? [...flattened, ...this.testRuns] : flattened


this.testRuns = flattened.concat(this.testRuns || [])

wdyt?

rwaldron · 2017-12-04T18:41:58Z

components/wpt-results.html

@@ -364,6 +364,14 @@
              : '&'
          path += 'sha=' + this.sha
        }
+        var diffRuns = this.testRuns.filter(r => r['revision'] == 'diff')


Use MemberExpression.IdentifierName: r.revision == 'diff'

rwaldron · 2017-12-04T18:46:46Z

components/wpt-results.html

+                  ? '?'
+                  : '&'
+          path += 'before=' + this.testRuns[0]['browser_name'] + '@' + this.testRuns[0]['revision']
+          path += '&after=' + this.testRuns[1]['browser_name'] + '@' + this.testRuns[1]['revision']


use destructuring to unpack this.testRuns[0] and this.testRuns[1]:
let [before, after] = this.testRuns;

use MemberExpression.IdentifierName (for all cases here and throughout): before.browser_name, before.revision, after.browser_name & after.revision

use template string:
path += `before=${before.browser_name}@${before.revision}`

TIL about unpack.

rwaldron · 2017-12-04T19:01:11Z

Have you seen this?

lukebjerring · 2017-12-04T19:08:06Z

Looks like the doubling-up of runs is a bug on single test-files, will fix along with other issues.

rwaldron · 2017-12-04T19:13:18Z

doubling-up of runs is already a bug

I'm not seeing this on master—where are you seeing it?

lukebjerring · 2017-12-04T19:33:01Z

Sorry, edited comment to clarify what I meant.

…

On Mon, 4 Dec 2017 at 14:13 Rick Waldron ***@***.***> wrote: doubling-up of runs is already a bug I'm not seeing this on master—where are you seeing it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#310 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAve5Ji7XC3fZFJqYztOPxYm_Yeuyp8Dks5s9EROgaJpZM4Qtzme> .

rwaldron

One last minor change needed and this can be merged (feel free to merge once you make the change)

rwaldron · 2017-12-07T15:12:38Z

components/runs.html

@@ -41,7 +41,7 @@
        async connectedCallback () {
          super.connectedCallback()

-          if (!this.testRuns) {
+          if (this.testRunResources) {


Got it, thanks for the details!

rwaldron · 2017-12-07T15:14:22Z

components/test-file-results.html

@@ -144,6 +145,9 @@
      }

      resultsURL (testRun, testFile) {
+        if (testRun.revision == 'diff') {
+          return testRun.results_url + '&path=' + encodeURI(testFile)


Missed a template string update here.

rwaldron · 2017-12-07T15:14:49Z

components/wpt-results.html

-          path += '&after=' + this.testRuns[1]['browser_name'] + '@' + this.testRuns[1]['revision']
+          let [before, after] = this.testRuns
+          path += `before=${before.browser_name}@${before.revision}`
+          path += `&after=${after.browser_name}@${after.revision}`


mattl

Seems reasonable.

Luke Bjerring and others added 4 commits November 27, 2017 10:53

Migrate /test-runs and / to use /api/runs

f84b111

Merge master branch

90758c4

WIP

e9b0d97

Run gofmt

21cd29e

lukebjerring requested a review from foolip November 28, 2017 19:44

foolip requested review from rwaldron and mattl and removed request for foolip November 29, 2017 23:27

Merge master branch

be17bb3

rwaldron suggested changes Dec 4, 2017

View reviewed changes

Cleanups as per review

a187c33

rwaldron suggested changes Dec 7, 2017

View reviewed changes

mattl approved these changes Dec 7, 2017

View reviewed changes

rwaldron approved these changes Dec 14, 2017

View reviewed changes

rwaldron merged commit a187c33 into web-platform-tests:master Dec 14, 2017

lukebjerring deleted the run-diff-html branch December 21, 2017 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype for viewing a diff on wpt.fyi #310

Prototype for viewing a diff on wpt.fyi #310

lukebjerring commented Nov 28, 2017

lukebjerring commented Nov 28, 2017

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017

rwaldron commented Dec 4, 2017

rwaldron left a comment

rwaldron Dec 4, 2017

lukebjerring Dec 4, 2017

rwaldron Dec 7, 2017

rwaldron Dec 4, 2017

rwaldron Dec 4, 2017

rwaldron Dec 4, 2017

lukebjerring Dec 4, 2017

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017 •

edited

Loading

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017 via email

rwaldron left a comment

rwaldron Dec 7, 2017

rwaldron Dec 7, 2017

rwaldron Dec 7, 2017

mattl left a comment

Prototype for viewing a diff on wpt.fyi #310

Prototype for viewing a diff on wpt.fyi #310

Conversation

lukebjerring commented Nov 28, 2017

lukebjerring commented Nov 28, 2017

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017

rwaldron commented Dec 4, 2017

rwaldron 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

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017 • edited Loading

rwaldron commented Dec 4, 2017

lukebjerring commented Dec 4, 2017 via email

rwaldron 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

mattl left a comment

Choose a reason for hiding this comment

lukebjerring commented Dec 4, 2017 •

edited

Loading