MD WebUI: Add tests for cr-toolbar-search-field

MD WebUI: Add tests for cr-toolbar-search-field

BUG=610609
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f66a6d6e8b7e87e6eb020880097561d00687bfe5
Cr-Commit-Position: refs/heads/master@{#396399}

[tsergeant, 2016-05-26 05:13:57]

tsergeant@chromium.org changed reviewers:
+ dpapad@chromium.org

[tsergeant, 2016-05-26 05:13:58]

As discussed, PTAL.

[dpapad, 2016-05-26 21:43:37]

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:31: var searches;
How about making this field a member variable of the delegate instead (see
related following snippets)?

var delegate = null;

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:34: function mockSearch(term)
{
"mock" is an overloaded term IMO. Personally I think either of the following
names would be clearer
simulateSearch()
triggerSearch()

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:43: function
MockSearchFieldDelegate() {
/**
  * @constructor
  * @implements {SearchFieldDelegate}
  */
function MockSearchFieldDelegate() {
  /** @type {!Array<string>} */
  this.searches = [];
}

MockSearchFieldDelegate.prototype = {
  /** @override */
  onSearchTermSearch: function(term) {
    searches.push(term);
  }
};

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:60: searches = [];
// no need to instantiate both "searches" and a new delegate every time.
// Constructor does that now.
delegate = new MockSearchFieldDelegate();

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:63: field.setDelegate(new
MockSearchFieldDelegate());
field.setDelegate(delegate);

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:64: });
It is generally good practice to have tearDown undo what setUp has done before,
such that no unwanted state is leaked between tests.


tearDown(function() {
  field.remove();
  field = null;
});

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:67: return
Promise.resolve().then(function() {
Why are you using a Promise here? This test does not do anything asynchronous.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:71: }).then(function() {
Same here (and below). Why is starting a new function here needed?

[dpapad, 2016-05-26 21:44:47]

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:43: function
MockSearchFieldDelegate() {
On 2016/05/26 at 21:43:36, dpapad wrote:
> /**
>   * @constructor
>   * @implements {SearchFieldDelegate}
>   */
> function MockSearchFieldDelegate() {
>   /** @type {!Array<string>} */
>   this.searches = [];
> }
> 
> MockSearchFieldDelegate.prototype = {
>   /** @override */
>   onSearchTermSearch: function(term) {
>     searches.push(term);
>   }
> };

Small correction on my previous suggestion:
this.searches.push(term);

[tsergeant, 2016-05-27 01:14:02]

Description was changed from

==========
MD WebUI: Add tests for cr-toolbar-search-field

BUG=610609
==========

to

==========
MD WebUI: Add tests for cr-toolbar-search-field

BUG=610609
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-05-27 01:16:54]

Patchset #3 (id:40001) has been deleted

[tsergeant, 2016-05-27 01:20:03]

Patchset #3 (id:60001) has been deleted

[tsergeant, 2016-05-27 01:20:14]

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:31: var searches;
On 2016/05/26 21:43:36, dpapad wrote:
> How about making this field a member variable of the delegate instead (see
> related following snippets)?
> 
> var delegate = null;

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:34: function mockSearch(term)
{
On 2016/05/26 21:43:37, dpapad wrote:
> "mock" is an overloaded term IMO. Personally I think either of the following
> names would be clearer
> simulateSearch()
> triggerSearch()

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:43: function
MockSearchFieldDelegate() {
On 2016/05/26 21:44:47, dpapad wrote:
> On 2016/05/26 at 21:43:36, dpapad wrote:
> > /**
> >   * @constructor
> >   * @implements {SearchFieldDelegate}
> >   */
> > function MockSearchFieldDelegate() {
> >   /** @type {!Array<string>} */
> >   this.searches = [];
> > }
> > 
> > MockSearchFieldDelegate.prototype = {
> >   /** @override */
> >   onSearchTermSearch: function(term) {
> >     searches.push(term);
> >   }
> > };
> 
> Small correction on my previous suggestion:
> this.searches.push(term);

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:60: searches = [];
On 2016/05/26 21:43:36, dpapad wrote:
> // no need to instantiate both "searches" and a new delegate every time.
> // Constructor does that now.
> delegate = new MockSearchFieldDelegate();

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:63: field.setDelegate(new
MockSearchFieldDelegate());
On 2016/05/26 21:43:36, dpapad wrote:
> field.setDelegate(delegate);

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:64: });
On 2016/05/26 21:43:36, dpapad wrote:
> It is generally good practice to have tearDown undo what setUp has done
before,
> such that no unwanted state is leaked between tests.
> 
> 
> tearDown(function() {
>   field.remove();
>   field = null;
> });

Done.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:67: return
Promise.resolve().then(function() {
On 2016/05/26 21:43:36, dpapad wrote:
> Why are you using a Promise here? This test does not do anything asynchronous.

CrSearchFieldBehavior.focus_() was previously async (due to the dom-if), which
is what I was hitting up against here.

I've removed the async bits from that method, and now this test can be
synchronous as well.

https://codereview.chromium.org/2017523002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:71: }).then(function() {
On 2016/05/26 21:43:36, dpapad wrote:
> Same here (and below). Why is starting a new function here needed?

See above.

[dpapad, 2016-05-27 01:31:12]

LGTM with nits.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:5: /** @fileoverview Suite of
tests for cr elements. */
Nit: Mention the exact element being tested here.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:29: var field;
Let's either instantiate both field and delegate to null, or leave both of them
undefined, for consistency. Also, let's add "?" in front of the types to be more
explicit.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:49: onSearchTermSearch:
function(term) {
Nit: @override

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:102: assertEquals(2,
delegate.searches.length);
Nit: You can replace all three assertions with a single one

assertEquals(['test', ''].join(), delegate.searches.join());

[tsergeant, 2016-05-27 03:07:37]

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:5: /** @fileoverview Suite of
tests for cr elements. */
On 2016/05/27 01:31:12, dpapad wrote:
> Nit: Mention the exact element being tested here.

Done.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:29: var field;
On 2016/05/27 01:31:12, dpapad wrote:
> Let's either instantiate both field and delegate to null, or leave both of
them
> undefined, for consistency. Also, let's add "?" in front of the types to be
more
> explicit.

Done.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:49: onSearchTermSearch:
function(term) {
On 2016/05/27 01:31:12, dpapad wrote:
> Nit: @override

Done.

https://codereview.chromium.org/2017523002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements_browsertest.js:102: assertEquals(2,
delegate.searches.length);
On 2016/05/27 01:31:12, dpapad wrote:
> Nit: You can replace all three assertions with a single one
> 
> assertEquals(['test', ''].join(), delegate.searches.join());

Done.

[tsergeant, 2016-05-27 03:10:02]

Patchset #4 (id:100001) has been deleted

[tsergeant, 2016-05-27 03:11:16]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2016-05-27 03:11:16]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2017523002/#ps120001
(title: "Review comments")

[commit-bot: I haz the power, 2016-05-27 03:11:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2017523002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2017523002/120001

[commit-bot: I haz the power, 2016-05-27 05:31:00]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-05-27 05:32:39]

Description was changed from

==========
MD WebUI: Add tests for cr-toolbar-search-field

BUG=610609
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD WebUI: Add tests for cr-toolbar-search-field

BUG=610609
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f66a6d6e8b7e87e6eb020880097561d00687bfe5
Cr-Commit-Position: refs/heads/master@{#396399}
==========

[commit-bot: I haz the power, 2016-05-27 05:32:40]

Patchset 4 (id:??) landed as
https://crrev.com/f66a6d6e8b7e87e6eb020880097561d00687bfe5
Cr-Commit-Position: refs/heads/master@{#396399}