<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c3">Comment # 3</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>
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:13
> +class MockLogger {</span >
think we should share the with the other test I had.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:29
> + MockData.resetV3Models();</span >
You don't need this.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:52
> + const sampleSubCommit0 = {</span >
Please call this otherSampleCommit or something instead of suffixing it with a number.
<span class="quote">> 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) => {</span >
You don't need to have "done" argument here or wrap the tests in a promise.
<span class="quote">> 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) => {</span >
Instead of taking done here.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:118
> + fetchter._availableBuildsFromCommand("OSX", ["list", "build1"], "^\\.*$", 1604000000).then((results) => {</span >
Just return this promise.
<span class="quote">> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:127
> + done();</span >
In particular, this test is broken because you're calling done before the promised returned by _availableBuildsFromCommand is resolved.
<span class="quote">> 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']);</span >
You can also do: assert.deepEqual to compare dictionaries / arrays.
<span class="quote">> Websites/perf.webkit.org/tools/pull-os-versions.js:26
> + global.AnalysisTask._fetchAllPromise = null;</span >
You don't need any of these.
<span class="quote">> Websites/perf.webkit.org/tools/pull-os-versions.js:46
> + console.log(`Fetching the manifest...`);
> +
> + Manifest.fetch().then(function () {</span >
It seems like this whole step is completely unnecessary.</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>