Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits.
Created attachment 304241 [details] patch
Comment on attachment 304241 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304241&action=review I'd say r- for now given the list of issues I listed below. > Websites/perf.webkit.org/server-tests/resources/mock-child-process.js:3 > +MockChildProcess = { As we talked in person, it's probably best to wrap node's child_process with some helper class or function which returns a Promise. That way, we can take the same approach as MockRemoteAPI to assert the expected path etc... > Websites/perf.webkit.org/server-tests/resources/mock-child-process.js:24 > + execFile : function(binFile, commandArguments, callback) I don't think we want to use the abbreviation such as "bin". Just say "file" to match the name used in child_process. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:7 > +let BuildbotSyncer = require('./buildbot-syncer').BuildbotSyncer; I don't think this statement should exit here. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:30 > + return this._remoteAPI.getJSONWithStatus(`/api/commits/${config['name']}/last-reported?from=${minRevisionOrder}&to=${maxRevisionOrder}`).then((result) => { You probably want to assert that repositoryName doesn't contain special characters like /, %, etc... In fact, you might wanna just URL escape the name or assert that it consists of [A-Za-z0-9\-_] > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:34 > + return this._addSubCommitInfoForBuild(builds, command['subCommitCommand']); We should probably only call this if subCommitCommand is defined. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:41 > + _calculateOrder(revision) I think we usually call these "computeX". > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:45 > + const major = parseInt(match[1]); We should probably assert that the regular expression matches. In fact, we probably want a test case for that. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:52 > + _availableBuildsFromCommand(repositoryName, command, linesToIgnore, minOrder) This name is a bit misleading since we're turning an array of commit dictionaries here. _commitsForAvailableBuilds? > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:54 > + return this._executeCommand(command[0], command.slice(1)).then((output) => { I think we should take care of [0] and splitting thing in a newly added wrapper for child_process. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:57 > + return lines.map((line) => {return {'repository': repositoryName, 'revision': line, 'order': this._calculateOrder(line)}}) You can just do (line) => ({'repository': repositoryName}) or even (revision) => ({repository, revision, order: ~}) if you renamed repositoryName to repository. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:62 > + _executeCommand(commandFile, commandArguments) Like as talked in person, this should probably be a separate class. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:75 > + _addSubCommitInfoForBuild(commitInfo, command) I think we should call the first argument commitList or commits. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:91 > + fetchAndReportNewBuilds() I think it's best to put this function at the top since it's a public interface. > Websites/perf.webkit.org/tools/pull-os-versions.js:48 > + osConfigList.forEacch(function(osConfig) { Typo. I think it's better to use map here. > Websites/perf.webkit.org/tools/pull-os-versions.js:50 > + fetcherList.push(fetcher.fetchAndReportNewBuilds(serverConfig)); And just return the fetcher here instead of manually calling push.
Comment on attachment 304241 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304241&action=review > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:13 > +class MockLogger { think we should share the with the other test I had. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:29 > + MockData.resetV3Models(); You don't need this. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:52 > + const sampleSubCommit0 = { Please call this otherSampleCommit or something instead of suffixing it with a number. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:102 > + it('should calculate the right order for a given valid revision', done => { > + new Promise((resolve, reject) => { You don't need to have "done" argument here or wrap the tests in a promise. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:114 > + it('should only return commits whose orders are higher than specified order', (done) => { Instead of taking done here. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:118 > + fetchter._availableBuildsFromCommand("OSX", ["list", "build1"], "^\\.*$", 1604000000).then((results) => { Just return this promise. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:127 > + done(); In particular, this test is broken because you're calling done before the promised returned by _availableBuildsFromCommand is resolved. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:142 > + assert.equal(results[0]['repository'], sampleCommit['repository']); > + assert.equal(results[0]['revision'], sampleCommit['revision']); You can also do: assert.deepEqual to compare dictionaries / arrays. > Websites/perf.webkit.org/tools/pull-os-versions.js:26 > + global.AnalysisTask._fetchAllPromise = null; You don't need any of these. > Websites/perf.webkit.org/tools/pull-os-versions.js:46 > + console.log(`Fetching the manifest...`); > + > + Manifest.fetch().then(function () { It seems like this whole step is completely unnecessary.
Created attachment 304475 [details] Patch
Comment on attachment 304475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304475&action=review > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:88 > + function fetch_last_reported_between_orders($repository_id, $from=NULL, $to=NULL) I don't think we should make $from and $to optional if the function name says between_orders. > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:97 > + $statements = 'SELECT commit_id as "id", > + commit_revision as "revision", > + commit_previous_commit as "previousCommit", > + commit_time as "time", > + commit_order as "order", > + committer_name as "authorName", > + committer_account as "authorEmail", > + commit_message as "message" Instead of repeating these names, we can just fetch * and then call format_single_commit. > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:100 > + if ($from && $to) { Get rid of this if. > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:103 > + $statements .= ' AND commit_order >=' . $from . ' AND commit_order <= ' . $to . ' ORDER BY commit_order DESC LIMIT 1'; Please use $1 and $2 instead. > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:112 > + foreach ($commits as &$commit) > + $commit['time'] = Database::to_js_time($commit['time']); > + This is in fact not fetching committer information so this API is kind of broken. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:301 > - it("should distinguish between repositories with the asme name but with a different owner.", () => { > + it("should distinguish between repositories with the same name but with a different owner.", function () { Keep the arrow function. > Websites/perf.webkit.org/server-tests/resources/mock-subprocess.js:30 > + var originalSubprocess = global.Subprocess; Use const. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:35 > + 'WebKit': { > + 'revision': '141978', > + } Might wanna put this in a single line for simplicity. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:41 > + 'WebKit': { > + 'revision': '141999', > + } Ditto. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:50 > + 'WebKit': { > + 'revision': '142000', > + }, > + 'JavaScriptCore': { > + 'revision': '142000', > + } Ditto. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:106 > + it('should calculate the right order for a given valid revision', (done) => { For sync tests, you don't need "done" at all. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:134 > + const fetchCommitsPromise = fetchter._commitsForAvailableBuilds('OSX', ['list', 'build1'], '^\\.*$', 1604000000); Please declare a local variable like "const minimumOrder = 1604000000" so that the number doesn't look as mysterious. > Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:148 > + assert.equal(results.length, 2); > + assert.equal(results[0]['repository'], 'OSX'); > + assert.equal(results[0]['revision'], '16E321z'); > + assert.equal(results[0]['order'], 1604032126); > + assert.equal(results[1]['repository'], 'OSX'); > + assert.equal(results[1]['revision'], '16F321'); > + assert.equal(results[1]['order'], 1605032100); You might wanna just do deepEqual here. > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:23 > + return Promise.all(customCommands.map(command => { Nit: We usually wrap arguments in (). > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:29 > + let fetchCommitsPromise = this._remoteAPI.getJSONWithStatus(`/api/commits/${escape(repositoryName)}/last-reported?from=${minRevisionOrder}&to=${maxRevisionOrder}`).then(result => { I think we usually wrap argument in () for arrow functions as in (result) => { > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:45 > + const buildNameRegex = /(\d+)([a-zA-z])(\d+)([a-zA-z]*)/; It should be A-Z, not A-z. You might wanna even add a test! > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:71 > + commits.forEach((commit) => { I think it reads better to use "for (let commit of commits)" > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:75 > + commit['subCommits'] = JSON.parse(subCommitOutput); I think we should verify that the content returned by the command in a good form. We might wanna extract that as a separate function along with the call to _subprocess.execute. And then use Array.prototype.reduce: return commits.reduce((promise, commit) => { return promise.then(this._executeSubCommitCommand(command, commit['revision'])); }, Promise.resolve()); > Websites/perf.webkit.org/tools/js/os-build-fetcher.js:89 > + fetchAndReportNewBuilds() Again, please put this right after the constructor. > Websites/perf.webkit.org/tools/js/subprocess.js:5 > + call(command) { We probably should not call this function "call" since we have https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call. execute()? > Websites/perf.webkit.org/tools/pull-os-versions.js:30 > + // v3 models use the global RemoteAPI to access the perf dashboard. > + global.RemoteAPI = new RemoteAPI(serverConfig.server); You can just get rid of the global RemoteAPI here. I don't any code uses it. > Websites/perf.webkit.org/tools/pull-os-versions.js:34 > + Promise.all( > + osConfigList.map(osConfig => new OSBuildFetcher(osConfig, global.RemoteAPI, new Subprocess, serverConfig.slave, console)) > + ).catch(function (error) { I think it reads better to put these three lines into one.
Created attachment 304487 [details] Patch for landing
Comment on attachment 304487 [details] Patch for landing Clearing flags on attachment: 304487 Committed r213976: <http://trac.webkit.org/changeset/213976>
All reviewed patches have been landed. Closing bug.