Test group results notification should not say a build request to build had failed...
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 23:26:52 +0000 (23:26 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 23:26:52 +0000 (23:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193064

Reviewed by Ryosuke Niwa.

Should show 'Build completed' or 'Build failed' for build type build requests.

* browser-tests/test-group-result-page-tests.js: Added a unit test to guard this bug.
* tools/js/test-group-result-page.js: Show 'Build completed' or 'Build failed' for build type build requests.

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/test-group-result-page-tests.js
Websites/perf.webkit.org/tools/js/test-group-result-page.js

index 4156c10..1b41349 100644 (file)
@@ -1,3 +1,15 @@
+2018-12-31  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Test group results notification should not say a build request to build had failed even when it had successfully completed.
+        https://bugs.webkit.org/show_bug.cgi?id=193064
+
+        Reviewed by Ryosuke Niwa.
+
+        Should show 'Build completed' or 'Build failed' for build type build requests.
+
+        * browser-tests/test-group-result-page-tests.js: Added a unit test to guard this bug.
+        * tools/js/test-group-result-page.js: Show 'Build completed' or 'Build failed' for build type build requests.
+
 2018-12-21  Dewei Zhu  <dewei_zhu@apple.com>
 
         Add UI in analysis task page to show commit testability information.
index eea1e4b..085e1b6 100644 (file)
@@ -6,7 +6,7 @@ describe('TestGroupResultPage', () => {
             'models/data-model.js', 'models/metric.js', '../shared/statistics.js',], 'TestGroupResultPage', 'Metric');
     }
 
-    async function prepareTestGroupResultPage(context, resultA, resultB)
+    async function prepareTestGroupResultPage(context, buildRequestsA, buildRequestsB)
     {
         const [TestGroupResultPage, Metric] = await importMarkupComponent(context);
 
@@ -20,15 +20,15 @@ describe('TestGroupResultPage', () => {
 
         const mockTestGroup = {
             requestedCommitSets: () => ['A', 'B'],
-            test: () => ({test: () => 'speeodmeter-2', name: () => 'speedometer-2'}),
+            test: () => ({test: () => 'speedometer-2', name: () => 'speedometer-2'}),
             labelForCommitSet: (commitSet) => commitSet,
-            requestsForCommitSet: (commitSet) => ({'A': resultA, 'B': resultB}[commitSet]),
+            requestsForCommitSet: (commitSet) => ({'A': buildRequestsA, 'B': buildRequestsB}[commitSet]),
             compareTestResults: (...args) => ({isStatisticallySignificant: true, changeType: 'worse'}),
             name: () => 'mock-test-group',
         };
 
         const mockAnalysisResults = {
-            viewForMetric: (metric) => ({resultForRequest: (buildRequest) => (buildRequest === null ? null : {value: buildRequest})})
+            viewForMetric: (metric) => ({resultForRequest: (buildRequest) => (buildRequest.value === null ? null : {value: buildRequest.value})})
         };
 
         const page = new TestGroupResultPage('test');
@@ -40,9 +40,19 @@ describe('TestGroupResultPage', () => {
         return page;
     }
 
+    function prepareBuildRequests(buildRequestValues, isTestBooleanList, hasCompletedBooleanList)
+    {
+        return Array.from(buildRequestValues.entries()).map((entry) => {
+            const [index, value] = entry;
+            const isTest = isTestBooleanList ? isTestBooleanList[index] : true;
+            const hasCompleted = hasCompletedBooleanList ? hasCompletedBooleanList[index] : true;
+            return {value, isTest: () => isTest, isBuild: () => !isTest, hasCompleted: () => hasCompleted};
+        });
+    }
+
     it('should render failed test group with empty bar', async () => {
         const context = new BrowsingContext();
-        const page = await prepareTestGroupResultPage(context, [null, 3, 5], [2, 4, 6]);
+        const page = await prepareTestGroupResultPage(context, prepareBuildRequests([null, 3, 5]), prepareBuildRequests([2, 4, 6]));
         await page.enqueueToRender();
         const document = context.document;
         document.open();
@@ -56,7 +66,7 @@ describe('TestGroupResultPage', () => {
         const context = new BrowsingContext();
         const resultA = [1, 3, 5];
         const resultB = [2, 4, 6];
-        const page = await prepareTestGroupResultPage(context, resultA, resultB);
+        const page = await prepareTestGroupResultPage(context, prepareBuildRequests(resultA), prepareBuildRequests(resultB));
         page.enqueueToRender();
         const document = context.document;
         document.open();
@@ -77,4 +87,27 @@ describe('TestGroupResultPage', () => {
             previousNodeWidth = currentNodeWidth;
         }
     });
+
+    it('should not render rows for build requests those are not test', async () => {
+        const context = new BrowsingContext();
+        const resultA = [null, 1, 3, 5];
+        const isTestBooleanListA = [false, true, true, true];
+        const isBuildBooleanListA = [true, true, true, true];
+        const resultB = [null, 2, 4, 6];
+        const isTestBooleanListB = [false, true, true, true];
+        const isBuildBooleanListB = [false, true, true, true];
+        const page = await prepareTestGroupResultPage(context,
+            prepareBuildRequests(resultA, isTestBooleanListA, isBuildBooleanListA),
+            prepareBuildRequests(resultB, isTestBooleanListB, isBuildBooleanListB));
+        page.enqueueToRender();
+        const document = context.document;
+        document.open();
+        document.write(page.generateMarkup());
+        document.close();
+
+        const barNodes = context.document.querySelectorAll('.result-cell');
+        expect(barNodes.length).to.be(8);
+        expect(barNodes[0].textContent).to.be('Build completed');
+        expect(barNodes[4].textContent).to.be('Build failed');
+    });
 });
\ No newline at end of file
index 9af21f1..eb370e5 100644 (file)
@@ -31,10 +31,14 @@ class TestGroupResultPage extends MarkupPage {
         for (const commitSet of testGroup.requestedCommitSets())
         {
             const buildRequestsForCommitSet = testGroup.requestsForCommitSet(commitSet);
-            const results = buildRequestsForCommitSet.map((buildRequest) => analysisResultsView.resultForRequest(buildRequest));
+            const buildTypeRequests = buildRequestsForCommitSet.filter((buildRequest) => buildRequest.isBuild());
+            const testTypeRequests = buildRequestsForCommitSet.filter((buildRequest) => buildRequest.isTest());
+
+            const buildTypeResults = buildTypeRequests.map((buildRequest) => ({isBuild: true, hasCompleted: buildRequest.hasCompleted()}));
+            const results = [...buildTypeResults, ...testTypeRequests.map((buildRequest) => analysisResultsView.resultForRequest(buildRequest))];
             resultsByCommitSet.set(commitSet, results);
             for (const result of results) {
-                if (!result)
+                if (!result || result.isBuild)
                     continue;
                 maxValue = Math.max(maxValue, result.value);
                 minValue = Math.min(minValue, result.value);
@@ -77,8 +81,8 @@ class TestGroupResultPage extends MarkupPage {
 
         const tableBodies = [];
 
-        const beforeResults = resultsByCommitSet.get(requestedCommitSets[0]).filter((result) => !!result);
-        const afterResults = resultsByCommitSet.get(requestedCommitSets[1]).filter((result) => !!result);
+        const beforeResults = resultsByCommitSet.get(requestedCommitSets[0]).filter((result) => !!result && !result.isBuild);
+        const afterResults = resultsByCommitSet.get(requestedCommitSets[1]).filter((result) => !!result && !result.isBuild);
         const comparison = testGroup.compareTestResults(metric, beforeResults, afterResults);
         const changeStyleClassForMean = `${comparison.isStatisticallySignificantForMean ? comparison.changeType : 'insignificant'}-result`;
         const changeStyleClassForIndividual = `${comparison.isStatisticallySignificantForIndividual ? comparison.changeType : 'insignificant'}-result`;
@@ -100,8 +104,16 @@ class TestGroupResultPage extends MarkupPage {
             const label = testGroup.labelForCommitSet(commitSet);
 
             for (const result of results) {
-                const cellValue = result ? formatValue(result.value, result.interval).join('') : 'Failed';
-                const barWidth = result ? widthForValue(result.value) : 0;
+                let cellValue = null;
+                let barWidth = 0;
+                if (!result)
+                    cellValue = 'Failed';
+                else if (result.isBuild)
+                    cellValue = 'Build ' + (result.hasCompleted ? 'completed' : 'failed');
+                else {
+                    cellValue = formatValue(result.value, result.interval).join('');
+                    barWidth = widthForValue(result.value);
+                }
                 tableRows.push(this._constructTableRow(cellValue, barWidth, firstRow, results.length, label, averageColumnContents));
                 firstRow = false;
             }