[Webkit-unassigned] [Bug 184340] Perf dashboard should send a notification when a test group finishes

Sat Jun 2 00:40:11 PDT 2018

https://bugs.webkit.org/show_bug.cgi?id=184340

--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 341829
  --> https://bugs.webkit.org/attachment.cgi?id=341829
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341829&action=review

> Websites/perf.webkit.org/public/api/test-groups.php:-16
> -    if (count($path) > 0 && $path[0]) {

I think you need to add an error case for when count($path) > 1 first
and then use something like $path_name = array_get($path, 0);

> Websites/perf.webkit.org/public/api/test-groups.php:16
> +    if (count($path) > 0 && $path[0] == 'notifications-pending') {

This should be count($path) == 1.

> Websites/perf.webkit.org/public/api/test-groups.php:31
> +    }elseif (count($path) > 0 && $path[0]) {

Nit: Need a space between } and else if.

> Websites/perf.webkit.org/public/v3/models/test-group.js:197
> +            needsNotification: false

Let's add a new database field to record when the notification was sent instead of just clearing it.

> Websites/perf.webkit.org/public/v3/models/test-group.js:203
> +    static createWithTask(taskName, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion)

We need a test for when notifyOnCompletion to set to false as well.

> Websites/perf.webkit.org/public/v3/models/test-group.js:250
> +        return this.cachedFetch('/api/test-groups/notifications-pending', null, true).then(this._createModelsFromFetchedTestGroups.bind(this));

Why don't we call the API /api/test-groups/notifications-available or /api/test-groups/available-for-notification
/api/test-groups/ready-for-notification or /api/test-groups/notification-ready

> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:68
> -        return TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2]);
> +        return TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2], true);

If you're modifying these tests, be sure to heck that the test group has this bit set later in the case.
Otherwise don't make the changes.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1234
> +            return Promise.all([AnalysisTask.fetchById(result['taskId']), TestGroup.fetchForTask(result['taskId'], true)]);

You should probably rename this test to say "with needs-notification flag set"

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:22
> +        const tempDir = fs.mkdtempSync(os.tmpdir());
> +        const tempFilePath = path.join(tempDir, 'temp-content.json');

Don't create a temporary directory or a file until everything else is ready.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:25
> +            const title = `Test group: "${testGroup.name()}" from Analysis task "${testGroup.task().name()}" has finished all build requests`;

This is a very verbose name. Why don't we just say: "${testGroup.task().name()}" - "${testGroup.name()}" has finished.
Or: "${testGroup.name()}" in "${testGroup.task().name()}" has completed.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:28
> +            content = this._constructMessageByRules(testGroup.platform().name(), testGroup.test().path()[0].name(), content);

I think we should call this applyRules or something.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:34
> +            await this._notifcationService.sendNotification(content);
> +            await testGroup.didSendNotification();

We should probably log before & after this critical section so that in the case the script got killed
after sending the notification but before clearing the flag, we would at least see it in the log.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:43
> +            if (matchingFunction(platformName, testName))

I think we should just do what _buildMessageConstructionRules is doing it here.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:72
> +    static _applyUpdate(message, update) {

{ should be on the next line.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:140
> +        return this._notificationServerRemoteAPI.postJSON(this._notificationServicePath, message);

I don't think we need this class. Just do it directly in AnalysisResultsNotifier.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:167
> +

Nit: extra line.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:172
> +            totalRows = 0;

This is very misleading. First off, this variable should be renamed to something like total runs.
Second off, it's very misleading for this counter to be reset each time.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:225
> +                barGraphPlaceholder.style.width = 20 + (value - minValue) / (maxValue - minValue) * 280 + 'px';

Use % instead.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:263
> +                'font': 'Serif',

Use sans-serif.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:266
> +                'font-size': '14pt',

Don't use pt. Use rem.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:270
> +                'font-size': '14pt',

Ditto.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:274
> +                'font-size': '14pt',

Use rem.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:275
> +                'margin-bottom': '2px',

Ditto.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:276
> +                'min-width': '600px',

Use rem instead.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:285
> +                'margin-top': '20px',

Ditto.

> Websites/perf.webkit.org/unit-tests/test-groups-tests.js:168
> +            const updatedTestGroup = sampleTestGroup();
> +            updatedTestGroup.testGroups[0].needsNotification = false;

We should instead of make sampleTestGroup take an argument like "options" which specifies this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180602/618a5c1a/attachment-0001.html>