[webkit-reviews] review granted: [Bug 184340] Perf dashboard should send a notification when a test group finishes : [Attachment 342101] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 184340: Perf dashboard should send a notification when a test group
finishes
https://bugs.webkit.org/show_bug.cgi?id=184340

Attachment 342101: Patch

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




--- 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.