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

Tue Jun 5 23:10:50 PDT 2018

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

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

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

> Websites/perf.webkit.org/public/api/test-groups.php:27
> +                WHERE EXISTS(SELECT 1 FROM build_requests

Nit: Wrong indentation. WHERE should appear exactly 4 spaces to the right of $test_groups.

> Websites/perf.webkit.org/public/api/test-groups.php:29
> +                  AND request_status IN (\'pending\', \'scheduled\', \'running\', \'canceled\')) IS FALSE

Can we use " for the string so that we don't have to escape it manually here?

> Websites/perf.webkit.org/public/privileged-api/update-test-group.php:25
> +        if (!($has_notification_sent_at_field xor $data['needsNotification']))

I think a simpler way to express this would be: !!$data['needsNotification'] == !!$has_notification_sent_at_field

> Websites/perf.webkit.org/server-tests/api-test-groups.js:63
> +        it('should not list test groups that have author notified before', async () => {

Nit: should not list test groups without needs notification flag.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:606
> +    it('should create an analysis task with test group and respect the "needsNotification" flag in the http request', async () => {

Don't change the test name. Keep the original test in tact, add a new test which sets needsNotification to true.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:638
> +            startRun: testRuns[0]['id'], endRun: testRuns[1]['id'], needsNotification: false});

Omit needsNotification here to keep the original test case.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:342
> -            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, commitSets}).then((content) => {
> +            return PrivilegedAPI.sendRequest('create-test-group', {name: 'test', task: taskId, commitSets, needsNotification: true}).then((content) => {

I don't think we should be modifying all these tests. Keep them intact.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1224
> +    it('should create a test group with an analysis task with needs-notification flag set', () => {

Use async & await?

> Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:132
> +    it('should throw "SlaveNotFound" if invalid slave name and password combination is provided', async () => {

Nit: API isn't throwing anything. It's simply returning an error code so revise the description as such.
Ditto for the rest of tests.

> Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:137
> +        await assertThrows('SlaveNotFound', () =>
> +            PrivilegedAPI.sendRequest('update-test-group', {}));

This can fit in a single line.

> Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:144
> +        await assertThrows('TestGroupNotSpecified', () =>
> +            PrivilegedAPI.sendRequest('update-test-group', {}));

Ditto.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:41
> +        const analysisTask = testGroup.task() || await AnalysisTask.fetchById(testGroup._taskId);
> +        const analysisResults = await AnalysisResults.fetch(analysisTask.id());

We should add a method on TestGroupResultPage to set test group which fetches these things
since AnalysisResultsNotifier itself doesn't need them.
Also, we should probably add TestGroup.fetchTask with this fallback logic.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:43
> +        return (new TestGroupResultPage('test', testGroup, analysisResults, analysisTask, AnalysisResultsNotifier._URLForAnalysisTask(analysisTask))).generateMarkup();

Use the email title for the title.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:46
> +    static _URLForAnalysisTask(analysisTask)

This should just be a method on TestGroupResultPage.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:76
> +    static _matchesRule(platform, test, platforms, tests)

I think function should just take rule object to match its name for clarity.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:108
> +            if (!message.hasOwnProperty(key))

We can just do: "!(key in message)"

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:110
> +            else if (Array.isArray(value) && Array.isArray(valueToMerge))

I think a simpler thing to do here is:
if (Array.isArray(value) || Array.isArray(valueToMerge)) {
    if (!Array.isArray(value))
        value = [value];
    if (!Array.isArray(valueToMerge))
        valueToMerge = [valueToMerge];
    mergedValue = [...value, ...valueToMerge];
}

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:21
> +        const element = ComponentBase.createElement.bind(ComponentBase);
> +        const link = ComponentBase.createLink.bind(ComponentBase);

Just directly call this.createElement & this.createLink.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:54
> +            }

We should extract this function which computes min & max, and it should build a map from commitSet to results.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:64
> +                const values = results.filter((result) => !!result).map((result) => result.value);
> +                arrayOfValues.push(values);

This arrayOfValues & values variables are confusing.
I think it's better if kept the list of all results: e.g. resultsList.

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

Nit: Need a space between for and (.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:69
> +                        barGraphPlaceholder.style.width = (10 + (result.value - minValue) / (maxValue - minValue) * 90) + '%';

I don't think this math makes sense. This will just always add 10% margin on the left.
Instead, expand min & max by 10%.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:72
> +                    const barGraphContainer = element('div', {class: 'bar-graph-container'}, barGraphPlaceholder);
> +                    const cellContent = [barGraphContainer, result ? formatValue(result.value, result.interval).join('') : 'Failed'];

We should just extra this as a very simple BarGraph component.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:79
> +                                [testGroup.labelForCommitSet(commitSet) + ': ',
> +                                    formatValue(Statistics.mean(values), Statistics.confidenceInterval(values)).map((content => element('span', {class: 'no-wrap'}, content)))]),

Can we define a local variable before generating the label?

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:83
> +                    }
> +                    else

Nit: } and else need to be in the same line.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:90
> +            const changeStyleClass =  `${comparison.isStatisticallySignificant ? comparison.changeType : 'insignificant'}-result`;

Nit: Two spaces between = and `.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:94
> +            tablesWithSummary.push([element('table', {class: 'result-table '}, [caption, tableBodies])]);

Nit: An extra space after result-table.

> Websites/perf.webkit.org/tools/js/test-group-result-page.js:102
> +
> +

Nit: Two blank lines here.

-- 
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/20180606/831b7987/attachment.html>