[Webkit-unassigned] [Bug 169542] Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=169542

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #304241|review?                     |review-
              Flags|                            |

--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 304241
  --> https://bugs.webkit.org/attachment.cgi?id=304241
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170313/9f8b3611/attachment.html>