<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=169542">bug 169542</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #304475 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=169542#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=169542">bug 169542</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=304475&action=diff" name="attach_304475" title="Patch">attachment 304475</a> <a href="attachment.cgi?id=304475&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=304475&action=review">https://bugs.webkit.org/attachment.cgi?id=304475&action=review</a>
<span class="quote">> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:88
> + function fetch_last_reported_between_orders($repository_id, $from=NULL, $to=NULL)</span >
I don't think we should make $from and $to optional if the function name says between_orders.
<span class="quote">> 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"</span >
Instead of repeating these names, we can just fetch * and then call format_single_commit.
<span class="quote">> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:100
> + if ($from && $to) {</span >
Get rid of this if.
<span class="quote">> 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';</span >
Please use $1 and $2 instead.
<span class="quote">> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:112
> + foreach ($commits as &$commit)
> + $commit['time'] = Database::to_js_time($commit['time']);
> +</span >
This is in fact not fetching committer information so this API is kind of broken.
<span class="quote">> 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 () {</span >
Keep the arrow function.
<span class="quote">> Websites/perf.webkit.org/server-tests/resources/mock-subprocess.js:30
> + var originalSubprocess = global.Subprocess;</span >
Use const.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:35
> + 'WebKit': {
> + 'revision': '141978',
> + }</span >
Might wanna put this in a single line for simplicity.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:41
> + 'WebKit': {
> + 'revision': '141999',
> + }</span >
Ditto.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:50
> + 'WebKit': {
> + 'revision': '142000',
> + },
> + 'JavaScriptCore': {
> + 'revision': '142000',
> + }</span >
Ditto.
<span class="quote">> 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) => {</span >
For sync tests, you don't need "done" at all.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:134
> + const fetchCommitsPromise = fetchter._commitsForAvailableBuilds('OSX', ['list', 'build1'], '^\\.*$', 1604000000);</span >
Please declare a local variable like "const minimumOrder = 1604000000" so that the number doesn't look as mysterious.
<span class="quote">> 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);</span >
You might wanna just do deepEqual here.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:23
> + return Promise.all(customCommands.map(command => {</span >
Nit: We usually wrap arguments in ().
<span class="quote">> 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 => {</span >
I think we usually wrap argument in () for arrow functions as in (result) => {
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:45
> + const buildNameRegex = /(\d+)([a-zA-z])(\d+)([a-zA-z]*)/;</span >
It should be A-Z, not A-z. You might wanna even add a test!
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:71
> + commits.forEach((commit) => {</span >
I think it reads better to use "for (let commit of commits)"
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:75
> + commit['subCommits'] = JSON.parse(subCommitOutput);</span >
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());
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:89
> + fetchAndReportNewBuilds()</span >
Again, please put this right after the constructor.
<span class="quote">> Websites/perf.webkit.org/tools/js/subprocess.js:5
> + call(command) {</span >
We probably should not call this function "call" since we have <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call">https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call</a>.
execute()?
<span class="quote">> 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);</span >
You can just get rid of the global RemoteAPI here.
I don't any code uses it.
<span class="quote">> 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) {</span >
I think it reads better to put these three lines into one.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>