Fix CrSettingsLanguagesPageTest dialog flake

chrome/test/data/webui/settings/languages_page_tests.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.
 
 cr.define('languages_page_tests', function() {
   /** @enum {string} */
   const TestNames = {
     AddLanguagesDialog: 'add languages dialog',
     LanguageMenu: 'language menu',
     InputMethods: 'input methods',
@@ skipping 59 matching lines @@
 
     teardown(function() {
       PolymerTest.clearBody();
     });
 
     suite(TestNames.AddLanguagesDialog, function() {
       var dialog;
       var dialogItems;
       var cancelButton;
       var actionButton;
+      var dialogClosedResolver;
+      var dialogClosedObserver;
+
+      // Resolves the PromiseResolver if the mutation includes removal of the
+      // settings-add-languages-dialog.
+      // TODO(michaelpg): Extract into a common method similar to
+      // test_util.whenAttributeIs for use elsewhere.
+      var onMutation = function(mutations, observer) {

[dpapad, 2017/05/16 00:19:43]

Is this actually called with multiple mutations? Can we simplify as follows and
remove the call to Array#some()?

assertEquals(1, mutations.length);
assertEquals('childList', mutations[0].type);
...

[michaelpg, 2017/05/16 19:57:39]

It's called a number of times, sometimes with several removed #text Nodes,
sometimes with the dialog. It's probably not feasible to hard-code the exact
sequence of changes to expect, and if we did I'd expect our tests to break over
time.

+        if (mutations.some(function(mutation) {
+          return mutation.type == 'childList' &&
+              Array.from(mutation.removedNodes).includes(dialog);
+        })) {
+          // Sanity check: the dialog should no longer be in the DOM.
+          assertEquals(null, languagesPage.$$('settings-add-languages-dialog'));
+          observer.disconnect();
+          assertTrue(!!dialogClosedResolver);
+          dialogClosedResolver.resolve();
+        }
+      };
 
       setup(function(done) {
         var addLanguagesButton =
             languagesCollapse.querySelector('#addLanguages');
         MockInteractions.tap(addLanguagesButton);
 
         // The page stamps the dialog, registers listeners, and populates the
         // iron-list asynchronously at microtask timing, so wait for a new task.
         setTimeout(function() {
           dialog = languagesPage.$$('settings-add-languages-dialog');
           assertTrue(!!dialog);
 
+          // Observe the removal of the dialog via MutationObserver since the
+          // HTMLDialogElement 'close' event fires at an unpredictable time.
+          dialogClosedResolver = new PromiseResolver;

[dpapad, 2017/05/16 00:19:43]

Nit (optional): new PromiseResolver()? It seems that always using the
parentheses is more prevalent in our code
(https://cs.chromium.org/search/?q=%22new+PromiseResolver%22+lang:js&type=cs). I
guess we can add an ESLint check for this if we care, once ESLint is in the
toolchain (hopefully soon).

[michaelpg, 2017/05/16 19:57:39]

Done. but I'm wary of forcing style opinions with ESLint that aren't part of our
official style guidelines :)

+          dialogClosedObserver = new MutationObserver(onMutation);
+          dialogClosedObserver.observe(languagesPage.root, {childList: true});
+
           actionButton = assert(dialog.$$('.action-button'));
           cancelButton = assert(dialog.$$('.cancel-button'));
 
           // The fixed-height dialog's iron-list should stamp far fewer than
           // 50 items.
           dialogItems =
               dialog.$.dialog.querySelectorAll('.list-item:not([hidden])');
           assertGT(dialogItems.length, 1);
           assertLT(dialogItems.length, 50);
 
           // No languages have been checked, so the action button is disabled.
           assertTrue(actionButton.disabled);
           assertFalse(cancelButton.disabled);
 
           done();
         });
       });
 
-      // After every test, check that the dialog is removed from the DOM.
       teardown(function() {
-        Polymer.dom.flush();
-        assertEquals(null, languagesPage.$$('settings-add-languages-dialog'));
+        dialogClosedObserver.disconnect();
       });
 
       test('cancel', function() {
         // Canceling the dialog should close and remove it.
         MockInteractions.tap(cancelButton);
+
+        return dialogClosedResolver.promise;
       });
 
-      test('add languages and cancel', function(done) {
+      test('add languages and cancel', function() {
         // Check some languages.
         MockInteractions.tap(dialogItems[1]);  // en-CA.
         MockInteractions.tap(dialogItems[2]);  // tk.
 
         // Canceling the dialog should close and remove it without enabling
-        // the checked languages. A small timeout lets us check this.
+        // the checked languages.
         MockInteractions.tap(cancelButton);
-        setTimeout(function() {
+        return dialogClosedResolver.promise.then(function() {
           assertEquals(initialLanguages,
                        languageHelper.getPref(languagesPref).value);
-          done();
-        }, 100);
+        });
       });
 
       test('add languages and confirm', function() {
         // No languages have been checked, so the action button is inert.
         MockInteractions.tap(actionButton);
         Polymer.dom.flush();
         assertEquals(dialog, languagesPage.$$('settings-add-languages-dialog'));
 
         // Check and uncheck one language.
         MockInteractions.tap(dialogItems[0]);
         assertFalse(actionButton.disabled);
         MockInteractions.tap(dialogItems[0]);
         assertTrue(actionButton.disabled);
 
         // Check multiple languages.
         MockInteractions.tap(dialogItems[0]);  // en.
         MockInteractions.tap(dialogItems[2]);  // tk.
         assertFalse(actionButton.disabled);
 
         // The action button should close and remove the dialog, enabling the
         // checked languages.
         MockInteractions.tap(actionButton);
 
         assertEquals(
             initialLanguages + ',en,tk',
             languageHelper.getPref(languagesPref).value);
+
+        return dialogClosedResolver.promise;
       });
     });
 
     suite(TestNames.LanguageMenu, function() {
       /*
        * Finds, asserts and returns the menu item for the given i18n key.
        * @param {string} i18nKey Name of the i18n string for the item's text.
        * @return {!HTMLElement} Menu item.
        */
       function getMenuItem(i18nKey) {
@@ skipping 216 matching lines @@
         // Now the secondary row is empty, so it shouldn't be shown.
         assertFalse(triggerRow.classList.contains('two-line'));
         assertEquals(
             0, triggerRow.querySelector('.secondary').textContent.length);
       }
     });
   });
 
   return {TestNames: TestNames};
 });