952125 - Master Dialog Modal is hanging in /restartTests\testPreferences_masterPassword\test2.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

With bug 951628 and bug 950003 landed the Master Password Modal is hanging in /restartTests\testPreferences_masterPassword\test2.js.
As a workaround for this I focused the hiddenDOMWindow at the beginning of callbackHandler and blur it at the finnal:

http://hg.mozilla.org/qa/mozmill-tests/file/39901d638d4a/firefox/tests/functional/restartTests/testPreferences_masterPassword/test2.js#l97
>var checkMasterHandler = function(controller) {
>  Services.appShell.hiddenDOMWindow.focus();

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

This failed due to fact that after we save a password on a page, restart the browser and navigate to the same page we will be prompted to give the master password.
Doing so at the beginning of the test2 the modal rise, and as it will steal focus and go unhandled without to be closed will determine the test to hang(bug 951628).

As all of this is expected the best we can do here is to navigate to a diffrent page in tearDown in test1 after we save the password.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Here is the report:
http://mozmill-crowd.blargon7.com/#/functional/report/b646dc9797659302414b7b8a9d23138b

Comment 3

[Andrei Eftimie]

Comment on attachment 8350119 [details] [diff] [review]
patch v1.0

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

Great find!

We never really handled that dialog before.
This makes the test more robust.

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/49d403ae6c90 (default)

Comment 4

[Andrei Eftimie]

We'll want to backport this.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8350119 [details] [diff] [review]
patch v1.0

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

::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js
@@ +24,5 @@
>    aModule.tabBrowser.closeAllTabs();
>  }
>  
>  var teardownModule = function(aModule) {
> +  controller.open("about:newtab");

Why can't we handle the modal dialog instead after the restart? That's how it should work and tested. The code here I see only as a workaround.

Comment 6

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Why can't we handle the modal dialog instead after the restart? That's how
> it should work and tested. The code here I see only as a workaround.

We could also do that and it should also work.

But the way the test is written, this modal dialog is not intended. We do not expect a modal dialog be shown right after a restart. We only work on the Panel in test2, we shouldn't also have to handle the page again.

For clarity we chose to properly clean up instead of handling unexpected cases.

If you feel strongly about this, please let me know and we'll change this to handle the modal dialog instead.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #6)
> But the way the test is written, this modal dialog is not intended. We do
> not expect a modal dialog be shown right after a restart. We only work on
> the Panel in test2, we shouldn't also have to handle the page again.

Why is this modal dialog not intended to be shown? If a password has been stored for a page, it will be used the next time the browser opens. And if a master password is set it should be asked. We don't have the Litmus test anymore but I strongly feel that with this little change in test2.js we can improve the testcoverage a lot and make sure that we do not regress this feature. Keep in mind that this test will only work with Mozmill due to the restart, so it's not covered somewhere else yet. So working around it it's a step back.

> For clarity we chose to properly clean up instead of handling unexpected
> cases.

We should do this in the teardown method of test2.js, so that test3.js doesn't bring up the dialog again.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

If I try to handle the modal in the second test it works only with mozmill 2.0, but not with 1.5. Because 1.5 is quite slower so the modal is shown before we have time to set the handler.

Also we only have to type the master password only once in a session, and we do so in test2. If we want to cover this in our test I would suggest that at the end of the test2 we navigate back to log-in page and check that the password was stored, which at this point the master password has already been given.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In that case we might want to file a new bug where we could inject another test module with only that specific check. We can work on it once 2.0 has been proven as stable over the next couple of days.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So lets get this patch backported and closed in favor of a follow-up bug.

Comment 11

[Andrei Eftimie]

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/7a43c89d6ab7 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/a0c458f1d01f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/7ddc30a9386e (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/9e32a8f0447d (mozilla-esr24)