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

[bugzilla-daemon at webkit.org]

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #342101|review?                     |review+
              Flags|                            |

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

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

> Websites/perf.webkit.org/public/api/test-groups.php:30
> +                    AND testgroup_needs_notification IS TRUE and testgroup_hidden IS FALSE");

Nit: and -> AND.

> Websites/perf.webkit.org/public/v3/models/test-group.js:43
> +    notificationSentAt() { return this._notificationSentAt; }

This should probably return new Date if _notificationSentAt is set.

> Websites/perf.webkit.org/public/v3/models/test-group.js:61
> +            await AnalysisTask.fetchById(this._taskId);

We should just return here.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:123
> +        createWithTestGroupCheckbox.onchange = () => repetitionCount.disabled = notifyOnCompletion.disabled = !createWithTestGroupCheckbox.checked;

Nit: Each assignment statement needs to be on its own line per our style guideline.
https://webkit.org/code-style-guidelines/#linebreaking-multiple-statements

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1253
> +    it('should be able to create a test group with needs-notification flag not set', async () => {

Nit: flag unset*

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:18
> +        this._messageConstructionRules = messageConstructionRules;

Why don't we add a check here to make sure each rule has either tests or platforms filter set,
and make sure they're also arrays of strings.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:80
> +        if (rule.tests) {
> +            if (!rule.tests.includes(test))

A simpler way to write this might be:
if (rule.tests && !rule.tests.includes(test))
    return false;
if (rule.platforms && !rule.platforms.includes(platform))
    return false;
return true;

and then do a check to make sure each rule has either tests or platforms set
as an early error check in the constructor.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:21
> +    static _URLForAnalysisTask(analysisTask)

Nit: I think we usually use lowercase url.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:26
> +    _buildCommitSetToResultsAMapAndBarWidthCalculator(testGroup, analysisResultsView)

I think it's better to simply call this _buildCommitSetToResultsMap.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:47
> +        return [resultsByCommitSet, (value) => (value - minValue) / (maxValue - minValue) * 100];

Let's return a dictionary instead; e.g. {resultsByCommitSet, barWidthFromValue: (value) => ~}

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:60
> +            const formatter = metric.makeFormatter(4);

Can we extract the content of this loop as a helper function?

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:80
> +                for (const result of results) {

Can we extract this as a helper function as well?

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:193
> +    _constructBarGraph(width)

We should put this below render() function so that it reads naturally top-down.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:195
> +        const barGraphPlaceholder = this.createElement('div',{class: 'bar-graph-placeholder'});

It's not necessary to create div each time. Just put it into the template and just set width instead.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:207
> +    static get pageContent()

This is not right. Use contentTemplate.

-- 
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/20180607/3e670f0f/attachment-0001.html>