bluetooth: Fix BluetoothInternalsTest timeout issue

chrome/test/data/webui/bluetooth_internals_browsertest.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 Tests for chrome://bluetooth-internals
  */
 
 /** @const {string} Path to source root. */
 var ROOT_PATH = '../../../../';
@@ skipping 126 matching lines @@
           setTestAdapter: function(adapterInfo) {
             this.adapterInfo_ = adapterInfo;
           },
 
           setTestDevices: function(devices) {
             this.devices_ = devices;
           }
         };
 
         frameInterfaces.addInterfaceOverrideForTesting(
           adapter.AdapterFactory.name,

[dpapad, 2016/12/09 00:53:15]

Indentation off by 2.

[mbrunson, 2016/12/09 02:51:03]

Done.

           function(handle) {
             var stub = connection.bindHandleToStub(
                 handle, adapter.AdapterFactory);
+
             this.adapterFactory = new TestAdapterFactoryProxy();

[dpapad, 2016/12/09 00:53:15]

I am slightly confused here by the naming.

1) TestAdapterFactoryProxy contains a TestAdapter
2) TestAdapter contains a TestAdapterProxy

The two "Proxy" terms in 1 and 2 seem to have different semantics. Anyway,
probably nothing to do here, just my lack of understanding of the complex
hierarchy.

[mbrunson, 2016/12/09 02:51:03]

The "Proxy" on the end refers to the fact that that class is a subclass of
settings.TestBrowserProxy. TestAdapter is a subclass of
adapter.Adapter.stubClass so it doesn't have "Proxy" on the end. TestAdapter has
to be a subclass of a Mojo-generated stub class so I can get a Mojo Handle for
the object which causes this weird hierarchy.

+
+            this.adapterFactory.adapter.proxy.setTestDevices([
+              this.fakeDeviceInfo1(),
+              this.fakeDeviceInfo2(),
+            ]);
+            this.adapterFactory.adapter.proxy.setTestAdapter(
+                this.fakeAdapterInfo());
+
             bindings.StubBindings(stub).delegate = this.adapterFactory;
 
             this.setupResolver.resolve();

[dpapad, 2016/12/08 22:36:15]

Instead of calling setTestDevices(), setTestAdapter() and resolve() here, how
about doing all that in suiteSetup()? This is the approach I am following at
https://codereview.chromium.org/2563743002, which fixes a similar race condition
(and I think it would result in slightly simpler code). WDYT?

[mbrunson, 2016/12/09 00:08:22]

The order that my test has to execute is a little different from yours. Since
you only have one layer of Mojo interfaces, you can create browserProxy outside
of the setupFn. I have to create my proxy and its contained proxies inside the
setupFn and block the test until it's created.

If I moved the resolve and set calls, I would then have to block the  web page
code so I could set the test data before it's read. There's no way to do this
without putting more test code in the app or adding multiple Promise guards in
the test code. I figured this was the simpler alternative.

[dpapad, 2016/12/09 00:53:15]

Thanks for the explanation. I think I sort-of understand the differences between
with the PluginsTest. How about this?

1) Create the 3 objects (TestAdapterFactoryProxy, TestAdapter, TestAdapterProxy)
involved in your tests first, without any references between them.
2) Return setupFnResolver.promise from setupFn (to block the prod code from
running).
3) Inside your suiteSetup() do someting as follows

suiteSetup(function() {
  // Prepare testAdapterProxy.
  testAdapterProxy.setFakeStuff();
  testAdapterProxy.setMoreFakeStuff();

  // Prepare testAdapter.
  testAdapter.setProxy(testAdapterProxy);

  // Prepare testAdapterFactory.
  testAdapterFactory.setAdapter(testAdapter);

  // Allow code being tested to proceed.
  setupFnResolver.resolve();

  return Promise.all([...]);
});

Pardon me if that suggestion would not work either. Just thought that this makes
the complex hierarchy more understandable (by creating it in one place).

[mbrunson, 2016/12/09 02:51:03]

Where is testAdapter being created in this situation? It's required to be a
subclass of a Mojo-generated class which isn't available until setupFn runs.

May I suggest as a compromise, I move the setting of the test data and add a
setupFnResolver to the setupFn that gets resolved in suiteSetup().

That would give an execution order like this:
1) test is blocked on setupResolver (not setupFnResolver)
----(in setupFn)----
2) set the testAdapterFactory
3) resolve setupResolver, unblocking the test
4) setupFn blocks on setupFnResolver
----(in suiteSetup)----
5) set the test data since it now has access to testAdapterFactory
6) resolve setupFnResolver, unblocking setupFn
7) suiteSetup blocks on Promise.all
----(in setupFn)----
8) setupFn finishes, prod code runs
9) Promise.all is resolved
10) test finishes

I've made the change so you can see it. How does that sound?

[dpapad, 2016/12/09 03:44:45]

Actually patchset 5 looks simpler than latest version patch 6. Having two
PromiseResolvers is unnecessarily complicated. Here is my version,
https://codereview.chromium.org/2552883013 (tested locally and tests pass).

Basically here are the main differences
1) Move setupFn() call in the prod code to the entry point (DOMContentLoaded),
instead of having it at interfaces.js. This guarantees that nothing runs until
that setupFnResolver is resolved.
2) Split setupFn in the test code to two methods,
 - testSetup() which returns a Promise when all test all Mojo related classes
have been defined and instantiated (no need for a PromiseResolver). suiteSetup()
calls this method.
 - setupFn() resolved after fake data has been set, inside suiteSetup().

           }.bind(this));
+
       }.bind(this));
     }.bind(this);
   },
+
+  /**
+   * Returns a copy of fake adapter info object.
+   * @return {!Object}
+   */
+  fakeAdapterInfo: function() {

[dpapad, 2016/12/08 22:36:15]

Does this have to be a function? Can it just be a member variable inside the
constructor (near line 19)? Similar question for fakeDeviceInfo1,2,3 below.

[mbrunson, 2016/12/09 00:08:22]

The bluetooth-internals page is directly manipulating these objects so the
values tend to change throughout the tests. It saves some code in the tests to
have a function that vends copies of the same object rather than manually doing
it in the tests. It also more closely mimics the way objects are handed to the
web page code from Mojo as new objects.

[dpapad, 2016/12/09 00:53:15]

Makes sense.

+    return {
+      address: '02:1C:7E:6A:11:5A',
+      discoverable: false,
+      discovering: false,
+      initialized: true,
+      name: 'computer.example.com-0',
+      powered: true,
+      present: true,
+    };
+  },
+
+  /**
+   * Returns a copy of a fake device info object (variant 1).
+   * @return {!Object}
+   */
+  fakeDeviceInfo1: function() {
+    return {
+      address: "AA:AA:84:96:92:84",
+      name: "AAA",
+      name_for_display: "AAA",
+      rssi: {value: -40},
+      services: [],
+    };
+  },
+
+  /**
+   * Returns a copy of a fake device info object (variant 2).
+   * @return {!Object}
+   */
+  fakeDeviceInfo2: function() {
+    return {
+      address: "BB:BB:84:96:92:84",
+      name: "BBB",
+      name_for_display: "BBB",
+      rssi: null,
+      services: [],
+    };
+  },
+
+  /**
+   * Returns a copy of fake device info object. The returned device info lack
+   * rssi and services properties.
+   * @return {!Object}
+   */
+  fakeDeviceInfo3: function() {
+    return {
+      address: "CC:CC:84:96:92:84",
+      name: "CCC",
+      name_for_display: "CCC",
+    };
+  },
 };
 
-// Times out. See https://crbug.com/667970.
-TEST_F('BluetoothInternalsTest', 'DISABLED_Startup_BluetoothInternals',
-    function() {
-  var fakeAdapterInfo = {
-    address: '02:1C:7E:6A:11:5A',
-    discoverable: false,
-    discovering: false,
-    initialized: true,
-    name: 'computer.example.com-0',
-    powered: true,
-    present: true,
-  };
+TEST_F('BluetoothInternalsTest', 'Startup_BluetoothInternals', function() {
+  var adapterFactory = null;
+  var deviceTable = null;
 
-  var fakeDeviceInfo1 = {
-    address: "AA:AA:84:96:92:84",
-    name: "AAA",
-    name_for_display: "AAA",
-    rssi: {value: -40},
-    services: []
-  };
-
-  var fakeDeviceInfo2 = {
-    address: "BB:BB:84:96:92:84",
-    name: "BBB",
-    name_for_display: "BBB",
-    rssi: null,
-    services: []
-  };
-
-  var fakeDeviceInfo3 = {
-    address: "CC:CC:84:96:92:84",
-    name: "CCC",
-    name_for_display: "CCC",
-  };
-
-  var adapterFactory = null;
+  var fakeDeviceInfo1 = this.fakeDeviceInfo1;
+  var fakeDeviceInfo2 = this.fakeDeviceInfo2;
+  var fakeDeviceInfo3 = this.fakeDeviceInfo3;
 
   // Before tests are run, make sure setup completes.
   var setupPromise = this.setupResolver.promise.then(function() {
     adapterFactory = this.adapterFactory;
   }.bind(this));
 
 
   suite('BluetoothInternalsUITest', function() {
     var EXPECTED_DEVICES = 2;
 
     suiteSetup(function() {
       return setupPromise.then(function() {
-        adapterFactory.adapter.proxy.setTestDevices([
-          fakeDeviceInfo1,
-          fakeDeviceInfo2
-        ]);
-        adapterFactory.adapter.proxy.setTestAdapter(fakeAdapterInfo);
-
         return Promise.all([
           adapterFactory.whenCalled('getAdapter'),
           adapterFactory.adapter.proxy.whenCalled('getInfo'),
           adapterFactory.adapter.proxy.whenCalled('getDevices'),
           adapterFactory.adapter.proxy.whenCalled('setClient'),
         ]);
       });
     });
 
     setup(function() {
+      deviceTable = document.querySelector('#devices table');
       devices.splice(0, devices.length);
-      adapterBroker.adapterClient_.deviceAdded(fakeDeviceInfo1);
-      adapterBroker.adapterClient_.deviceAdded(fakeDeviceInfo2);
+      adapterBroker.adapterClient_.deviceAdded(fakeDeviceInfo1());
+      adapterBroker.adapterClient_.deviceAdded(fakeDeviceInfo2());
     });
 
     teardown(function() {
       adapterFactory.reset();
     });
 
     /**
      * Updates device info and verifies the contents of the device table.
      * @param {!device_collection.DeviceInfo} deviceInfo
      */
     function changeDevice(deviceInfo) {
-      var deviceRow = document.querySelector('#' + escapeDeviceAddress(
+      var deviceRow = deviceTable.querySelector('#' + escapeDeviceAddress(
           deviceInfo.address));
       var nameForDisplayColumn = deviceRow.children[0];
       var addressColumn = deviceRow.children[1];
       var rssiColumn = deviceRow.children[2];
       var servicesColumn = deviceRow.children[3];
 
       expectTrue(!!nameForDisplayColumn);
       expectTrue(!!addressColumn);
       expectTrue(!!rssiColumn);
       expectTrue(!!servicesColumn);
@@ skipping 23 matching lines @@
     function escapeDeviceAddress(address) {
       return address.replace(/:/g, '\\:');
     }
 
     /**
      * Expects whether device with |address| is removed.
      * @param {string} address
      * @param {boolean} expectRemoved
      */
     function expectDeviceRemoved(address, expectRemoved) {
-      var removedRow = document.querySelector(
+      var removedRow = deviceTable.querySelector(
           '#' + escapeDeviceAddress(address));
 
       expectEquals(expectRemoved, removedRow.classList.contains('removed'));
     }
 
     /**
      * Tests whether a device is added successfully and not duplicated.
      */
     test('DeviceAdded', function() {
-      var devices = document.querySelectorAll('#device-table tbody tr');
+      var devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
 
       // Copy device info because device collection will not copy this object.
-      var infoCopy = Object.assign({}, fakeDeviceInfo3);
+      var infoCopy = fakeDeviceInfo3();
       adapterBroker.adapterClient_.deviceAdded(infoCopy);
 
       // Same device shouldn't appear twice.
       adapterBroker.adapterClient_.deviceAdded(infoCopy);
 
-      devices = document.querySelectorAll('#device-table tbody tr');
+      devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES + 1, devices.length);
     });
 
     /**
      * Tests whether a device is marked properly as removed.
      */
     test('DeviceSetToRemoved', function() {
-      var devices = document.querySelectorAll('#device-table tbody tr');
+      var devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
-      adapterBroker.adapterClient_.deviceRemoved(fakeDeviceInfo2);
+
+      var fakeDevice = fakeDeviceInfo2();
+      adapterBroker.adapterClient_.deviceRemoved(fakeDevice);
 
       // The number of rows shouldn't change.
-      devices = document.querySelectorAll('#device-table tbody tr');
+      devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
 
-      expectDeviceRemoved(fakeDeviceInfo2.address, true);
+      expectDeviceRemoved(fakeDevice.address, true);
     });
 
     /**
      * Tests whether a changed device updates the device table properly.
      */
     test('DeviceChanged', function() {
-      var devices = document.querySelectorAll('#device-table tbody tr');
+      var devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
 
       // Copy device info because device collection will not copy this object.
-      var newDeviceInfo = Object.assign({}, fakeDeviceInfo1);
+      var newDeviceInfo = fakeDeviceInfo1();
       newDeviceInfo.name_for_display = 'DDDD';
       newDeviceInfo.rssi = { value: -20 };
       newDeviceInfo.services = ['service1', 'service2', 'service3'];
 
       changeDevice(newDeviceInfo);
     });
 
     /**
      * Tests the entire device cycle, added -> updated -> removed -> re-added.
      */
     test('DeviceUpdateCycle', function() {
-      var devices = document.querySelectorAll('#device-table tbody tr');
+      var devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
 
       // Copy device info because device collection will not copy this object.
-      var originalDeviceInfo = Object.assign({}, fakeDeviceInfo3);
+      var originalDeviceInfo = fakeDeviceInfo3();
       adapterBroker.adapterClient_.deviceAdded(originalDeviceInfo);
 
-      var newDeviceInfo = Object.assign({}, fakeDeviceInfo3);
+      var newDeviceInfo = fakeDeviceInfo3();
       newDeviceInfo.name_for_display = 'DDDD';
       newDeviceInfo.rssi = { value: -20 };
       newDeviceInfo.services = ['service1', 'service2', 'service3'];
 
       changeDevice(newDeviceInfo);
       changeDevice(originalDeviceInfo);
 
       adapterBroker.adapterClient_.deviceRemoved(originalDeviceInfo);
       expectDeviceRemoved(originalDeviceInfo.address, true);
 
       adapterBroker.adapterClient_.deviceAdded(originalDeviceInfo);
       expectDeviceRemoved(originalDeviceInfo.address, false);
     });
 
     test('DeviceAddedRssiCheck', function() {
-      var devices = document.querySelectorAll('#device-table tbody tr');
+      var devices = deviceTable.querySelectorAll('tbody tr');
       expectEquals(EXPECTED_DEVICES, devices.length);
 
       // Copy device info because device collection will not copy this object.
-      var newDeviceInfo = Object.assign({}, fakeDeviceInfo3);
+      var newDeviceInfo = fakeDeviceInfo3();
       adapterBroker.adapterClient_.deviceAdded(newDeviceInfo);
 
-      var deviceRow = document.querySelector('#' + escapeDeviceAddress(
+      var deviceRow = deviceTable.querySelector('#' + escapeDeviceAddress(
           newDeviceInfo.address));
       var rssiColumn = deviceRow.children[2];
       expectEquals('Unknown', rssiColumn.textContent);
 
-      var newDeviceInfo1 = Object.assign({}, fakeDeviceInfo3);
+      var newDeviceInfo1 = fakeDeviceInfo3();
       newDeviceInfo1.rssi = {value: -42};
       adapterBroker.adapterClient_.deviceChanged(newDeviceInfo1);
       expectEquals('-42', rssiColumn.textContent);
 
       // Device table should keep last valid rssi value.
-      var newDeviceInfo2 = Object.assign({}, fakeDeviceInfo3);
+      var newDeviceInfo2 = fakeDeviceInfo3();
       newDeviceInfo2.rssi = null;
       adapterBroker.adapterClient_.deviceChanged(newDeviceInfo2);
       expectEquals('-42', rssiColumn.textContent);
 
-      var newDeviceInfo3 = Object.assign({}, fakeDeviceInfo3);
+      var newDeviceInfo3 = fakeDeviceInfo3();
       newDeviceInfo3.rssi = {value: -17};
       adapterBroker.adapterClient_.deviceChanged(newDeviceInfo3);
       expectEquals('-17', rssiColumn.textContent);
     });
   });
 
   // Run all registered tests.
   mocha.run();
 });