perf.webkit.org/tools/js/analysis-results-notifier.js should allow sending of completion emails regardless of test name
rdar://74202006
Created attachment 419900 [details] Patch
Created attachment 419952 [details] Patch
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
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.
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.
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.
rreapor@apple.com 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.
Committed r272813: <https://commits.webkit.org/r272813> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420093 [details].