1108077 - Update tests to use Menu API for handling context menus

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Barbara Miller (:galgeek)]

For example, in firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js:

    controller.rightClick(testFolder);
    controller.click(openAllInTabs);
    controller.waitForPageLoad();

sometimes fails with message: "controller.waitForPageLoad(URI=about:newtab, readyState=complete) because we fail to click the element.

Replace with controller.getMenu(id)

Comment 1

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

rightClick() with a following click() is not that safe to use, even it doesn't close the context menu if something fails. All this gets done by Mozmill when using the Menu API. So we should indeed convert all of those instances to use the API.

Bumping to P2 given that we know the cause now and it could happen at any time.

Thank you Barbara for working on it!

Comment 2

[Barbara Miller (:galgeek)]

Attachment: Bug1108077-WIP.patch

This patch updates files in mozmill-tests using the .rightClick method. Not all of these use .click immediately after .rightClick, and so far I have not succeeded in updating all that do use .click right after .rightClick to use the Menu API--I've rewritten .click as .waitThenClick for now in those. This patch includes my unsuccessful menu api code, commented out. 

details...

firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js is the test we updated Friday.

firefox/tests/functional/testAwesomeBar/testPasteLocationBar.js...
the context menu created with .rightClick on line 57 uses an anonid--
see http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#452
I get "Menu element 'undefined' not found." when I test my version using the menu api. 
    
firefox/lib/tabs.js now uses the menu api, but I'm not positive testruns reach that code to test it.

firefox/lib/toolbars.js--there's no .click following .rightClick, rather var toggle gets created, then toggle.mouseDown; toggle.mouseUp; added toggle.waitForElement(); just after var toggle gets created.

firefox/tests/functional/restartTests/testDefaultBookmarks/test1.js--similar to toolbars.js.

firefox/tests/functional/testSearch/testSearchSelection.js--no .click immediately after .rightClick & already using controller.waitforElement();

firefox/tests/functional/testTabbedBrowsing/testOpenInForeground.js--errors trying to use menu api similar to above, instead rewriting .click as .waitThenClick

firefox/tests/functional/testTechnicalTools/testAccessPageInfoDialog.js is updated.

firefox/tests/l10n/testAccessKeys/test2.js is unchanged; there's no .click following .rightClick; note context menu hooks and setupModule declarations

Comment 3

[Barbara Miller (:galgeek)]

Attachment: Updating tests to handle context menus reliably

I looked at these updates again today, hoping to get more tests running with menu API code. Instead, I discovered that yesterday's tabs.js updates were failing and noted additional details about these tests, including the ID of the context menu used by the ".rightClick then .click" tests. 

firefox/tests/functional/testTabbedBrowsing/testOpenInForeground.js may be particularly worth checking; it includes commented-out code to use the menu API and to test the update to tabs.js. I wonder if the menu API code might be failing on OS X but not other platforms.

details
=======

updated using menu API!

* firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js (the test we updated Friday, 12/5) (#placesContext)
* firefox/tests/functional/testTechnicalTools/testAccessPageInfoDialog.js (#contentAreaContextMenu)

updated by rewriting .click as .waitThenClick

my menu API code fails with “Menu element ‘undefined; not found.”—because the menu element in these isn’t available before the context menu displays? I wonder if it succeeds on other platforms.

* firefox/tests/functional/testAwesomeBar/testPasteLocationBar.js (the context menu created with .rightClick on line 57 uses an anonid — see http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#452)
* firefox/lib/tabs.js — tested with alternate code in testOpenInForeground.js (#contentAreaContextMenu)
* firefox/tests/functional/testTabbedBrowsing/testOpenInForeground.js—contains alternate code to test tabs.js updates; this test also fails sometimes using menu API code with "Element.waitForElement(): Element 'ID: id' has been found" (Bug 1055442)((#contentAreaContextMenu)

other

* firefox/tests/functional/testSearch/testSearchSelection.js is unchanged. There’s no .click immediately after .rightClick & already using controller.waitforElement();
note: this test is failing reliably for me running individually via mozmill -t with "Search engines drop down open state has been changed” as in Bug 951212 (#contentAreaContextMenu)
* firefox/tests/l10n/testAccessKeys/test2.js is unchanged; there's no .click following .rightClick;
note: this test includes context menu hooks as well as a context menu setupModule declaration (#contentAreaContextMenu)
* firefox/tests/remote/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test2.js — added "TODO: test should be updated per bug 1108077"; this test is failing, skipped, and blocked by another bug, so I won’t spend more time on it. (#contentAreaContextMenu)
* firefox/lib/toolbars.js — there's no .click following .rightClick, rather var toggle gets created, then toggle.mouseDown; toggle.mouseUp; added toggle.waitForElement(); just after var toggle gets created; or should this be left alone, or perhaps made another bug?
* firefox/tests/functional/restartTests/testDefaultBookmarks/test1.js — similar to toolbars.js.

Comment 4

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

(In reply to Barbara Miller (:galgeek) from comment #3)
> firefox/tests/functional/testTabbedBrowsing/testOpenInForeground.js may be
> particularly worth checking; it includes commented-out code to use the menu
> API and to test the update to tabs.js. I wonder if the menu API code might
> be failing on OS X but not other platforms.

I don't see commented out code for the Menu API usage. I only see rightClick, click, and closing the context menu in lines 57 to 60. Mind helping me what you mean here in detail?

> updated using menu API!
> 
> * firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js (the test we
> updated Friday, 12/5) (#placesContext)
> * firefox/tests/functional/testTechnicalTools/testAccessPageInfoDialog.js
> (#contentAreaContextMenu)
> 
> updated by rewriting .click as .waitThenClick
> 
> my menu API code fails with “Menu element ‘undefined; not found.”—because
> the menu element in these isn’t available before the context menu displays?
> I wonder if it succeeds on other platforms.
> 
> * firefox/tests/functional/testAwesomeBar/testPasteLocationBar.js (the
> context menu created with .rightClick on line 57 uses an anonid — see
> http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> textbox.xml#452)

I will comment in the review request.

> * firefox/tests/functional/testSearch/testSearchSelection.js is unchanged.
> There’s no .click immediately after .rightClick & already using
> controller.waitforElement();

I would suggest we leave special cases alone, so we can get this bug fixed sooner than later.

> * firefox/tests/l10n/testAccessKeys/test2.js is unchanged; there's no .click
> following .rightClick;
> note: this test includes context menu hooks as well as a context menu
> setupModule declaration (#contentAreaContextMenu)

l10n tests are also special. So just keep what is contained.

> *
> firefox/tests/remote/restartTests/
> testAddons_RestartlessExtensionWorksAfterRestart/test2.js — added "TODO:
> test should be updated per bug 1108077"; this test is failing, skipped, and
> blocked by another bug, so I won’t spend more time on it.
> (#contentAreaContextMenu)

No changes then.

> * firefox/lib/toolbars.js — there's no .click following .rightClick, rather
> var toggle gets created, then toggle.mouseDown; toggle.mouseUp; added
> toggle.waitForElement(); just after var toggle gets created; or should this
> be left alone, or perhaps made another bug?
> * firefox/tests/functional/restartTests/testDefaultBookmarks/test1.js —
> similar to toolbars.js.

No changes then.

I will also give code in-line comments.

Comment 5

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

Comment on attachment 8534770 [details] [diff] [review]
Updating tests to handle context menus reliably

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

::: firefox/lib/toolbars.js
@@ +1392,5 @@
>  
>      navbar.rightClick(navbar.getNode().boxObject.width / 2, 2);
>  
>      var toggle = this.getElement({type: 'toggle_PersonalToolbar'});
> +    toggle.waitForElement();

How is this change related to the context menu work?

::: firefox/tests/functional/restartTests/testDefaultBookmarks/test1.js
@@ +48,5 @@
>    controller.rightClick(navbar, navbar.getNode().boxObject.width / 2, 2);
>  
>    var toggle = new elementslib.ID(controller.window.document,
>                                    "toggle_PersonalToolbar");
> +  controller.waitForElement(toggle);

How is this change related to the context menu work?

::: firefox/tests/functional/testAwesomeBar/testPasteLocationBar.js
@@ +57,5 @@
>    controller.rightClick(input);
>    var contextMenuEntry = locationBar.getElement({type: "contextMenu_entry", subtype: "paste"});
> +  contextMenuEntry.waitThenClick();
> +  // menu API code
> +  //var contextMenu = controller.getMenu(locationBar.getElement({type: "contextMenu"}).id);

getMenu() expects a CSS selector as parameter. That's not what you are passing in here. For IDs it needs a leading #.

::: firefox/tests/functional/testTabbedBrowsing/testOpenInForeground.js
@@ +66,2 @@
>        controller.rightClick(currentLink);
> +      controller.waitThenClick(contextMenuItem);

Does using waitThenClick() in Mozmill directly fix your problem too?

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L171

If yes we should get this into the next release, and I need a bug to be filed.

Comment 6

[Barbara Miller (:galgeek)]

Attachment: minimal patch updating tests to use menu API

Thanks for your feedback, Henrik!

This patch includes updates only to the tests using .rightClick that succeed for me rewritten using the menu API. (My earlier comments and patches for this bug considered all tests that use .rightClick.)

I'm not sure that rewriting .click as .waitThenClick makes a difference for any of the other tests using .rightClick, and I'm not including those edits in this patch.

Comment 7

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

Comment on attachment 8537021 [details] [diff] [review]
minimal patch updating tests to use menu API

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

It would have been good to also fix testAddons_RestartlessExtensionWorksAfterRestart/test2.js, but lets leave it out for now. If necessary we can do it later.

Comment 8

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

https://hg.mozilla.org/qa/mozmill-tests/rev/5830c91319e7

Barbara, can you please check which other branches could also benefit from this change?

Comment 9

[Barbara Miller (:galgeek)]

The affected tests in mozilla-beta, mozilla-aurora, and mozilla-esr31 all pass for me with this patch applied.

But! all of the affected tests in these branches pass for me right now also *without* the patch applied.

I did a purge before each mozmill test.

Comment 10

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

For stability improvements we should get this backported, even they are working for you at the moment.

https://hg.mozilla.org/qa/mozmill-tests/rev/7e088128ab2d (aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/7d9506e3cdc9 (beta)

For the esr31 branch the patch does not apply. Can you please re-check that?

Comment 11

[Barbara Miller (:galgeek)]

Attachment: a patch for ESR

Here's a patch for ESR, applied against a fresh clone.

I apologize for the confusion.

Comment 12

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

Comment on attachment 8539368 [details] [diff] [review]
a patch for ESR

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

This applies fine. Regarding the commit message please keep the one we already had, so its consistent between the branches. There is no need to explicitly name "ESR" given that this implied by landing it on that branch.

Comment 13

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

https://hg.mozilla.org/qa/mozmill-tests/rev/9e2ae232144e (esr31)