Bug 184340

Created attachment 337299 [details] Patch

Comment on attachment 337299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337299&action=review I really don't think we should be making these database schema changes without a corresponding frontend or a backend change. > Websites/perf.webkit.org/public/v3/models/test-group.js:191 > + async updateNotifiedAuthorFlag(notified) Why do we need this function in frontend? Also, this should really be called didNotifyAuthor() and shouldn't take a boolean because it never makes sense to unset the flag. > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:131 > + webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0]; Why aren't we using const webkit here instead? > Websites/perf.webkit.org/server-tests/privileged-api-update-test-group-tests.js:135 > + insertedGroupId = result['testGroupId']; Ditto about doing const testGroupId here instead.

Created attachment 339006 [details] Patch

Comment on attachment 339006 [details] Patch Again, I don't think we should be making these kind of database schema changes independently of the rest of the changes.

Created attachment 341254 [details] Patch

Comment on attachment 341254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341254&action=review > Websites/perf.webkit.org/init-database.sql:282 > + testgroup_has_pending_notification boolean NOT NULL DEFAULT FALSE, As we discussed in person, I think we should call this needs_notification instead since has_pending_notification sounds as though we've already got a notification to send when in reality this boolean indicates that we must send a notification WHEN all build requests complete. > Websites/perf.webkit.org/public/api/test-groups.php:16 > + if (count($path) > 0 && $path[0] == 'all-with-pending-notification') { I think ?notifications-pending=true will do. > Websites/perf.webkit.org/public/api/test-groups.php:24 > + $test_group_id_list = array(); > + foreach($test_groups as $group) > + array_push($test_group_id_list, $group['testgroup_id']); Why don't we move this code into fetch_requests_for_groups so that we can call int() on each test group ID to make sure they're not random strings. > Websites/perf.webkit.org/public/api/test-groups.php:26 > + if (!count($test_group_id_list)) You need { ~ } here. > Websites/perf.webkit.org/tools/js/email-notifier.js:4 > +class EmailNotifier { Since this class doesn't directly send an email but rather notifies an external service, we shouldn't call this EmailNotifier. How about AnalysisResultsNotifier?

Created attachment 341502 [details] Patch

Comment on attachment 341502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341502&action=review > Websites/perf.webkit.org/ChangeLog:3 > + Added sending notification feature when test group finishes. I updated the bugzilla title to be more descriptive. Please update the title in the change log accordingly.

Created attachment 341829 [details] Patch

Comment on attachment 341829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341829&action=review > Websites/perf.webkit.org/public/api/test-groups.php:-16 > - if (count($path) > 0 && $path[0]) { I think you need to add an error case for when count($path) > 1 first and then use something like $path_name = array_get($path, 0); > Websites/perf.webkit.org/public/api/test-groups.php:16 > + if (count($path) > 0 && $path[0] == 'notifications-pending') { This should be count($path) == 1. > Websites/perf.webkit.org/public/api/test-groups.php:31 > + }elseif (count($path) > 0 && $path[0]) { Nit: Need a space between } and else if. > Websites/perf.webkit.org/public/v3/models/test-group.js:197 > + needsNotification: false Let's add a new database field to record when the notification was sent instead of just clearing it. > Websites/perf.webkit.org/public/v3/models/test-group.js:203 > + static createWithTask(taskName, platform, test, groupName, repetitionCount, commitSets, notifyOnCompletion) We need a test for when notifyOnCompletion to set to false as well. > Websites/perf.webkit.org/public/v3/models/test-group.js:250 > + return this.cachedFetch('/api/test-groups/notifications-pending', null, true).then(this._createModelsFromFetchedTestGroups.bind(this)); Why don't we call the API /api/test-groups/notifications-available or /api/test-groups/available-for-notification /api/test-groups/ready-for-notification or /api/test-groups/notification-ready > Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:68 > - return TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2]); > + return TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2], true); If you're modifying these tests, be sure to heck that the test group has this bit set later in the case. Otherwise don't make the changes. > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1234 > + return Promise.all([AnalysisTask.fetchById(result['taskId']), TestGroup.fetchForTask(result['taskId'], true)]); You should probably rename this test to say "with needs-notification flag set" > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:22 > + const tempDir = fs.mkdtempSync(os.tmpdir()); > + const tempFilePath = path.join(tempDir, 'temp-content.json'); Don't create a temporary directory or a file until everything else is ready. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:25 > + const title = `Test group: "${testGroup.name()}" from Analysis task "${testGroup.task().name()}" has finished all build requests`; This is a very verbose name. Why don't we just say: "${testGroup.task().name()}" - "${testGroup.name()}" has finished. Or: "${testGroup.name()}" in "${testGroup.task().name()}" has completed. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:28 > + content = this._constructMessageByRules(testGroup.platform().name(), testGroup.test().path()[0].name(), content); I think we should call this applyRules or something. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:34 > + await this._notifcationService.sendNotification(content); > + await testGroup.didSendNotification(); We should probably log before & after this critical section so that in the case the script got killed after sending the notification but before clearing the flag, we would at least see it in the log. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:43 > + if (matchingFunction(platformName, testName)) I think we should just do what _buildMessageConstructionRules is doing it here. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:72 > + static _applyUpdate(message, update) { { should be on the next line. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:140 > + return this._notificationServerRemoteAPI.postJSON(this._notificationServicePath, message); I don't think we need this class. Just do it directly in AnalysisResultsNotifier. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:167 > + Nit: extra line. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:172 > + totalRows = 0; This is very misleading. First off, this variable should be renamed to something like total runs. Second off, it's very misleading for this counter to be reset each time. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:225 > + barGraphPlaceholder.style.width = 20 + (value - minValue) / (maxValue - minValue) * 280 + 'px'; Use % instead. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:263 > + 'font': 'Serif', Use sans-serif. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:266 > + 'font-size': '14pt', Don't use pt. Use rem. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:270 > + 'font-size': '14pt', Ditto. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:274 > + 'font-size': '14pt', Use rem. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:275 > + 'margin-bottom': '2px', Ditto. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:276 > + 'min-width': '600px', Use rem instead. > Websites/perf.webkit.org/tools/js/analysis-results-notifier.js:285 > + 'margin-top': '20px', Ditto. > Websites/perf.webkit.org/unit-tests/test-groups-tests.js:168 > + const updatedTestGroup = sampleTestGroup(); > + updatedTestGroup.testGroups[0].needsNotification = false; We should instead of make sampleTestGroup take an argument like "options" which specifies this.

Created attachment 342008 [details] Patch

Comment on attachment 342008 [details] 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.

Created attachment 342101 [details] Patch

Comment on attachment 342101 [details] 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.