MD Settings: Certificate manager, hooking up all dialogs.

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

Index: chrome/test/data/webui/settings/certificate_manager_page_test.js
diff --git a/chrome/test/data/webui/settings/certificate_manager_page_test.js b/chrome/test/data/webui/settings/certificate_manager_page_test.js
index b4644ecfa215f1c2f63fc2e34b2c88c7fd1f8b0b..50689e9124c023a24f7ff0e05365f23d2053cbe4 100644
--- a/chrome/test/data/webui/settings/certificate_manager_page_test.js
+++ b/chrome/test/data/webui/settings/certificate_manager_page_test.js
@@ -16,14 +16,20 @@ cr.define('certificate_manager_page', function() {
     settings.TestBrowserProxy.call(this, [
       'deleteCertificate',
       'editCaCertificateTrust',
+      'exportCertificate',
+      'exportPersonalCertificate',
       'exportPersonalCertificatePasswordSelected',
       'getCaCertificateTrust',
       'importPersonalCertificatePasswordSelected',
       'refreshCertificates',
+      'viewCertificate',
     ]);
 
     /** @private {!CaTrustInfo} */
     this.caTrustInfo_ = {ssl: true, email: true, objSign: true};
+
+    /** @private {?CertificatesError} */
+    this.certificatesError_ = null;
   };
 
   TestCertificatesBrowserProxy.prototype = {
@@ -47,33 +53,68 @@ cr.define('certificate_manager_page', function() {
       this.methodCalled('editCaCertificateTrust', {
         id: id, ssl: ssl, email: email, objSign: objSign,
       });
-      return Promise.resolve();
+      return this.fulfillRequest_();
+    },
+
+    /**
+     * Forces some of the browser proxy methods to start returning errors.
+     */
+    forceCertificatesError: function() {
+      this.certificatesError_ = /** @type {!CertificatesError} */ ({
+        title: 'DummyError', description: 'DummyDescription'
+      });
+    },
+
+    /**
+     * @return {!Promise} A promise that is resolved or rejected based on the
+     * value of |certificatesError_|.
+     * @private
+     */
+    fulfillRequest_: function() {
+      return this.certificatesError_ === null ?
+          Promise.resolve() : Promise.reject(this.certificatesError_);
     },
 
     /** @override */
     deleteCertificate: function(id) {
       this.methodCalled('deleteCertificate', id);
-      return Promise.resolve();
+      return this.fulfillRequest_();
     },
 
     /** @override */
     exportPersonalCertificatePasswordSelected: function(password) {
       this.resolverMap_.get(
           'exportPersonalCertificatePasswordSelected').resolve(password);
-      return Promise.resolve();
+      return this.fulfillRequest_();
     },
 
     /** @override */
     importPersonalCertificatePasswordSelected: function(password) {
       this.resolverMap_.get(
           'importPersonalCertificatePasswordSelected').resolve(password);
-      return Promise.resolve();
+      return this.fulfillRequest_();
     },
 
     /** @override */
     refreshCertificates: function() {
       this.methodCalled('refreshCertificates');
     },
+
+    /** @override */
+    viewCertificate: function(id) {
+      this.methodCalled('viewCertificate', id);
+    },
+
+    /** @override */
+    exportCertificate: function(id) {
+      this.methodCalled('exportCertificate', id);
+    },
+
+    /** @override */
+    exportPersonalCertificate: function(id) {
+      this.methodCalled('exportPersonalCertificate', id);
+      return Promise.resolve();
+    },
   };
 
   /** @return {!Certificate} */
@@ -111,6 +152,19 @@ cr.define('certificate_manager_page', function() {
     MockInteractions.keyEventOn(element, 'input', kSpaceBar);
   }
 
+  /**
+   * Converts an event occurrence to a promise.
+   * @param {string} eventType
+   * @param {!HTMLElement} target
+   * @return {!Promise} A promise firing once the event occurs.
+   */
+  function waitForEvent(eventType, target) {
+    return new Promise(function(resolve, reject) {
+      target.addEventListener(eventType, resolve);

[tommycli, 2016/03/23 19:28:05]

Is it a problem that we never remove the event listener?

[dpapad, 2016/03/24 22:06:11]

It is not a problem, for two reasons.
1) "target" is destroyed in teardown() of every test, so the listener is
destroyed along with it.
2) Even if the event fired twice (which does not happen), a promise can only be
fulfilled once, and silently ignores any subsequent calls to resolve(),
reject().

+    });
+  }
+

[tommycli, 2016/03/23 19:28:05]

nit: extra newline

[dpapad, 2016/03/24 22:06:11]

Removed.

+
   function registerCaTrustEditDialogTests() {
     /** @type {?SettingsCaTrustEditDialogElement} */
     var dialog = null;
@@ -138,7 +192,7 @@ cr.define('certificate_manager_page', function() {
 
       teardown(function() { dialog.remove(); });
 
-      test('Dialog', function() {
+      test('EditSuccess', function() {
         return browserProxy.whenCalled('getCaCertificateTrust').then(
             function(id) {
               assertEquals(model.id, id);
@@ -154,17 +208,31 @@ cr.define('certificate_manager_page', function() {
               // Simulate clicking 'OK'.
               MockInteractions.tap(dialog.$.ok);
 
-              return browserProxy.whenCalled('editCaCertificateTrust').then(
-                  function(args) {
-                    assertEquals(model.id, args.id);
-                    // Checking that the values sent to C++ are reflecting the
-                    // changes made by the user (toggling all checkboxes).
-                    assertEquals(caTrustInfo.ssl, !args.ssl);
-                    assertEquals(caTrustInfo.email, !args.email);
-                    assertEquals(caTrustInfo.objSign, !args.objSign);
-                    // Check that the dialog is closed.
-                    assertFalse(dialog.$.dialog.opened);
-                  });
+              return browserProxy.whenCalled('editCaCertificateTrust');

[tommycli, 2016/03/23 19:28:05]

Interesting. I assume you are just linearizing the then chain? 

[dpapad, 2016/03/24 22:06:11]

Yes, this is a typical case of chaining promises one after the other.

return promiseA.then(function(resultA) {
  // use resultA here
  return promiseB;
}).then(function(resultB) {
  // use resultB here
});

Instead of the equivalent, but less readable

return promiseA.then(function(resultA) {
  // use resultA here
  return promiseB.then(function(resultB) {
    // use resultB here
  });
});

+            }).then(
+            function(args) {
+              assertEquals(model.id, args.id);
+              // Checking that the values sent to C++ are reflecting the
+              // changes made by the user (toggling all checkboxes).
+              assertEquals(caTrustInfo.ssl, !args.ssl);
+              assertEquals(caTrustInfo.email, !args.email);
+              assertEquals(caTrustInfo.objSign, !args.objSign);
+              // Check that the dialog is closed.
+              assertFalse(dialog.$.dialog.opened);
+            });
+      });
+
+      test('EditError', function() {
+        browserProxy.forceCertificatesError();
+        var whenErrorEventFired = waitForEvent('certificates-error', dialog);
+
+        return browserProxy.whenCalled('getCaCertificateTrust').then(
+            function() {
+              MockInteractions.tap(dialog.$.ok);
+              return browserProxy.whenCalled('editCaCertificateTrust');
+            }).then(
+            function() {
+              return whenErrorEventFired;
             });
       });
     });
@@ -210,6 +278,20 @@ cr.define('certificate_manager_page', function() {
               assertFalse(dialog.$.dialog.opened);
             });
       });
+
+      test('DeleteError', function() {
+        browserProxy.forceCertificatesError();
+        var whenErrorEventFired = waitForEvent('certificates-error', dialog);
+
+        // Simulate clicking 'OK'.
+        MockInteractions.tap(dialog.$.ok);
+        return browserProxy.whenCalled('deleteCertificate').then(
+            function(id) {
+              assertEquals(model.id, id);
+              // Ensure that the 'error' event was fired.
+              return whenErrorEventFired;
+            });
+      });
     });
   }
 
@@ -223,6 +305,8 @@ cr.define('certificate_manager_page', function() {
     /** @type {!CertificateSubnode} */
     var model = createSampleCertificateSubnode();
 
+    var methodName = 'exportPersonalCertificatePasswordSelected';
+
     suite('CertificatePasswordEncryptionDialogTests', function() {
       setup(function() {
         browserProxy = new TestCertificatesBrowserProxy();
@@ -262,13 +346,31 @@ cr.define('certificate_manager_page', function() {
         // Simulate clicking 'OK'.
         MockInteractions.tap(dialog.$.ok);
 
-        var methodName = 'exportPersonalCertificatePasswordSelected';
         return browserProxy.whenCalled(methodName).then(function(password) {
           assertEquals(passwordInputElement.value, password);
           // Check that the dialog is closed.
           assertFalse(dialog.$.dialog.opened);
         });
       });
+
+      test('EncryptError', function() {
+        browserProxy.forceCertificatesError();
+
+        var passwordInputElements =
+            Polymer.dom(dialog.$.dialog).querySelectorAll('paper-input');
+        var passwordInputElement = passwordInputElements[0];
+        passwordInputElement.value = 'foopassword';
+        var confirmPasswordInputElement = passwordInputElements[1];
+        confirmPasswordInputElement.value = passwordInputElement.value;
+        triggerInputEvent(passwordInputElement);
+
+        var whenErrorEventFired = waitForEvent('certificates-error', dialog);

[tommycli, 2016/03/23 19:28:05]

It is a bit unfortunate that we have to attach this handler beforehand since it
makes it harder to read. I don't have a concrete suggestion for improving this
though.

Maybe declare this in setup()? Maybe call this errorEventPromise or something to
make clear it's a promise that's fulfilled only when there's an error?

[dpapad, 2016/03/24 22:06:11]

Done. I moved it to the setup() per your 1st suggestion, although now this
variable is alive in 3 different scopes instead of only being alive in the scope
it is being used.

Regarding your 2nd suggestion of renaming: The convention I have been using to
indicate that some variable is a promise is prefixing it with "when", instead of
suffixing it with "Promise". The former makes it read more naturally
"whenFoo.then(doSomething)" compared "fooPromise.then(doSomething)".

Therefore, I was hoping that seeing "whenErrorEventFired" already makes it clear
that it is a Promise firing once "error event fired".

[tommycli, 2016/03/24 22:34:14]

Ah given your point about the scope maybe the original location was better.

About the naming: I agree that the whenFoo.then(something) is very easy to
understand. However, when you just "return whenErrorEventFired;" it's less
obvious to me that it's a promise.

Maybe just comment above it "// A promise that's resolved when the event is
fired" ?

In any case this is just nitpicking, it's all good with me ultimately.

+        MockInteractions.tap(dialog.$.ok);
+
+        return browserProxy.whenCalled(methodName).then(function() {
+          return whenErrorEventFired;
+        });
+      });
     });
   }
 
@@ -279,6 +381,8 @@ cr.define('certificate_manager_page', function() {
     /** @type {?TestCertificatesBrowserProxy} */
     var browserProxy = null;
 
+    var methodName = 'importPersonalCertificatePasswordSelected';
+
     suite('CertificatePasswordDecryptionDialogTests', function() {
       setup(function() {
         browserProxy = new TestCertificatesBrowserProxy();
@@ -308,13 +412,138 @@ cr.define('certificate_manager_page', function() {
         // Simulate clicking 'OK'.
         MockInteractions.tap(dialog.$.ok);
 
-        var methodName = 'importPersonalCertificatePasswordSelected';
         return browserProxy.whenCalled(methodName).then(function(password) {
           assertEquals(passwordInputElement.value, password);
           // Check that the dialog is closed.
           assertFalse(dialog.$.dialog.opened);
         });
       });
+
+      test('DecryptError', function() {
+        browserProxy.forceCertificatesError();
+        // Simulate entering some password.
+        var passwordInputElement =
+            Polymer.dom(dialog.$.dialog).querySelector('paper-input');
+        passwordInputElement.value = 'foopassword';
+        triggerInputEvent(passwordInputElement);
+
+        var whenErrorEventFired = waitForEvent('certificates-error', dialog);
+        MockInteractions.tap(dialog.$.ok);
+        return browserProxy.whenCalled(methodName).then(function() {
+          return whenErrorEventFired;
+        });
+      });
+    });
+  }
+
+  function registerCertificateSubentryTests() {
+    var subentry = null;
+
+    /** @type {?TestCertificatesBrowserProxy} */
+    var browserProxy = null;
+
+    var waitForActionEvent = function() {

[tommycli, 2016/03/23 19:28:04]

Same comment as above, it would be easier for me to read if this was
actionEventPromise, so it was crystal clear it was a promise only fulfilled by
the action event.

[dpapad, 2016/03/24 22:06:11]

Added JSDoc for the local function which I think makes it crystal clear. See
previous comment regarding explanation of not using the "Promise" suffix.

+      return waitForEvent(settings.CertificateActionEvent, subentry);
+    };
+
+    suite('CertificateManagerPageTests', function() {
+      setup(function() {
+        browserProxy = new TestCertificatesBrowserProxy();
+        settings.CertificatesBrowserProxyImpl.instance_ = browserProxy;
+        PolymerTest.clearBody();
+        subentry = document.createElement('settings-certificate-subentry');
+        subentry.model = createSampleCertificateSubnode();
+        subentry.certificateType = settings.CertificateType.PERSONAL;
+        document.body.appendChild(subentry);
+
+        // Bring up the popup menu for the following tests to use.
+        MockInteractions.tap(subentry.$.dots);
+        Polymer.dom.flush();
+      });
+
+      teardown(function() { subentry.remove(); });
+
+      // Test case where 'View' option is tapped.
+      test('MenuOptions_View', function() {
+        var viewButton = subentry.shadowRoot.querySelector('#view');
+        MockInteractions.tap(viewButton);
+        return browserProxy.whenCalled('viewCertificate').then(function(id) {
+          assertEquals(subentry.model.id, id);
+        });
+      });
+
+      // Test that the 'Edit' option is only shown when appropriate and that
+      // once tapped the correct event is fired.
+      test('MenuOptions_Edit', function() {
+        var editButton = subentry.shadowRoot.querySelector('#edit');
+        // Should be disabled for any certificate type other than
+        // CertificateType.CA
+        assertTrue(editButton.hidden);
+        subentry.certificateType = settings.CertificateType.CA;
+        assertFalse(editButton.hidden);
+
+        var whenActionEventFired = waitForActionEvent();

[tommycli, 2016/03/23 19:28:04]

Same comment as above, I just found this required a bit of thinking to
understand.

[dpapad, 2016/03/24 22:06:11]

I am sensing that you are not very fond/comfortable with the Promise usage here
(because of repeated related comments). Would you prefer to stop converting
event-to-promise with waitForEvent() helper and do the following instead?

test('MenuOptions_Edit', function() {
  ...
  return new Promise(function(resolve, reject) {
    subentry.addEventListener(settings.CertificateActionEvent, function(event) {
      // do assertions here.
      resolve();
    });
  });
});

My intention with the event-to-promise waitForEvent() helper was to make things
more readable by reducing the repetition of "return new Promise(....)",  given
the fact that a Promise needs to be returned by test(function() {}) to indicate
an async test to Mocha.

[dpapad, 2016/03/24 22:11:17]

To be more precise I forgot some stuff in my snippet above

test('MenuOptions_Edit', function() {
  ...
  return new Promise(function(resolve, reject) {
    subentry.addEventListener(settings.CertificateActionEvent, function(event) {
      try {
        // do assertions here..
      } catch (e) {
        reject(e);
      }
      resolve();
    });
    MockInteractions.tap(editButton);
  });
});

I am trying to avoid repeating this verbose snippet over and over (by using the
event-to-promise helper).

[tommycli, 2016/03/24 22:34:14]

Hi Demetrios, Although the above would be easy to understand, I agree with you
that it's too verbose.

I think I'm just having trouble with the waitForActionEvent() method, since it's
not clear to me that it returns a Promise.

When it's used as waitForFoo().then(bar), then it's very clear.

But when it's just 'return waitForFoo()', it confuses me. This may just be
because I'm not as familiar with this usage pattern.

What if it was named eventToPromise? Ultimately these are all fine.

[dpapad, 2016/03/25 17:36:03]

Thanks for explaining the points of confusion Tommy, it helped me understand the
issue. So, I followed your suggestions
1) Renamed waitForEvent() -> eventToPromise()
2) Renamed waitForActionEvent() -> actionEventToPromise()
3) Renamed variable whenActionEventFired to waitForActionEvent and added a
comment mentioning that it is a promise in places where I am chaining with
'return waitForActionEvent;'

+        MockInteractions.tap(editButton);
+        return whenActionEventFired.then(function(event) {
+          var detail = /** @type {!CertificateActionEventDetail} */ (
+              event.detail);
+          assertEquals(settings.CertificateAction.EDIT, detail.action);
+          assertEquals(subentry.model.id, detail.subnode.id);
+          assertEquals(subentry.certificateType, detail.certificateType);
+        });
+      });
+
+      // Test that the 'Delete' option is only shown when appropriate and that
+      // once tapped the correct event is fired.
+      test('MenuOptions_Delete', function() {
+        subentry.set('model.readonly', true);
+
+        var deleteButton = subentry.shadowRoot.querySelector('#delete');
+        // Should be disabled when 'model.readonly' is true.
+        assertTrue(deleteButton.hidden);
+        subentry.set('model.readonly', false);
+        assertFalse(deleteButton.hidden);
+
+        var whenActionEventFired = waitForActionEvent();
+        MockInteractions.tap(deleteButton);
+        return whenActionEventFired.then(function(event) {
+          var detail = /** @type {!CertificateActionEventDetail} */ (
+              event.detail);
+          assertEquals(settings.CertificateAction.DELETE, detail.action);
+          assertEquals(subentry.model.id, detail.subnode.id);
+        });
+      });
+
+      // Test case of exporting a certificate that is not PERSONAL.
+      test('MenuOptions_Export', function() {
+        subentry.certificateType = settings.CertificateType.SERVER;
+        var exportButton = subentry.shadowRoot.querySelector('#export');
+        MockInteractions.tap(exportButton);
+        return browserProxy.whenCalled('exportCertificate').then(function(id) {
+          assertEquals(subentry.model.id, id);
+        });
+      });
+
+      // Test case of exporting a PERSONAL certificate.
+      test('MenuOptions_ExportPersonal', function() {
+        var whenActionEventFired = waitForActionEvent();
+
+        var exportButton = subentry.shadowRoot.querySelector('#export');
+        MockInteractions.tap(exportButton);
+        return browserProxy.whenCalled('exportPersonalCertificate').then(
+            function(id) {
+              assertEquals(subentry.model.id, id);
+              return whenActionEventFired;
+            }).then(
+            function(event) {
+              var detail = /** @type {!CertificateActionEventDetail} */ (
+                  event.detail);
+              assertEquals(
+                  settings.CertificateAction.EXPORT_PERSONAL,
+                  detail.action);
+              assertEquals(subentry.model.id, detail.subnode.id);
+            });
+      });
     });
   }
 
@@ -393,6 +622,58 @@ cr.define('certificate_manager_page', function() {
               assertCertificateListLength(CertificateCategoryIndex.OTHER, 0);
             });
       });
+
+      /**
+       * Dispatches a settings.CertificateActionEvent.
+       * @param {!settings.CertificateAction} action The type of action to
+       *     simulate.
+       */
+      function dispatchCertificateActionEvent(action) {
+        var eventDetail = /** @type {!CertificateActionEventDetail} */ ({
+          action: action,
+          subnode: createSampleCertificateSubnode(),
+          certificateType: settings.CertificateType.PERSONAL
+        });
+        page.dispatchEvent(new CustomEvent(
+            settings.CertificateActionEvent, {detail: eventDetail}));
+      }
+
+      /**
+       * Tests that a dialog opens as a response to a
+       * settings.CertificateActionEvent.
+       * @param {string} dialogTagName The type of dialog to test.
+       * @param {!settings.CertificateAction} action The action that is supposed
+       *     to trigger the dialog.
+       */
+      function testDialogOpensOnAction(dialogTagName, action) {
+        assertFalse(!!page.shadowRoot.querySelector(dialogTagName));
+        dispatchCertificateActionEvent(action);
+        Polymer.dom.flush();
+        assertTrue(!!page.shadowRoot.querySelector(dialogTagName));
+      }
+
+      test('OpensDialog_DeleteConfirmation', function() {
+        testDialogOpensOnAction(
+              'settings-certificate-delete-confirmation-dialog',
+              settings.CertificateAction.DELETE);
+      });
+
+      test('OpensDialog_PasswordEncryption', function() {
+        testDialogOpensOnAction(
+              'settings-certificate-password-encryption-dialog',
+              settings.CertificateAction.EXPORT_PERSONAL);
+      });
+
+      test('OpensDialog_PasswordDecryption', function() {
+        testDialogOpensOnAction(
+              'settings-certificate-password-decryption-dialog',
+              settings.CertificateAction.IMPORT_PERSONAL);
+      });
+
+      test('OpensDialog_CaTrustEdit', function() {
+        testDialogOpensOnAction(
+              'settings-ca-trust-edit-dialog', settings.CertificateAction.EDIT);
+      });
     });
   }
 
@@ -403,6 +684,7 @@ cr.define('certificate_manager_page', function() {
       registerPasswordEncryptDialogTests();
       registerPasswordDecryptDialogTests();
       registerPageTests();
+      registerCertificateSubentryTests();
     },
   };
 });