change site-settings -> usb-device to use cr-action-menu instead of paper-menu

chrome/test/data/webui/settings/usb_devices_tests.js

+// Copyright 2016 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.
+
+/** @fileoverview Suite of tests for usb_devices. */
+cr.define('usb_devices', function() {
+  function registerTests() {
+    suite('UsbDevices', function() {
+      /**
+       * A dummy usb-devices element created before each test.
+       * @type {UsbDeviceEntry}
+       */
+      var testElement;
+
+      /**
+       * The mock proxy object to use during test.
+       * @type {TestSiteSettingsPrefsBrowserProxy}
+       */
+      var browserProxy = null;
+
+      /**
+       * An example USB device entry list.
+       * @type {!Array<UsbDeviceEntry>}
+       */
+      var deviceList = [
+        {
+          embeddingOrigin: 'device-1-embedding-origin',
+          object: {
+            name: 'device-1',
+            "product-id": 1,
+            "serial-number": "device-1-sn",
+            "vendor-id": 1
+          },
+          objectName: 'device-1',
+          origin: 'device-1-origin',
+          setting: 'device-1-settings',
+          source: 'device-1-source'
+        }, {
+          embeddingOrigin: 'device-2-embedding-origin',
+          object: {
+            name: 'device-2',
+            "product-id": 2,
+            "serial-number": "device-2-sn",
+            "vendor-id": 2
+          },
+          objectName: 'device-2',
+          origin: 'device-2-origin',
+          setting: 'device-2-settings',
+          source: 'device-2-source'
+        }
+      ];
+
+      // Import necessary html before running suite.
+      suiteSetup(function() {
+        return PolymerTest.importHtml(
+           'chrome://md-settings/site_settings/usb_devices.html');
+      });
+
+      setup(function() {
+        browserProxy = new TestSiteSettingsPrefsBrowserProxy();
+        settings.SiteSettingsPrefsBrowserProxyImpl.instance_ = browserProxy;
+      });
+
+      teardown(function() {
+        testElement.remove();
+        testElement = null;
+      });
+
+      /** @return {!Promise} */
+      function initPage() {
+        browserProxy.reset();
+        PolymerTest.clearBody();
+        testElement = document.createElement('usb-devices');
+        document.body.appendChild(testElement);
+        return browserProxy.whenCalled('fetchUsbDevices');

[dpapad, 2016/11/09 20:32:09]

Polymer.dom.flush() is called after every initPage() call, so let's move it here
instead.

return browserProxy.whenCalled('fetchUsbDevices').then(function() {
  Polymer.dom.flush();
});

[scottchen, 2016/11/10 06:08:21]

Done.

+      }
+
+      test('empty devices list', function() {
+        return initPage().then(function(){
+          Polymer.dom.flush();
+          var listItems = testElement.root.querySelectorAll('.list-item');
+          assertEquals(0, listItems.length);
+        });
+      });
+
+      test('non-empty device list', function() {
+        browserProxy.setUsbDevices(deviceList);
+
+        return initPage().then(function() {
+          Polymer.dom.flush();
+          var listItems = testElement.root.querySelectorAll('.list-item');
+          assertEquals(deviceList.length, listItems.length);
+        });
+      });
+
+
+      test('non-empty device list creates dialog menu correctly', function() {

[dpapad, 2016/11/09 20:32:09]

I don't think this test is adding much value. the cr-action-menu exists on the
page regardless of the |devices| array being not empty. If the dialog did not
exist at all the next test would fail anyway, so how about we remove this one?

[scottchen, 2016/11/10 06:08:21]

Done.

+        browserProxy.setUsbDevices(deviceList);
+
+        return initPage().then(function() {
+          Polymer.dom.flush();
+          var dialog = testElement.$$('dialog');

[dpapad, 2016/11/09 20:32:09]

Let's be specific about what kind of dialog we look for (here and elsewhere).

$$('dialog[is=cr-action-menu]')

[scottchen, 2016/11/10 06:08:21]

Done.

+
+          assertTrue(!!dialog);
+          assertFalse(dialog.hasAttribute('open'));
+
+          var removeButton = testElement.$["remove-button"];
+          assertTrue(!!removeButton);
+        });
+      });
+
+      test('non-empty device list has working menu buttons', function() {
+        browserProxy.setUsbDevices(deviceList);
+
+        return initPage().then(function() {
+          Polymer.dom.flush();
+
+          var menuButton = testElement.$$('paper-icon-button');
+          assertTrue(!!menuButton);
+
+          MockInteractions.tap(menuButton);
+
+          var dialog = testElement.$$('dialog');
+          assertTrue(dialog.hasAttribute('open'));

[dpapad, 2016/11/09 20:32:09]

Nit: I think this would also work

assertTrue(dialog.open)

[scottchen, 2016/11/10 06:08:21]

Done.

+        });
+      });
+
+      test('try removing items using remove button', function() {

[dpapad, 2016/11/09 20:32:09]

Given that we are not actually removing anything during the test (we simply
report back arguments from removeUsbDevice), I think testing two removals
instead of just one has no benefit. How about we simply test only one removal
and merge this test with the one above?

[scottchen, 2016/11/10 06:08:21]

- I think the previous test is to test the menu button's showMenu_ handler works
correctly, where as this one is testing the removeButton sending correct params,
so if for some reason showMenu_ is broken. By having two separate tests, I think
it'll be easier to tell what's wrong just by looking at which test failed.

- I tested tapping on the remove button for two different device, because I want
to make sure the showMenu_ handler is setting the right actionMenuModel_ and
onRemoveTap_ is deleting the right item both times. I could imagine incorrect
implementations of the code where onRemoveTap_ is just always trying to delete
the first device, or whichever device gets clicked first, etc.

Please let me know if those two points make sense.

[dpapad, 2016/11/10 17:35:09]

OK, that makes sense.
Makes sense, except from the fact that the testRemovalFlow() calls initPage(),
which means the 1st and 2nd removal are actually happening on different
<usb-devices> instances, and effectively we are testing a 1st removal twice.

+        browserProxy.setUsbDevices(deviceList);
+
+        var self = this;
+
+        /**
+         * A reusable function to test removing different devices.
+         * @param {!number} indexToRemove index of devices to be removed.
+         * @return {!Promise}
+         */
+        function testRemovalFlow(indexToRemove){

[dpapad, 2016/11/10 17:35:09]

Probably more readable if you move this helper function outside of
test(function() {...});

[scottchen, 2016/11/10 19:58:00]

Done.

+          return initPage().then(function() {
+            Polymer.dom.flush();
+
+            /**
+             * Test whether or not clicking remove-button sends the correct
+             * parameters to the browserProxy.removeUsbDevice() function.
+             */
+            var menuButton = testElement.root
+                .querySelectorAll('paper-icon-button')[indexToRemove];
+            var removeButton = testElement.$["remove-button"];
+            MockInteractions.tap(menuButton);
+            MockInteractions.tap(removeButton);
+            return browserProxy.whenCalled('removeUsbDevice');
+          }).then(function(args){
+            /**
+             * removeUsbDevice() is expected to be called with arguments as
+             * [origin, embeddingOrigin, object].
+             */
+            assertEquals(deviceList[indexToRemove].origin, args[0]);
+            assertEquals(deviceList[indexToRemove].embeddingOrigin, args[1]);
+            assertEquals(deviceList[indexToRemove].object, args[2]);
+          });
+        }
+
+        return testRemovalFlow(0).then(function(){
+          return testRemovalFlow(1);
+        });
+      });
+    });
+  }
+
+  return {
+    registerTests: registerTests,
+  };
+});