Bug 152910 - Add a unit test to test BuildbotQueueView._appendPendingRevisionCount.
Summary: Add a unit test to test BuildbotQueueView._appendPendingRevisionCount.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 152345 152913 152982
  Show dependency treegraph
 
Reported: 2016-01-08 11:33 PST by Jason Marcell
Modified: 2016-01-14 11:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (138.40 KB, patch)
2016-01-08 11:33 PST, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (138.42 KB, patch)
2016-01-08 11:40 PST, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (138.28 KB, patch)
2016-01-11 14:35 PST, Jason Marcell
no flags Details | Formatted Diff | Diff
Patch (19.91 KB, patch)
2016-01-13 16:58 PST, Jason Marcell
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2016-01-08 11:33:08 PST
Add QUnit for unit testing and add a unit test to test BuildbotQueueView._appendPendingRevisionCount.
Comment 1 Jason Marcell 2016-01-08 11:33:53 PST
Created attachment 268560 [details]
Patch
Comment 2 Jason Marcell 2016-01-08 11:40:42 PST
Created attachment 268561 [details]
Patch
Comment 3 Dean Johnson 2016-01-11 12:23:41 PST
Comment on attachment 268560 [details]
Patch

LGTM!
Comment 4 Daniel Bates 2016-01-11 12:58:57 PST
Comment on attachment 268561 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:2
> +QUnit.test("BuildBotQueue Test", function( assert )

Please remove the space characters inside the parentheses.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:5
> +    assert.ok(trac, "trac is not null");

What is the purpose of this test? I mean, this test is not meaningful given that trac is guaranteed to be non-null by definition of the new operator.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:11
> +            name: "Webkit Repo",

Nit: Webkit => WebKit

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:15
> +    assert.ok(queue, "queue is not null");

Similarly, this test is not meaningful by the same reason as given in my remark for line 5. Moreover, if queue was null then this script would cause a JavaScript TypeError when we assign property branches to it on line 8; => we would never execute this line.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:18
> +    assert.ok(view, "view is not null");

Similarly, this is test is not meaningful.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:22
> +    var revisionsBehind = view.element.getElementsByClassName("message")[0].innerHTML.match(/.*(\d+) revision(|s) behind/)[1];
> +    assert.equal(revisionsBehind, 1, "assert revisions behind");

Can you elaborate on how we are one revision behind?
Comment 5 Jason Marcell 2016-01-11 14:12:41 PST
(In reply to comment #4)
> Comment on attachment 268561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268561&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:2
> > +QUnit.test("BuildBotQueue Test", function( assert )
> 
> Please remove the space characters inside the parentheses.

Will do.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:5
> > +    assert.ok(trac, "trac is not null");
> 
> What is the purpose of this test? I mean, this test is not meaningful given
> that trac is guaranteed to be non-null by definition of the new operator.

Will remove.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:11
> > +            name: "Webkit Repo",
> 
> Nit: Webkit => WebKit

Will fix.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:15
> > +    assert.ok(queue, "queue is not null");
> 
> Similarly, this test is not meaningful by the same reason as given in my
> remark for line 5. Moreover, if queue was null then this script would cause
> a JavaScript TypeError when we assign property branches to it on line 8; =>
> we would never execute this line.

Will remove.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:18
> > +    assert.ok(view, "view is not null");
> 
> Similarly, this is test is not meaningful.

Ditto.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:22
> > +    var revisionsBehind = view.element.getElementsByClassName("message")[0].innerHTML.match(/.*(\d+) revision(|s) behind/)[1];
> > +    assert.equal(revisionsBehind, 1, "assert revisions behind");
> 
> Can you elaborate on how we are one revision behind?

The MockBuildbotQueueView is programmed to return 33020 as the "_latestProductiveIteration" whereas the "latestRecordedRevisionNumber" is 33022 and 33021 is on "someOtherBranch". Therefore, out of all of the known revision numbers on "trunk", there is only one revision, 33022, which is later than the "_latestProductiveIteration".
Comment 6 Jason Marcell 2016-01-11 14:35:57 PST
Created attachment 268715 [details]
Patch
Comment 7 Daniel Bates 2016-01-13 12:37:11 PST
Comment on attachment 268715 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2016-01-13 13:24:36 PST
Comment on attachment 268715 [details]
Patch

Rejecting attachment 268715 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 268715, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0-ab3c-d52691b4dbfc ...
Currently at 194969 = 6659b1fa06ca46a239e2d27a2f80c6958755a196
r194976 = a9120de96b5de25dcf995589183cac998cee765c
r194977 = e0787c86de857f00c6a31d1d1c7be2911d125115
r194978 = 2fbf587dcfbfba20d661a6ae9b990fd424ffb3e0
r194979 = 7d5c95e93fceb042bbf91c048666251a8147607d
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/687618
Comment 9 Jason Marcell 2016-01-13 14:44:11 PST
I think it failed to apply because of the tabs in QUnit.

However, this is probably a fortunate thing because I recently noticed that we already have QUnit in our codebase:
https://trac.webkit.org/browser/trunk/Source/ThirdParty/qunit

I will prepare a new patch that uses this copy of QUnit instead of needlessly adding QUnit to our repository a second time.
Comment 10 Jason Marcell 2016-01-13 15:31:25 PST
It looks like QUnit was added here https://bugs.webkit.org/show_bug.cgi?id=63967
Unfortunately it was also added here: https://bugs.webkit.org/show_bug.cgi?id=55911

It looks like it's being used in these files:
Tools/TestResultServer/static-dashboards/run-unittests.html
Tools/TestResultServer/static-dashboards/run-embedded-unittests.html
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/run-unittests.html

I'll look at how we have previously used QUnit and try to match any patterns and conventions previously used.

At first glance, however, I'm already encountering some issues likely caused by the fact that this version of QUnit is about five years older than the one I was initially using when I created my patch.
Comment 11 Jason Marcell 2016-01-13 16:24:55 PST
I can either downgrade my work to comply with QUnit 1.x or upgrade the existing QUnit tests in our codebase to QUnit 2.x:
http://qunitjs.com/upgrade-guide-2.x/

I think it might be easiest to downgrade my work (for now) and come along later with a patch (or patches) that upgrades our unit tests to QUnit 2.x in the future.
Comment 12 Jason Marcell 2016-01-13 16:58:41 PST
Created attachment 268920 [details]
Patch
Comment 13 Jason Marcell 2016-01-13 17:00:52 PST
Comment on attachment 268920 [details]
Patch

Using already existing QUnit. Made a few changes to be compatible with the older version of QUnit. Added license headers to the new files that I'm adding.
Comment 14 Daniel Bates 2016-01-13 17:58:43 PST
Comment on attachment 268920 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockBuildbotQueueView.js:34
> +    this.element = document.createElement("div");
> +    this.element.classList.add("queue-view");
> +    this.element.__queueView = this;
> +
> +    this.queues = queues || [];
> +
> +    BaseObject.call(this);

It may be nice if we could make this class extend QueueView instead of duplicating similar logic here. We can do that in a separate patch if it turns out to be useful.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockBuildbotQueueView.js:47
> +                "WebKit Repo": 33020

Nit: You may want to consider using the same repository name as used in production.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockOverrides.js:26
> +// The real "label" setter was throwing an exception in the unit tests so I overrode it with a simplified version.

Should this be a FIXME comment?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:29
> +    <meta charset="UTF-8" />

Nit: The slash is not necessary as this is a HTML5 document.
Comment 15 Jason Marcell 2016-01-14 11:05:07 PST
(In reply to comment #14)
> Comment on attachment 268920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268920&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockBuildbotQueueView.js:34
> > +    this.element = document.createElement("div");
> > +    this.element.classList.add("queue-view");
> > +    this.element.__queueView = this;
> > +
> > +    this.queues = queues || [];
> > +
> > +    BaseObject.call(this);
> 
> It may be nice if we could make this class extend QueueView instead of
> duplicating similar logic here. We can do that in a separate patch if it
> turns out to be useful.

I will look into this for a future patch.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockBuildbotQueueView.js:47
> > +                "WebKit Repo": 33020
> 
> Nit: You may want to consider using the same repository name as used in
> production.

I did this.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/MockOverrides.js:26
> > +// The real "label" setter was throwing an exception in the unit tests so I overrode it with a simplified version.
> 
> Should this be a FIXME comment?

I investigated this further. The error that I was working around seemed to be particular to PhantomJS but did not affect regular usage (i.e. running the tests in Safari). Therefore I just removed this file from that patch.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html:29
> > +    <meta charset="UTF-8" />
> 
> Nit: The slash is not necessary as this is a HTML5 document.

Removed the slash.

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