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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits.

Created attachment 304241 [details]
patch

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[WebKit Commit Bot]

Comment on attachment 304487 [details]
Patch for landing

Clearing flags on attachment: 304487

Committed r213976: <http://trac.webkit.org/changeset/213976>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.