Fix CrSettingsLanguagesPageTest dialog flake

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

Index: chrome/test/data/webui/settings/languages_page_tests.js
diff --git a/chrome/test/data/webui/settings/languages_page_tests.js b/chrome/test/data/webui/settings/languages_page_tests.js
index 8bccfad534280762cb873ba0299faaa60d106d9c..1372320310522ed727d7abe1b61945b7e0ddea6d 100644
--- a/chrome/test/data/webui/settings/languages_page_tests.js
+++ b/chrome/test/data/webui/settings/languages_page_tests.js
@@ -77,6 +77,25 @@ cr.define('languages_page_tests', function() {
       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 =
@@ -89,6 +108,12 @@ cr.define('languages_page_tests', 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'));
 
@@ -107,30 +132,29 @@ cr.define('languages_page_tests', function() {
         });
       });
 
-      // 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() {
@@ -157,6 +181,8 @@ cr.define('languages_page_tests', function() {
         assertEquals(
             initialLanguages + ',en,tk',
             languageHelper.getPref(languagesPref).value);
+
+        return dialogClosedResolver.promise;
       });
     });