[MD Bookmarks] Add toasts.

chrome/test/data/webui/md_bookmarks/toast_manager_test.js

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+suite('<bookmarks-toast-manager>', function() {
+  var toastManager;
+  var store;
+
+  setup(function() {
+    toastManager = document.createElement('bookmarks-toast-manager');
+    replaceBody(toastManager);
+  });
+
+  test('simple show/hide', function() {
+    toastManager.show('test', false);
+    assertTrue(toastManager.open_);
+    assertEquals('test', toastManager.$.content.textContent);
+    assertTrue(toastManager.$$('paper-button').hidden);
+
+    toastManager.hide();
+    assertFalse(toastManager.open_);
+
+    toastManager.show('test', true);
+    assertFalse(toastManager.$$('paper-button').hidden);
+  });
+
+  test('auto hide', function() {
+    toastManager.duration = 100;
+
+    var timeoutFunc = null;
+    var timeoutCounter = 0;
+    var clearedTimeout = null;
+    toastManager.setTimeout_ = function(f) {

[dpapad, 2017/06/06 01:33:41]

Is there an alternative way to test this functionality without replacing private
methods (and in general recferring to private properties of toastManager)? How
about the following?

1) change toastManager.show() to return the #toast div.
2) call toastMnaager.show() in this test.
3) Wait for duration + x (say 100ms + 10ms).
4) Call window.getComputedStyle(#toast div), assert that it is hidden (display
hidden).

The gist of the above approach is to test the observed behavior not this
particular implementation.

[calamity, 2017/06/06 04:44:05]

I'm extremely wary of adding sleeps to tests. This causes unnecessary and costly
test slowdown (100ms per show() call for all browser test runs forever) and is a
very common vector for flakes. Mocking out a fake setTimeout and clearTimeout is
a compromise between encapsulation and testability. We are able to assert
precise timing behaviors because we can mock out the sequence of events (e.g the
3rd block where a second show() is called before the first ends). These are
worth testing, and can't reliably be done when timers are introduced.

Testing for the computed style here is also more volatile, as the styles are
more likely to change than the behavior of the open_ property. I think ensuring
that the reason that the styles are applied (the state of open_) is a more
robust test of observed behavior than the particular CSS implementation.

I think that black box testing with intricate internals that are only subtly
expressed via public properties can miss a lot of cases, and exposing these
internals for the sake of testing would lead to a bloated public API. The way
we've been writing tests has aimed to balance clean public APIs and relatively
clean internal implementations, without compromising testability.

[dpapad, 2017/06/06 18:18:41]

I only mentioned 100ms as an example. You could even specify as 0 or 1 and still
be able to test the code without slowing down the test.

Having said that I understand the argument about encapsulation and testability,
but I do think that current approach leans heavily on the encapsulation side of
the trade-off. Test-friendly code is written in a way that makes it easier to
test. In other words, it is OK to reasonably tweak the prod code to allow easier
testing. Current approach does not take tests into account, and relies on the
"bad parts" of JS, where there is no real "private" state enforcement.

So, here is an alternative approach (that still allows you to test the case of
calling show() twice):

In toast_manager.js, group the timing related functions to |timerApi_| object
and add a way to override it.

timerApi_: {
  clearTimeout: window.clearTimeout.bind(window),
  setTimeout: window.setTimeout.bind(window),
},

// Used only in tests.
setTimerApi: function(timerApi) { this.timerApi_ = timerApi; },

in toast_manager_test.js pass your own fake timerApi that does exactly what you
need for testing.

toastManager.setTimerApi({
  ...
});

That seems a better trade-off IMO. In my experience with JS tests, there is
almost always a reasonable way to avoid referring to private member variables
and still be able to test all code paths/cases. Consider my suggestion optional,
since I know that some aspects of what is considered "reasonable tweaks" are
subjective.

+      timeoutFunc = f;
+      return timeoutCounter++;
+    };
+    toastManager.clearTimeout_ = function(n) {
+      clearedTimeout = n;
+    };
+
+    toastManager.show('test', false);
+    assertEquals(0, toastManager.hideTimeout_);
+    assertTrue(toastManager.open_);
+
+    timeoutFunc();
+    assertEquals(null, toastManager.hideTimeout_);
+    assertFalse(toastManager.open_);
+
+    // Check that multiple shows reset the timeout.
+    toastManager.show('test', false);
+    assertEquals(1, toastManager.hideTimeout_);
+    assertTrue(toastManager.open_);
+
+    toastManager.show('test2', false);
+    assertEquals(1, clearedTimeout);
+    assertEquals(2, toastManager.hideTimeout_);
+    assertTrue(toastManager.open_);
+  });
+});