<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 #304241 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#c2">Comment # 2</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=304241&action=diff" name="attach_304241" title="patch">attachment 304241</a> <a href="attachment.cgi?id=304241&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=304241&action=review">https://bugs.webkit.org/attachment.cgi?id=304241&action=review</a>
I'd say r- for now given the list of issues I listed below.
<span class="quote">> Websites/perf.webkit.org/server-tests/resources/mock-child-process.js:3
> +MockChildProcess = {</span >
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...
<span class="quote">> Websites/perf.webkit.org/server-tests/resources/mock-child-process.js:24
> + execFile : function(binFile, commandArguments, callback)</span >
I don't think we want to use the abbreviation such as "bin". Just say "file" to match the name used in child_process.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:7
> +let BuildbotSyncer = require('./buildbot-syncer').BuildbotSyncer;</span >
I don't think this statement should exit here.
<span class="quote">> 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) => {</span >
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\-_]
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:34
> + return this._addSubCommitInfoForBuild(builds, command['subCommitCommand']);</span >
We should probably only call this if subCommitCommand is defined.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:41
> + _calculateOrder(revision)</span >
I think we usually call these "computeX".
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:45
> + const major = parseInt(match[1]);</span >
We should probably assert that the regular expression matches.
In fact, we probably want a test case for that.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:52
> + _availableBuildsFromCommand(repositoryName, command, linesToIgnore, minOrder)</span >
This name is a bit misleading since we're turning an array of commit dictionaries here. _commitsForAvailableBuilds?
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:54
> + return this._executeCommand(command[0], command.slice(1)).then((output) => {</span >
I think we should take care of [0] and splitting thing in a newly added wrapper for child_process.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:57
> + return lines.map((line) => {return {'repository': repositoryName, 'revision': line, 'order': this._calculateOrder(line)}})</span >
You can just do (line) => ({'repository': repositoryName})
or even (revision) => ({repository, revision, order: ~}) if you renamed repositoryName to repository.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:62
> + _executeCommand(commandFile, commandArguments)</span >
Like as talked in person, this should probably be a separate class.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:75
> + _addSubCommitInfoForBuild(commitInfo, command)</span >
I think we should call the first argument commitList or commits.
<span class="quote">> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:91
> + fetchAndReportNewBuilds()</span >
I think it's best to put this function at the top since it's a public interface.
<span class="quote">> Websites/perf.webkit.org/tools/pull-os-versions.js:48
> + osConfigList.forEacch(function(osConfig) {</span >
Typo. I think it's better to use map here.
<span class="quote">> Websites/perf.webkit.org/tools/pull-os-versions.js:50
> + fetcherList.push(fetcher.fetchAndReportNewBuilds(serverConfig));</span >
And just return the fetcher here instead of manually calling push.</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>