[Webkit-unassigned] [Bug 152913] Refactor compareIterations to remove duplicate code.

[bugzilla-daemon at webkit.org]

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

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #269369|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #13 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 269369
  --> https://bugs.webkit.org/attachment.cgi?id=269369
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269369&action=review

Thank you Jason for iterating on the patch. I have some minor suggestions.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:61
> +    var id1 = 42;
> +    var id2 = 43;

For your consideration, I suggest using values 1 and 2 for the identifiers for iteration1 and iteration2, respectively, and inline these values into the BuildbotIteration constructor call:

var iteration1 = new BuildbotIteration(this.queue, 1, finished);
var iteration2 = new BuildbotIteration(this.queue, 2, finished);

Then remove the local variables id1 and id2. It will be clear (or clear enough to allow a person to infer) that 1, 2 are the identifiers associated with the respective iteration objects because the ids are mirrored in the names of the variables.

Notice that you define id1 and id2 in other test functions. I suggest making similar changes throughout this patch.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:66
> +    iteration1.revision = { "openSource": 42 };
> +    iteration2.revision = { "openSource": 43 };

For your consideration, you may want to consider picking revision numbers that do not coincide with the ids so as to avoid giving a person the impression that the id of a BuildbotIteration is identical its revision.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:69
> +    ok(this.queue.compareIterations(iteration2, iteration2) === 0, "compareIterations: equal"); // if statement actually falls through

On first reading the inline comment I thought it was referring to the code in this function instead of the code in BuildbotQueue.prototype.compareIterations(). Regardless, I do not find this comment meaningful. If you feel that having a comment to describe that the iteration comparison will "fall through" to comparing iterations by their property id then I suggest explicitly referring to this aspect, say by writing a comment "compares iterations by id".

On another note, we could use strictEqual() to write this line (you do not need to make this change in this patch). See <http://api.qunitjs.com/strictEqual/> for more details.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:72
> +test("compareIterations by loaded", function()

For your consideration, I suggest either adding another test that is identical to this one and explicitly defines revisions for the iterations or modify this test to explicitly define revisions for the iterations. I suggest having at least one test explicitly define revisions so as to be more representative of a real-world iteration object.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:85
> +test("compareIterations by id", function()

Similar to my suggestion for test "compareIterations by loaded", I suggest either modifying this test or adding another test that explicitly defines both revisions and loaded for the iterations that are being compared to better reflect real-world iteration objects. This also minimizes the assumptions on the default initialization of the iterations objects (though this is also valuable to test).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:94
> +    ok(this.queue.compareIterations(iteration2, iteration2) === 0, "compareIterations: equal");

Nit: This is OK as-is. We could write this using strictEqual().

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:97
> +test("compareIterationsByRevisions", function()

For your consideration, I suggest that we explicitly define the loaded property for iterations1 and iteration2 to be true and false, respectively, to ensure that compareIterationsByRevisions() does not care about the loaded state of the iterations.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:108
> +    ok(this.queue.compareIterationsByRevisions(iteration2, iteration2) === 0, "compareIterationsByRevisions: equal");

Nit: This is OK as-is. We could write this using strictEqual().

-- 
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/20160120/397cdf3f/attachment-0001.html>