Refactor compareIterations to remove duplicate code.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Marcell]

Refactor compareIterations to remove duplicate code.

[Jason Marcell]

Created attachment 268564 [details]
Patch

[Jason Marcell]

Created attachment 268733 [details]
Patch

[Jason Marcell]

Created attachment 268981 [details]
Patch

[Jason Marcell]

Comment on attachment 268981 [details]
Patch

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

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

Additional explanation:

My intention for the three groups of "compareIterations" tests was to give full code coverage by forcing the execution path through the three different return statements.

    compareIterations: function(a, b)
    {
        result = this.compareIterationsByRevisions(a, b);
        if (result)
1.          return result;

        // A loaded iteration may not have revision numbers if it failed early, before svn steps finished.
        result = b.loaded - a.loaded;
        if (result)
2.          return result;

3.      return b.id - a.id;
    },

For this particular test we call this.compareIterationsByRevisions and it returns 0, which is falsy, and so we don't actually return via the first return statement -- it falls through.

[Dean Johnson]

LGTM! rs=me

[Daniel Bates]

Comment on attachment 268981 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:48
> +        queue = new MockBuildbotQueue();

This is error prone. Specifically, it is error prone to pass module state using global variables. From looking at the examples for setup()/tearDown() on <http://qunitjs.com/upgrade-guide-2.x/> and beforeEach()/afterEach() on <http://api.qunitjs.com/QUnit.module/> the preferred way to share such state is by modifying |this|.

[Daniel Bates]

Comment on attachment 268981 [details]
Patch

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

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

I suggest that we instantiate local BuildbotIterations in each test function, including this one, instead of modifying the "global" iteration instances. Shared objects between tests should be immutable and/or conveyed to be immutable. The benefit of using this approach and only sharing immutable data between tests is that it makes it straightforward to verify correctness.

[Jason Marcell]

Created attachment 269364 [details]
Patch

[Jason Marcell]

(In reply to comment #6)
> Comment on attachment 268981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268981&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:48
> > +        queue = new MockBuildbotQueue();
> 
> This is error prone. Specifically, it is error prone to pass module state
> using global variables. From looking at the examples for setup()/tearDown()
> on <http://qunitjs.com/upgrade-guide-2.x/> and beforeEach()/afterEach() on
> <http://api.qunitjs.com/QUnit.module/> the preferred way to share such state
> is by modifying |this|.

I addressed in the latest patch.

[Jason Marcell]

(In reply to comment #7)
> Comment on attachment 268981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268981&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:65
> > +    iteration1.revision = { "openSource": 42 };
> 
> I suggest that we instantiate local BuildbotIterations in each test
> function, including this one, instead of modifying the "global" iteration
> instances. Shared objects between tests should be immutable and/or conveyed
> to be immutable. The benefit of using this approach and only sharing
> immutable data between tests is that it makes it straightforward to verify
> correctness.

I addressed in the latest patch.

[Jason Marcell]

Created attachment 269369 [details]
Patch

[Jason Marcell]

I had to add .bind(this) to the call to compareIterations.

[Daniel Bates]

Comment on attachment 269369 [details]
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().

[Jason Marcell]

Created attachment 269391 [details]
Patch

[Jason Marcell]

Thanks, Dan. I think I addressed everything you mentioned in your last review in my latest patch. Mind having another look before I land this?

[Jason Marcell]

Comment on attachment 269391 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:273
> +            console.log("   compareIterations by revisions");

Oops. I will remove this before landing.

[Jason Marcell]

Created attachment 269395 [details]
Patch

[Daniel Bates]

Comment on attachment 269395 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:77
> +    iteration1.revision = {};

This is OK as-is. I meant to suggest explicitly specifying a repository revision.

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

This is OK as-is. I meant to suggest explicitly specifying repository revisions.

[Jason Marcell]

Created attachment 269398 [details]
Patch

[Jason Marcell]

Landed: https://trac.webkit.org/changeset/195391