Bug 221712 – perf.webkit.org/tools/js/analysis-results-notifier.js should allow sending of completion emails regardless of test name

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Roy Reapor]

perf.webkit.org/tools/js/analysis-results-notifier.js should allow sending of completion emails regardless of test name

[Roy Reapor]

rdar://74202006

[Roy Reapor]

Created attachment 419900 [details]
Patch

[Roy Reapor]

Created attachment 419952 [details]
Patch

[Roy Reapor]

Created attachment 419954 [details]
Patch

Comment on attachment 419954 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:10
> +            - `platforms` and `tests` can now be undefined or an empty array

Nit: I don't think we indent here, also the indentation doesn't seem to be consistent within your commit message.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:31
> +        function isUnderfinedOrEmptyArrayOrArrayOfStrings(object) {
>              if (!object)
> -                return false;
> +                return true;
>              if (!Array.isArray(object))
>                  return false;
>              return object.every((entry) => entry instanceof String || typeof(entry) === 'string');
>          }

Discussed in person with Roy, it turns out `isNonemptyArrayOfStrings` has a bug that will return true if an empty array is passed in, which doesn't quite match the name of the function.

Since we are making change to extend the syntax of config to allow `undefined`, we also allow an empty array `[]` as a legitimate option. `undefined` means match everything, `[]` means match nothing.

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

For empty array, we should return false, so the length check is not needed here.

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:95
> +        if (rule.platforms && rule.platforms.length && !rule.platforms.includes(platform))

Ditto

[Roy Reapor]

Comment on attachment 419954 [details]
Patch

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

> Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:93
>              return false;

I like  "`undefined` means match everything, `[]` means match nothing." But `_matchesRule()` will always return false if `tests` or `platforms` is empty. does that seem correct?

All the rules below will match nothing:
{platforms: [], tests: [speedometer2, jetStream]}
{platforms: [], tests: [speedometer2, jetStream], userInitiated: true};
{tests: [], platforms: [trunkMacBook, trunkMacBookAir], userInitiated: false};

how about not allowing empty?

Seems right to me.

Basically, if user specifies 'tests' in the rule, we should respect that even though that will filter out everything.

If user doesn't specify anything, then it's OK for us to use the default behavior which include all tests.

I think strategy for sending notification to user should be conservative in general.

[Roy Reapor]

Created attachment 420093 [details]
Patch

Comment on attachment 420093 [details]
Patch

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

> Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js:86
> +        it('should match rule when platforms, tests, and userInitiated are undefined', () => {

Let's add two more test cases where either platform or tests is specified.

r=me with comment.

[Roy Reapor]

Comment on attachment 420093 [details]
Patch

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

>> Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js:86
>> +        it('should match rule when platforms, tests, and userInitiated are undefined', () => {
> 
> Let's add two more test cases where either platform or tests is specified.

You mean line 33 http://trac.webkit.org/browser/webkit/trunk/Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js#L33 and 44 http://trac.webkit.org/browser/webkit/trunk/Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js#L44 ?

Comment on attachment 420093 [details]
Patch

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

>>> Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js:86
>>> +        it('should match rule when platforms, tests, and userInitiated are undefined', () => {
>> 
>> Let's add two more test cases where either platform or tests is specified.
> 
> You mean line 33 http://trac.webkit.org/browser/webkit/trunk/Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js#L33 and 44 http://trac.webkit.org/browser/webkit/trunk/Websites/perf.webkit.org/unit-tests/analysis-results-notifier-tests.js#L44 ?

You are right, I didn't check that.

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 420093 [details] from commit queue.

[EWS]

Committed r272813: <https://commits.webkit.org/r272813>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420093 [details].