[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

--- Comment #3 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

> 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.

-- 
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/59d5a3c4/attachment-0001.html>