1430979 - Fix timeout in browser/extensions/formautofill/test/browser/ if fix of bug 1193394 is applied

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

The following 2 tests are timeout if bug 1193394 is fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6a98dcc99f5065d4f1ae9212eed3436ad617260&selectedJob=156776475

[browser_editAddressDialog.js]
TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editAddressDialog.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editAddressDialog.js | Found a unknown window with document URI: chrome://formautofill/content/editAddress.xhtml and title: Edit Address after previous test timed out -

[browser_editCreditCardDialog.js]
TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js | Found a unknown window with document URI: chrome://formautofill/content/editCreditCard.xhtml and title: Edit Credit Card after previous test timed out -

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

These tests looks problematic because they utilize EventUtils.synthesizeXxx without waiting for window focus (which might cause the key input or click invalid and failed to close the dialog properly before the frame tree is ready):
https://searchfox.org/mozilla-central/search?q=testDialog%7CFormReady&case=false&regexp=true&path=browser%2Fextensions%2Fformautofill
browser_editAddressDialog.js
browser_editCreditCardDialog.js
browser_manageAddressesDialog.js
browser_manageCreditCardsDialog.js

We should refine them using waitForFocus(), then, for example, the testDialog in head.js will be simlifed as following and we could remove the "FormReady" event from editDialog.js and manageDialog.js that is only used for testing:
> function testDialog(url, testFn, arg) {
>  return new Promise(resolve => {
>    let win = window.openDialog(url, null, null, arg);
>    waitForFocus(() => {
>      win.addEventListener("unload", () => {
>        ok(true, "Dialog is closed");
>        resolve();
>      }, {once: true});
>      testFn(win);
>    }, win);
>  });
>}

Comment 2

[Tooru Fujisawa [:arai]]

taking over.
I'll use the solution described in comment #1.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Use focus event instead of FormReady event in formautofill tests.

as explained in comment #1, the test can be written with focus event, instead of dispatching dedicated FormReady event.
I've replaced all of its consumers, and also removed dispatch.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8945635 [details] [diff] [review]
Use focus event instead of FormReady event in formautofill tests.

Review of attachment 8945635 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/formautofill/content/editDialog.js
@@ -35,5 @@
>      }
>      this.attachEventListeners();
> -    // For testing only: loadInitialValues for credit card is an async method, and tests
> -    // need to wait until the values have been filled before editing the fields.
> -    window.dispatchEvent(new CustomEvent("FormReady"));

Switching to only wait for focus means that the tests will no longer wait for the data to be loaded. The event listeners are only attached after that and some tests are testing the listeners. It seems like we still need this event to ensure the data is loaded before starting the test. It seems like we need to wait for both focus and `init` to finish (e.g. with FormReady). Is there something I'm missing?

I'm fine with the hunks that switch from the load event to waitForFocus though.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Wait for focus event in addition to FormReady event in formautofill tests.

Thank you for reviewing :)

Here's updated patch that:
  * adds waitForFocusAndFormReady function in head.js, that returns a promise that resolves after load event and focus
  * changes the code that waits for FormReady to use waitForFocusAndFormReady
  * also slightly changes the code structure to stop using event handler function, but use promise and await, to make it clearer the execution order

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8946022 [details] [diff] [review]
Wait for focus event in addition to FormReady event in formautofill tests.

Review of attachment 8946022 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

::: browser/extensions/formautofill/test/browser/head.js
@@ +331,5 @@
>  
> +async function waitForFocusAndFormReady(win) {
> +  return Promise.all([
> +    new Promise(resolve => waitForFocus(resolve, win)),
> +    BrowserTestUtils.waitForEvent(win, "FormReady")

Nit: include the trailing comma here so the blame is preserved if we add a third array entry

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7400dab78d5
Wait for focus event in addition to FormReady event in formautofill tests. r=MattN

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7400dab78d50ab6735e28d81cca4c1f5a0eba77
Bug 1430979 - Wait for focus event in addition to FormReady event in formautofill tests. r=MattN

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f7400dab78d5