Attachment 359598 Details for Bug 193563 – Patch

[patch] Patch

bug-193563-20190119005825.patch (text/plain), 8.38 KB, created by dewei_zhu on 2019-01-19 00:58:26 PST

(hide)

Description:

Filename:

MIME Type:

Creator: dewei_zhu

Created: 2019-01-19 00:58:26 PST

Size: 8.38 KB

patch

obsolete

>Subversion Revision: 240136
>diff --git a/Websites/perf.webkit.org/ChangeLog b/Websites/perf.webkit.org/ChangeLog
>index 4156c10f0782da67f791486e6846b12d114022bc..058b842f7a518be1bdbf025c63619259dfc1b63c 100644
>--- a/Websites/perf.webkit.org/ChangeLog
>+++ b/Websites/perf.webkit.org/ChangeLog
>@@ -1,3 +1,21 @@
>+2019-01-17  Dewei Zhu  <dewei_zhu@apple.com>
>+
>+        Analyzing a chart that does not exist should not halt whole run-analysis script.
>+        https://bugs.webkit.org/show_bug.cgi?id=193563
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Halting whole run-analysis script while there is any invalid chart specified in Manifest makes the script fragile.
>+        Run-analysis is also responsible for adding retry and sending notification which should not be block by this error.
>+        Skipping analyzing the corresponding configuration seems reasonable.
>+
>+        * public/v3/models/measurement-set.js:
>+        (MeasurementSet.prototype._ensureClusterPromise): Only add callback when callback is specified.
>+        This will help to fix 'UnhandledPromiseRejectionWarning' while running the test.
>+        * tools/js/measurement-set-analyzer.js:
>+        (MeasurementSetAnalyzer.prototype.async._analyzeMeasurementSet): Catch the exception while failing to fetch a measurement set and skip the analysis for this config.
>+        * unit-tests/measurement-set-analyzer-tests.js: Added unit tests for this.
>+
> 2018-12-21  Dewei Zhu  <dewei_zhu@apple.com>
> 
>         Add UI in analysis task page to show commit testability information.
>diff --git a/Websites/perf.webkit.org/public/v3/models/measurement-set.js b/Websites/perf.webkit.org/public/v3/models/measurement-set.js
>index 57a24ce786ca07409fbca837e966db39836a95fd..57437c3f400bfa0cc255fcdb1569db1ea6822306 100644
>--- a/Websites/perf.webkit.org/public/v3/models/measurement-set.js
>+++ b/Websites/perf.webkit.org/public/v3/models/measurement-set.js
>@@ -62,7 +62,8 @@ class MeasurementSet {
>         if (!this._primaryClusterPromise)
>             this._primaryClusterPromise = this._fetchPrimaryCluster(noCache);
>         var self = this;
>-        this._primaryClusterPromise.catch(callback);
>+        if (callback)
>+            this._primaryClusterPromise.catch(callback);
>         return this._primaryClusterPromise.then(function () {
>             self._allFetches[self._primaryClusterEndTime] = self._primaryClusterPromise;
>             return Promise.all(self.findClusters(startTime, endTime).map(function (clusterEndTime) {
>@@ -76,12 +77,13 @@ class MeasurementSet {
>         if (!this._callbackMap.has(clusterEndTime))
>             this._callbackMap.set(clusterEndTime, new Set);
>         var callbackSet = this._callbackMap.get(clusterEndTime);
>-        callbackSet.add(callback);
>+        if (callback)
>+            callbackSet.add(callback);
> 
>         var promise = this._allFetches[clusterEndTime];
>-        if (promise)
>+        if (promise && callback) {
>             promise.then(callback, callback);
>-        else {
>+        } else if (!promise) {
>             promise = this._fetchSecondaryCluster(clusterEndTime);
>             for (var existingCallback of callbackSet)
>                 promise.then(existingCallback, existingCallback);
>diff --git a/Websites/perf.webkit.org/tools/js/measurement-set-analyzer.js b/Websites/perf.webkit.org/tools/js/measurement-set-analyzer.js
>index ae3a2c4cbd00f14bd22f2ed3ff71695fd35b235d..ab86a1d4e33794be78cfbbe0f76599ea857bebaf 100644
>--- a/Websites/perf.webkit.org/tools/js/measurement-set-analyzer.js
>+++ b/Websites/perf.webkit.org/tools/js/measurement-set-analyzer.js
>@@ -49,7 +49,14 @@ class MeasurementSetAnalyzer {
>         const metric = Metric.findById(measurementSet.metricId());
>         const platform = Platform.findById(measurementSet.platformId());
>         this._logger.info(`==== "${metric.fullName()}" on "${platform.name()}" ====`);
>-        await measurementSet.fetchBetween(this._startTime, this._endTime);
>+        try {
>+            await measurementSet.fetchBetween(this._startTime, this._endTime);
>+        } catch (error) {
>+            if (error != 'ConfigurationNotFound')
>+                throw error;
>+            this._logger.warn(`Skipping analysis for "${metric.fullName()}" on "${platform.name()}" as time series does not exit.`);
>+            return;
>+        }
>         const currentTimeSeries = measurementSet.fetchedTimeSeries('current', false, false);
>         const rawValues = currentTimeSeries.values();
>         if (!rawValues || rawValues.length < 2)
>diff --git a/Websites/perf.webkit.org/unit-tests/measurement-set-analyzer-tests.js b/Websites/perf.webkit.org/unit-tests/measurement-set-analyzer-tests.js
>index 1f21491c559eea9128694be2134cb816bbba603e..7abeae2ba3be460a22ee7f75f1c838fae205002e 100644
>--- a/Websites/perf.webkit.org/unit-tests/measurement-set-analyzer-tests.js
>+++ b/Websites/perf.webkit.org/unit-tests/measurement-set-analyzer-tests.js
>@@ -35,10 +35,12 @@ describe('MeasurementSetAnalyzer', () => {
>     {
>         const info_logs = [];
>         const error_logs =[];
>+        const warn_logs =[];
>         return {
>             info: (message) => info_logs.push(message),
>+            warn: (message) => warn_logs.push(message),
>             error: (message) => error_logs.push(message),
>-            info_logs, error_logs
>+            info_logs, warn_logs, error_logs
>         };
>     }
> 
>@@ -89,6 +91,44 @@ describe('MeasurementSetAnalyzer', () => {
>             assert.deepEqual(logger.error_logs, []);
>         });
> 
>+        it('should not analyze if no corresponding time series for a measurement set', async () => {
>+            const measurementSet = MeasurementSet.findSet(MockModels.somePlatform.id(), MockModels.someMetric.id(), 5000);
>+            const logger = mockLogger();
>+            const measurementSetAnalyzer = new MeasurementSetAnalyzer([measurementSet], 4000, 5000, logger);
>+            const analysisPromise = measurementSetAnalyzer.analyzeOnce(measurementSet);
>+            assert.equal(requests.length, 1);
>+            assert.equal(requests[0].url, `/data/measurement-set-${MockModels.somePlatform.id()}-${MockModels.someMetric.id()}.json`);
>+            requests[0].reject('ConfigurationNotFound');
>+
>+            try {
>+                await analysisPromise;
>+            } catch (error) {
>+                assert(false, 'Should not throw any exception here');
>+            }
>+            assert.deepEqual(logger.info_logs, ['==== "Some test : Some metric" on "Some platform" ====']);
>+            assert.deepEqual(logger.warn_logs, [`Skipping analysis for "${MockModels.someMetric.fullName()}" on "${MockModels.somePlatform.name()}" as time series does not exit.`]);
>+            assert.deepEqual(logger.error_logs, []);
>+        });
>+
>+        it('should throw an error if "measurementSet.fetchBetween" is not failed due to "ConfugurationNotFound"', async () => {
>+            const measurementSet = MeasurementSet.findSet(MockModels.somePlatform.id(), MockModels.someMetric.id(), 5000);
>+            const logger = mockLogger();
>+            const measurementSetAnalyzer = new MeasurementSetAnalyzer([measurementSet], 4000, 5000, logger);
>+            const analysisPromise = measurementSetAnalyzer.analyzeOnce(measurementSet);
>+            assert.equal(requests.length, 1);
>+            assert.equal(requests[0].url, `/data/measurement-set-${MockModels.somePlatform.id()}-${MockModels.someMetric.id()}.json`);
>+            requests[0].reject('SomeError');
>+
>+            try {
>+                await analysisPromise;
>+            } catch (error) {
>+                assert.equal(error, 'SomeError');
>+            }
>+            assert.deepEqual(logger.info_logs, ['==== "Some test : Some metric" on "Some platform" ====']);
>+            assert.deepEqual(logger.warn_logs, []);
>+            assert.deepEqual(logger.error_logs, []);
>+        });
>+
>         it('should not analyze if there is only one data point in the measurement set', async () => {
>             const measurementSet = MeasurementSet.findSet(MockModels.somePlatform.id(), MockModels.someMetric.id(), 5000);
>             const logger = mockLogger();
>@@ -297,7 +337,6 @@ describe('MeasurementSetAnalyzer', () => {
>             assert.deepEqual(logger.error_logs, []);
>         });
> 
>-
>         it('should not create confirming A/B tests if a new regression is detected but no triggerable available', async () => {
>             PrivilegedAPI.configure('test', 'password');
>             const measurementSet = MeasurementSet.findSet(MockModels.somePlatform.id(), MockModels.someMetric.id(), 5000);

Flags:

rniwa: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193563: 359449 | 359598