A/B testing results page show results for the top-level tests instead of the one...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Jul 2017 03:45:00 +0000 (03:45 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Jul 2017 03:45:00 +0000 (03:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174304

Reviewed by Antti Koivisto.

When a specific subtest is analyzed (e.g. Images subtest of MotionMark), then TestGroupResultsViewer
should expand and highlight that specific subtest instead of simply showing the top-level test's score.
This is especially misleading since AnalysisResultsViewer (stacking bars for each test group) uses
the score of the specific subtest being analyzed.

Fixed the bug by passing in the metric associated with the analysis task from AnalysisTaskPage to
TestGroupResultsViewer via AnalysisTaskTestGroupPane. Also made TestGroupResultsViewer.setAnalysisResults
auto-expand the tests that are ancestors of the specified metric. Without that, the test won't be shown
to the user until the ancestor tests are expanded by the user.

Also fixed the bug that we were always listing sub-tests regardless of whether they have results or not.
Since tests tend to change over time, we shouldn't show a test if it doesn't have any results associated.

* public/v3/components/test-group-results-viewer.js:
(TestGroupResultsViewer.prototype.setAnalysisResults): Expand the ancestor tests of the metric.
(TestGroupResultsViewer.prototype._buildRowsForTest): Exit early if this test doesn't have any results.
* public/v3/models/analysis-results.js:
(AnalysisResults.prototype.containsTest): Added.
* public/v3/pages/analysis-task-page.js:
(AnalysisTaskTestGroupPane.prototype.setAnalysisResults): Takes a metric to pass it to the results viewer.
(AnalysisTaskPage.prototype._assignTestResultsIfPossible):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219321 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/v3/components/test-group-results-viewer.js
Websites/perf.webkit.org/public/v3/models/analysis-results.js
Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js

index d7f820a..ffbd679 100644 (file)
@@ -1,3 +1,32 @@
+2017-07-10  Ryosuke Niwa  <rniwa@webkit.org>
+
+        A/B testing results page show results for the top-level tests instead of the one being analyzed
+        https://bugs.webkit.org/show_bug.cgi?id=174304
+
+        Reviewed by Antti Koivisto.
+
+        When a specific subtest is analyzed (e.g. Images subtest of MotionMark), then TestGroupResultsViewer
+        should expand and highlight that specific subtest instead of simply showing the top-level test's score.
+        This is especially misleading since AnalysisResultsViewer (stacking bars for each test group) uses
+        the score of the specific subtest being analyzed.
+
+        Fixed the bug by passing in the metric associated with the analysis task from AnalysisTaskPage to
+        TestGroupResultsViewer via AnalysisTaskTestGroupPane. Also made TestGroupResultsViewer.setAnalysisResults
+        auto-expand the tests that are ancestors of the specified metric. Without that, the test won't be shown
+        to the user until the ancestor tests are expanded by the user.
+
+        Also fixed the bug that we were always listing sub-tests regardless of whether they have results or not.
+        Since tests tend to change over time, we shouldn't show a test if it doesn't have any results associated.
+
+        * public/v3/components/test-group-results-viewer.js:
+        (TestGroupResultsViewer.prototype.setAnalysisResults): Expand the ancestor tests of the metric.
+        (TestGroupResultsViewer.prototype._buildRowsForTest): Exit early if this test doesn't have any results.
+        * public/v3/models/analysis-results.js:
+        (AnalysisResults.prototype.containsTest): Added.
+        * public/v3/pages/analysis-task-page.js:
+        (AnalysisTaskTestGroupPane.prototype.setAnalysisResults): Takes a metric to pass it to the results viewer.
+        (AnalysisTaskPage.prototype._assignTestResultsIfPossible):
+
 2017-07-06  Ryosuke Niwa  <rniwa@webkit.org>
 
         Safari 10.1 fails to upload a patch on perf try bots page
index 10be0f2..0d97db8 100644 (file)
@@ -24,6 +24,12 @@ class TestGroupResultsViewer extends ComponentBase {
     {
         this._analysisResults = analysisResults;
         this._currentMetric = metric;
+        if (metric) {
+            const path = metric.test().path();
+            for (let i = path.length - 2; i >= 0; i--)
+                this._expandedTests.add(path[i]);
+        }
+
         this.enqueueToRender();
     }
 
@@ -58,6 +64,9 @@ class TestGroupResultsViewer extends ComponentBase {
 
     _buildRowsForTest(testGroup, expandedTests, test, sharedPath, maxDepth, depth)
     {
+        if (!this._analysisResults.containsTest(test))
+            return [];
+
         const element = ComponentBase.createElement;
         const rows = element('tbody', test.metrics().map((metric) => this._buildRowForMetric(testGroup, metric, sharedPath, maxDepth, depth)));
 
index 0bb165c..bc395af 100644 (file)
@@ -17,6 +17,16 @@ class AnalysisResults {
 
     highestTests() { return this._lazilyComputedHighestTests.evaluate(this._metricIds); }
 
+    containsTest(test)
+    {
+        console.assert(test instanceof Test);
+        for (let metric of test.metrics()) {
+            if (metric.id() in this._metricToBuildMap)
+                return true;
+        }
+        return false;
+    }
+
     _computeHighestTests(metricIds)
     {
         const testsInResults = new Set(metricIds.map((metricId) => Metric.findById(metricId).test()));
index 3dc4358..0a6a02e 100644 (file)
@@ -280,10 +280,10 @@ class AnalysisTaskTestGroupPane extends ComponentBase {
         this.enqueueToRender();
     }
 
-    setAnalysisResults(analysisResults)
+    setAnalysisResults(analysisResults, metric)
     {
         this.part('revision-table').setAnalysisResults(analysisResults);
-        this.part('results-viewer').setAnalysisResults(analysisResults, null);
+        this.part('results-viewer').setAnalysisResults(analysisResults, metric);
         this.enqueueToRender();
     }
 
@@ -613,8 +613,8 @@ class AnalysisTaskPage extends PageWithHeading {
         if (!this._task || !this._testGroups || !this._analysisResults)
             return false;
 
-        this.part('group-pane').setAnalysisResults(this._analysisResults);
         let metric = this._metric;
+        this.part('group-pane').setAnalysisResults(this._analysisResults, metric);
         if (metric) {
             const view = this._analysisResults.viewForMetric(metric);
             this.part('results-pane').setAnalysisResultsView(view);