Extend create-analysis-test api to be able to create with confirming test group.
Created attachment 338713 [details] Patch
Comment on attachment 338713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338713&action=review > Websites/perf.webkit.org/public/include/json-header.php:201 > +function insert_commit_sets_and_construct_configuration_list($db, $commit_sets) How do you like the idea that creating a new file to contain all helper functions those are not related to 'json-header.php'? For example, it seems like 'find_triggerable_for_task' (line169) should not be in 'json-header.php'.
Comment on attachment 338713 [details] Patch Attachment 338713 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7461112 New failing tests: http/tests/preload/onload_event.html
Created attachment 338831 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 338713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338713&action=review > Websites/perf.webkit.org/ChangeLog:10 > + Refactored 'create-test-group' API to share some creating test group logic with 'create-analysis-task' API. Nit: We usually refer to an API by its URL: /privileged-api/create-test-group and /privileged-api/create-analysis-task > Websites/perf.webkit.org/ChangeLog:11 > + Moved the shared logic to 'json-header.php'. Nit: we don't normally quote filenames. Just say json-header.php. > Websites/perf.webkit.org/ChangeLog:16 > + Added new helper functions 'create_build_requests_with_configurations_and_order' and Please use (~) syntax to just functions. Just because prepare-ChangeLogs doesn't generate it, it doesn't mean we don't need them. > Websites/perf.webkit.org/ChangeLog:18 > + 'insert_commit_sets_and_construct_configuration_list' which will be shared by both 'create-analysis-task.php' and > + 'create-test-group.php'. Ditto about not quoting filenames. >> Websites/perf.webkit.org/public/include/json-header.php:201 >> +function insert_commit_sets_and_construct_configuration_list($db, $commit_sets) > > How do you like the idea that creating a new file to contain all helper functions those are not related to 'json-header.php'? For example, it seems like 'find_triggerable_for_task' (line169) should not be in 'json-header.php'. Yes, we should create a new file for this. Maybe commit-sets-helpers.php since all these help functions are commit sets. > Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:11 > + $confirming_iteration = array_get($data, 'confirmingIteration'); This should be repetitionCount to match privileged-api/create-test-group > Websites/perf.webkit.org/public/privileged-api/create-test-group.php:96 > + $order = - count($build_configuration_list); > + create_build_requests_with_configurations_and_order($db, $build_configuration_list, $order, $triggerable_id, $platform_id, NULL, $group_id); > + assert($order == 0); > + for ($i = 0; $i < $repetition_count; $i++) > + create_build_requests_with_configurations_and_order($db, $test_configuration_list, $order, $triggerable_id, $platform_id, $test_id, $group_id); Probably makes sense to extract this entire section as a function. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:306 > - static create(name, startRunId, endRunId) > + static create(name, startPoint, endPoint, testGroupName, confirmingIteration) If one argument is called testGroupName then the other one shouldn't be called confirmingIteration. Just call it iterationCount to match the naming convention elsewhere. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:309 > + if (confirmingIteration > 0) { Nit: should either check if (testGroupName) or if (iterationCount) instead. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:311 > + parameters['confirmingIteration'] = confirmingIteration; This should be called repetitionCount to match /privileged-api/create-test-group. > Websites/perf.webkit.org/public/v3/models/test-group.js:222 > - static _revisionSetsFromCommitSets(commitSets) > + static revisionSetsFromCommitSets(commitSets) Maybe this should be a method on CommitSet itself? > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:567 > + <label>Confirm with test group of iteration <input type="number" value=4 min=1></label> This is a long description. How about just "Confirm with X iterations." We should be using select & option elements here. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:346 > + let test1 = Test.findByPath(['Suite', 'test1']); > + let platform = Platform.findByName('some platform'); Make these const? Also, call it somePlatform to match the name? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:354 > + const oneRevisionSet = {}; Do: oneRevisionSet = {[webkitId]: {revision: '191622'}}; > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:372 > + it('should create an analysis task with no test group when confirming repetition is 0', async () => { Update the test description since we've updated the API to use repetitionCount as well. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:380 > + let test1 = Test.findByPath(['Suite', 'test1']); > + let platform = Platform.findByName('some platform'); Use const here too. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:415 > + it('should create an analysis task with test group when commit set list and positive repetition count are specified', async () => { *a* positive repetition count > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:423 > + let test1 = Test.findByPath(['Suite', 'test1']); > + let platform = Platform.findByName('some platform'); Why don't we add these things directly into the database with a known ID? > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:435 > + db.insert('build_triggerables', {id: 1234, name: 'test-triggerable'}), > + db.insert('triggerable_repository_groups', {id: 2345, name: 'webkit-only', triggerable: 1234}), > + db.insert('triggerable_repositories', {repository: webkitId, group: 2345}), > + db.insert('triggerable_configurations', {test: test1.id(), platform: platform.id(), triggerable: 1234}) Along with these. Then you wouldn't need to fetch Manifest twice. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:448 > + TestServer.cleanDataDirectory(); > + await Manifest.fetch(); I'm a worried that clearing all the states like this here may cause us to not catch bugs like the infamous "task is undefined in task.id()" bug we had. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:494 > +describe('/privileged-api/create-analysis-task with node privileged api', function () { I don't think we need to duplicate all the test cases like this since there isn't any difference in terms of how the server would respond. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:497 > + PrivilegedAPI.configure('test', 'password'); We need to test that when node node privileged api is used without slave name/password, the attempt to create an analysis task with/without a test group would fail.
Comment on attachment 338713 [details] Patch r- because we probably want to go through another iteration.
Created attachment 338972 [details] Patch
Comment on attachment 338972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338972&action=review > Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:77 > + if (!$triggerable || !$triggerable['id']) > + exit_with_error('TriggerableNotFoundForTask', array('task' => $task_id, 'platform' => $config['config_platform'])); Call $db->rollback_transaction() here before exiting. > Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:79 > + if ($triggerable['platform'] != $config['config_platform']) > + exit_with_error('InconsistentPlatform', array('configPlatform' => $config['config_platform'], 'taskPlatform' => $triggerable['platform'])); Ditto. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:310 > + if (repetitionCount) { > + console.assert(testGroupName); Reverse this. When testGroupName is set, repetitionCount should also be set. Also give these two arguments default values of null / 0. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:120 > + const createWithConfirmingTestGroupCheckbox = this.content('create-with-confirming-test-group'); Call this just create-with-test-group. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:238 > + const name = analyzePopover.querySelector('input[type=text]').value; Please give it an ID and use this.part(~) to find the element instead. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:239 > + const createWithConfirmingTestGroup = analyzePopover.querySelector('input[type=checkbox]').checked; Ditto. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:241 > + AnalysisTask.create(name, startPoint, endPoint, 'Confirm', createWithConfirmingTestGroup ? repetitionCount : 0).then((data) => { We shouldn't pass the test group name when we're not creating one. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:735 > + .chart-pane-actions .popover input[type=number] { In general, we prefer specifying rules using ID than nested descendent selectors like this. > Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:497 > + it('should return "SlaveNotFound" when incorrect slave user and password combination is provided and no analysis task, test group or build request should be created', async () => { Why don't we add another one which DOES specify slave name & password, and make sure that one works.
rdar://problem/39783538