Add Unit Tests to SnackbarCollection

Add Unit Tests to SnackbarCollection

In this CL, SnackbarCollection is pulled out from SnackbarManager and is
tested by JUnit test. All public methods in SnackbarCollection are
covered in the unit tests.

This CL also fixes a bug in removeMatchingSnackbars() that was found while
writing the tests. Hooray!

BUG=579347
Committed: https://crrev.com/276c80ef8a8b060e60400f85c6b411bd3e257b79
Cr-Commit-Position: refs/heads/master@{#372277}

[Ian Wen, 2016-01-27 22:24:14]

ianwen@chromium.org changed reviewers:
+ newt@chromium.org

[Ian Wen, 2016-01-27 22:24:15]

PTAL.

[newt (away), 2016-01-28 17:32:00]

Thanks for writing unit tests!

A few comments inline.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java
(right):

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:15:
* A collection of {@link Snackbar}s.
Explain why this class exists :)

The current comment is basically useless; it just repeats the name of the class.
(Unfortunately, this is a relatively common pattern in our code.)

As a developer looking at this class for the first time, my major question would
be: "Why the heck do we need a custom collection for snackbars? Why not just use
an ArrayList?"

The comment doesn't need to be long, but it should be long enough to answer the
most fundamental question of why this class exists and what its primary function
is.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:27:
public void add(Snackbar snackbar) {
My personal preference is to remove "public" on all these methods, since they're
all package-private anyway due to the class scope anyway.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:42:
public Snackbar removeCurrent(boolean isAction) {
Since removeCurrent(false) shouldn't be called by outsiders, you might want to
make this method private and add a new public method
"removeCurrentDueToAction()" that calls removeCurrent(true).

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java
(right):

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:31:
public void testAddOverride() throws Exception {
maybe: "testActionCoversNotification"

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:44:
assertEquals("Notibar was not overriden by actionBar!", actionBar,
collection.getCurrent());
use terms that other developers who see this test failure can understand.
"notibar" probably doesn't count ;)

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:64:
assertFalse(onDismissWasCalled());
It's confusing which snackbar onDismiss() might be associated with. Using
mockito (as explained below) and using separate SnackbarControllers for each
snackbar would make this clearer.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:159:
private boolean onActionWasCalled() {
You could use Mockito to mock out a SnackbarController. Something like:

  SnackbarController controller = mock(SnackbarController.class);
  verify(controller, times(1)).onAction(null);

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:161:
mActionCalled = false;
Resetting these values inside a method whose name indicates that it's a pure
getter is a bit suspect. I definitely would advise against it for production
code; even for test code, it's confusing.

Using mockito is one way around this issue. Or you could add separate methods
for resetting these values. Or you could change the method name to indicate that
it both reads and resets the value.

[Ian Wen, 2016-01-29 02:38:43]

All addressed. Mockito is indeed very handy to use. :P

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java
(right):

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:15:
* A collection of {@link Snackbar}s.
On 2016/01/28 17:32:00, newt wrote:
> Explain why this class exists :)
> 
> The current comment is basically useless; it just repeats the name of the
class.
> (Unfortunately, this is a relatively common pattern in our code.)
> 
> As a developer looking at this class for the first time, my major question
would
> be: "Why the heck do we need a custom collection for snackbars? Why not just
use
> an ArrayList?"
> 
> The comment doesn't need to be long, but it should be long enough to answer
the
> most fundamental question of why this class exists and what its primary
function
> is.

Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:27:
public void add(Snackbar snackbar) {
On 2016/01/28 17:32:00, newt wrote:
> My personal preference is to remove "public" on all these methods, since
they're
> all package-private anyway due to the class scope anyway.

Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarCollection.java:42:
public Snackbar removeCurrent(boolean isAction) {
On 2016/01/28 17:32:00, newt wrote:
> Since removeCurrent(false) shouldn't be called by outsiders, you might want to
> make this method private and add a new public method
> "removeCurrentDueToAction()" that calls removeCurrent(true).

Ah good suggestion!! Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java
(right):

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:31:
public void testAddOverride() throws Exception {
On 2016/01/28 17:32:00, newt wrote:
> maybe: "testActionCoversNotification"

Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:44:
assertEquals("Notibar was not overriden by actionBar!", actionBar,
collection.getCurrent());
On 2016/01/28 17:32:00, newt wrote:
> use terms that other developers who see this test failure can understand.
> "notibar" probably doesn't count ;)

Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:159:
private boolean onActionWasCalled() {
On 2016/01/28 17:32:00, newt wrote:
> You could use Mockito to mock out a SnackbarController. Something like:
> 
>   SnackbarController controller = mock(SnackbarController.class);
>   verify(controller, times(1)).onAction(null);

Done.

https://codereview.chromium.org/1649543002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:161:
mActionCalled = false;
On 2016/01/28 17:32:00, newt wrote:
> Resetting these values inside a method whose name indicates that it's a pure
> getter is a bit suspect. I definitely would advise against it for production
> code; even for test code, it's confusing.
> 
> Using mockito is one way around this issue. Or you could add separate methods
> for resetting these values. Or you could change the method name to indicate
that
> it both reads and resets the value.

Done.

[newt (away), 2016-01-29 02:49:01]

lgtm after comment

Looks good!

https://codereview.chromium.org/1649543002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java
(right):

https://codereview.chromium.org/1649543002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:77:
assertEquals("Action snackbar should ", actionBar, collection.getCurrent());
missing end of sentence

[Ian Wen, 2016-01-29 02:53:21]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-01-29 02:53:22]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1649543002/#ps40001
(title: "nit")

[Ian Wen, 2016-01-29 02:53:32]

https://codereview.chromium.org/1649543002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java
(right):

https://codereview.chromium.org/1649543002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java:77:
assertEquals("Action snackbar should ", actionBar, collection.getCurrent());
On 2016/01/29 02:49:01, newt wrote:
> missing end of sentence

Done.

[commit-bot: I haz the power, 2016-01-29 02:53:55]

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

[commit-bot: I haz the power, 2016-01-29 03:34:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-29 03:36:03]

Description was changed from

==========
Add Unit Tests to SnackbarCollection

In this CL, SnackbarCollection is pulled out from SnackbarManager and is
tested by JUnit test. All public methods in SnackbarCollection are
covered in the unit tests.

This CL also fixes a bug in removeMatchingSnackbars() that was found while
writing the tests. Hooray!

BUG=579347
==========

to

==========
Add Unit Tests to SnackbarCollection

In this CL, SnackbarCollection is pulled out from SnackbarManager and is
tested by JUnit test. All public methods in SnackbarCollection are
covered in the unit tests.

This CL also fixes a bug in removeMatchingSnackbars() that was found while
writing the tests. Hooray!

BUG=579347

Committed: https://crrev.com/276c80ef8a8b060e60400f85c6b411bd3e257b79
Cr-Commit-Position: refs/heads/master@{#372277}
==========

[commit-bot: I haz the power, 2016-01-29 03:36:04]

Patchset 3 (id:??) landed as
https://crrev.com/276c80ef8a8b060e60400f85c6b411bd3e257b79
Cr-Commit-Position: refs/heads/master@{#372277}