474313 - Mozmill test for adding bookmark to bookmarks menu

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

Attachment: add bookmark to menu

I don't have permissions to check this test into our new repository. Further it would be nice to see if we can go with this coding style. 

Litmus test to be converted:
https://litmus.mozilla.org/show_test.cgi?id=5954

What do you think?

Comment 1

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

As what dietrich mentioned yesterday, he would also like to see functional checks if bookmarks were really created.

Mikeal, is it possible to step through the different menu hierarchies by accessing the id or the name? In my case I've to do a click on "Bookmarks" and on the created bookmark afterward, which was saved to the Bookmarks Menu with a special name.

Comment 2

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

Accessing the menu via name or id does work now. But the next issue is the waitForEval call.

Adam, it would be very helpful if you could give me the real expression string I have to use to get this call working. For now I always run into a timeout. :/

let panel = new elementslib.ID(controller.window.document,"editBookmarkPanelContent");
controller.waitForEval(panel.initialized == 'true', 10000, 100);

Comment 3

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

Attachment: Non-working waitForEval call

Adam, running this test you will see that you always run into the timeout, which is specified for waitForEval. There is no error/exception thrown. So I've no idea what's the reason.

Comment 4

[Adam Christian]

Unless it takes more then 10 seconds, to load I am seeing no attribute named 'initialized' being set on the node with ID editBookmarkPanel, can someone follow up on this and make sure the code to do this is in the nightlies? Is there a thread on this somewhere?

Comment 5

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

Adam, we probably have to reopen bug 473976, where this attribute was added. It really looks like that it doesn't work the way it should. Even it produces leaks as Marco told me on IRC. Lets use a workaround for now.

Comment 6

[Marco Bonardo [:mak]]

adam looks like the binding is lazy attached, when we try to read a property from it it gets initialized, still later the attribute was not found... not sure if that's due to a lazy inizialization of the overlay (sounds like that though)
hwv, what's the correct syntax to read an attribute from an element with waitForEval?

Comment 7

[Adam Christian]

if the element was given a new property called initialized when it was finished, 

let panel = new
elementslib.ID(controller.window.document,"editBookmarkPanelContent");
controller.waitForEval(panel.initialized == 'true', 10000, 100);

Would work, however it sounds like that has been backed out, I didn't get the feeling that a new patch was in the works either.. so considering that unless you are on a 486 this will only take a few seconds to load tops maybe we should just use a sleep.. unless you guys would like to pursue some developers to find a way to make this attribute actually happen.

Comment 8

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

(In reply to comment #7)
> let panel = new
> elementslib.ID(controller.window.document,"editBookmarkPanelContent");
> controller.waitForEval(panel.initialized == 'true', 10000, 100);
[...]
> you are on a 486 this will only take a few seconds to load tops maybe we should
> just use a sleep.. unless you guys would like to pursue some developers to find
> a way to make this attribute actually happen.

Adam, I've changed the timeout to 1000 which gives us a sleep of 1000ms. If it will ever work, we hopefully don't have to change the test.

Comment 9

[Adam Christian]

According to bug 473976 it looks like the change to make this possible was backed out, so no matter how long you make the timeout it will not work.

Comment 10

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

Attachment: Patch v2

This is an updated version of the last patch. I'm still using waitForEval but will silently run into a timeout for now. With 1000ms it should be enough to have an initialized editBookmarksPanel ready. Further I don't check the existence of the bookmark via the menu, but check it via the places API.

Comment 11

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

Attachment: Patch v2.1

Sorry, I've inserted the wrong license plate.

Comment 12

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

Attachment: Patch v2.2

Huh, I shouldn't use the license plate from the core js files while those run through the pre-compiler.

Comment 13

[cmtalbert]

Comment on attachment 359082 [details] [diff] [review]
Patch v2.2

It's a good patch.  I'm a bit more picky on this one because of the shared module in it.  Comments inline.

>diff --git a/firefox/bookmarks/test_addBookmarkToMenu.js b/firefox/bookmarks/test_addBookmarkToMenu.js
>new file mode 100644
>--- /dev/null
>+++ b/firefox/bookmarks/test_addBookmarkToMenu.js
>@@ -0,0 +1,87 @@
>+/* * ***** BEGIN LICENSE BLOCK *****
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
I'm pretty sure it's not developed by Netscape. ;-)  This should reference the mozilla.org code or the mozilla foundation.

>+var mozmill = {}; Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill);
>+var elementslib = {}; Components.utils.import('resource://mozmill/modules/elementslib.js', elementslib);
>+
>+// Include necessary modules
>+var RELATIVE_ROOT = '../../shared-modules';
>+var MODULE_REQUIRES = ['placesAPI'];
>+
>+
Don't need two blank lines here ^

>+var teardownModule = function(module) {
>+  module.places.restoreDefaultBookmarks();
>+}
>+
>+
two blank lines again ^^

>+  // Open URI and wait until it has been finished loading
>+  controller.open(uri.spec);
>+  controller.waitForPageLoad(controller.tabs.activeTab, 10000);
Is there a reason we need a 10 second timeout on this?  That seems too long.

>+  // Until bug 474486 isn't fixed we cannot check via the menu entry
>+  // controller.click(new elementslib.Elem(controller.menus.bookmarksMenu["Mozilla.org - Home of the Mozilla Project"])); 
Is there any point to check this in if we can't actually run the test?

>diff --git a/shared-modules/test_placesAPI.js b/shared-modules/test_placesAPI.js
>new file mode 100644
>--- /dev/null
>+++ b/shared-modules/test_placesAPI.js
>@@ -0,0 +1,88 @@
>+/* * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
Same comments pertain to this license.
>+
>+var MODULE_NAME = 'placesAPI';
>+
>+var Cc = Components.classes;
>+var Ci = Components.interfaces;
Don't use var for these, they should be 'const'.  If you can't load them as const
because someone else defined them, then just spell them out.
>+
>+/**
>+ * Create a new URI
>+ */
>+function createURI(aSpec) {
>+  var iosvc = Cc["@mozilla.org/network/io-service;1"].
>+              getService(Ci.nsIIOService);
>+
>+  return iosvc.newURI(aSpec, null, null);
>+}
Food for thought...if this is going to be a shared module for places, then it doesn't
seem to make sense to include "createURI" in a places library.  That seems like it
should be a more general purpose utility module that everyone will need (whether or not
they use the places API.).

Also, for forward compatibility, I think we want to allow callers to set the
aOriginCharset and aBaseURI on this call (the other two params for newURI).
Docs are here: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

>+/**
>+ * Restore the default bookmarks by overwriting all existing entries
>+ */
>+function restoreDefaultBookmarks() {
>+  // Get the default bookmarks.html
>+  var dirService = Cc["@mozilla.org/file/directory_service;1"].
>+                   getService(Ci.nsIProperties);
>+
>+  bookmarksFile = dirService.get("profDef", Ci.nsILocalFile);
>+  bookmarksFile.append("bookmarks.html");
>+
>+  // Run the import
>+  var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].
>+                 getService(Ci.nsIPlacesImportExportService);
>+  importer.importHTMLFromFile(bookmarksFile, true);
>+}
This function depends on bookmarks.html existing.  If that file doesn't exist,
how will it handle that case?  I believe as written the code will throw, and if
a module throws, does that mean that every testcase depending on this module will
automatically fail?

Comment 14

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

Attachment: Patch v3

(In reply to comment #13)
> (From update of attachment 359082 [details] [diff] [review])
> It's a good patch.  I'm a bit more picky on this one because of the shared
> module in it.  Comments inline.

That's fine. Let produce tests with high quality...

> >+ * The Initial Developer of the Original Code is
> >+ * Netscape Communications Corporation.
> >+ * Portions created by the Initial Developer are Copyright (C) 1998
> I'm pretty sure it's not developed by Netscape. ;-)  This should reference the
> mozilla.org code or the mozilla foundation.

No it isn't. As already said in the other mozmill test bug for the MSN website, I've updated all the license texts to match our needs.

> >+  // Open URI and wait until it has been finished loading
> >+  controller.open(uri.spec);
> >+  controller.waitForPageLoad(controller.tabs.activeTab, 10000);
> Is there a reason we need a 10 second timeout on this?  That seems too long.

I've used this timeout because we don't know anything about the underlying machine. I did a test-run with my vista VM image and it was a good value which doesn't made the whole test fail. Please keep in mind that we will already return earlier from that function, if the page was loaded faster. In any way we can also remove it completely to base on the default timeout value of 30s. That will be much safer.

> >+  // Until bug 474486 isn't fixed we cannot check via the menu entry
> >+  // controller.click(new elementslib.Elem(controller.menus.bookmarksMenu["Mozilla.org - Home of the Mozilla Project"])); 
> Is there any point to check this in if we can't actually run the test?

No, I've removed it and made a comment so we don't forget about this one.

> >+var Cc = Components.classes;
> >+var Ci = Components.interfaces;
> Don't use var for these, they should be 'const'.  If you can't load them as
> const
> because someone else defined them, then just spell them out.

Ok, changed that to const. But even if other tests or modules will define those constants they will be scoped by their name. Means we shouldn't worry about that for now.

> >+function createURI(aSpec) {
> >+  var iosvc = Cc["@mozilla.org/network/io-service;1"].
> >+              getService(Ci.nsIIOService);
> >+
> >+  return iosvc.newURI(aSpec, null, null);
> >+}
> Food for thought...if this is going to be a shared module for places, then it
> doesn't
> seem to make sense to include "createURI" in a places library.  That seems like
> it
> should be a more general purpose utility module that everyone will need
> (whether or not
> they use the places API.).

True. But mainly you will have to use it when you wanna interact with Places functions. I've separated it in its own module. I think it will be fine to call it utils in general.

> Also, for forward compatibility, I think we want to allow callers to set the
> aOriginCharset and aBaseURI on this call (the other two params for newURI).
> Docs are here:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

Indeed. Fixed.

> >+function restoreDefaultBookmarks() {
> >+  // Get the default bookmarks.html
> >+  var dirService = Cc["@mozilla.org/file/directory_service;1"].
> >+                   getService(Ci.nsIProperties);
> >+
> >+  bookmarksFile = dirService.get("profDef", Ci.nsILocalFile);
> >+  bookmarksFile.append("bookmarks.html");
[...]
> >+}
> This function depends on bookmarks.html existing.  If that file doesn't exist,
> how will it handle that case?  I believe as written the code will throw, and if
> a module throws, does that mean that every testcase depending on this module
> will
> automatically fail?

AFAIK MozMill test require a proper Firefox installation and uses a profile. If a user hasn't messed-up with his default settings, this file should always be present.

If we don't fail here other tests will probably fail when they expect a default set.

Comment 15

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

Comment on attachment 358287 [details]
Non-working waitForEval call

We know why we failed here. So marking this example obsolete.

Comment 16

[cmtalbert]

(In reply to comment #14)
> Created an attachment (id=359638) [details]
> Patch v3
> > >+  // Open URI and wait until it has been finished loading
> > >+  controller.open(uri.spec);
> > >+  controller.waitForPageLoad(controller.tabs.activeTab, 10000);
> > Is there a reason we need a 10 second timeout on this?  That seems too long.
> 
> I've used this timeout because we don't know anything about the underlying
> machine. I did a test-run with my vista VM image and it was a good value which
> doesn't made the whole test fail. Please keep in mind that we will already
> return earlier from that function, if the page was loaded faster. In any way we
> can also remove it completely to base on the default timeout value of 30s. That
> will be much safer.
Good plan.
> 
> > >+  // Until bug 474486 isn't fixed we cannot check via the menu entry
> > >+  // controller.click(new elementslib.Elem(controller.menus.bookmarksMenu["Mozilla.org - Home of the Mozilla Project"])); 
> > Is there any point to check this in if we can't actually run the test?
> 
> No, I've removed it and made a comment so we don't forget about this one.
I like the way you handled this.
> 
> > >+function createURI(aSpec) {
> > >+  var iosvc = Cc["@mozilla.org/network/io-service;1"].
> > >+              getService(Ci.nsIIOService);
> > >+
> > >+  return iosvc.newURI(aSpec, null, null);
> > >+}
> > Food for thought...if this is going to be a shared module for places, then it
> > doesn't
> > seem to make sense to include "createURI" in a places library.  That seems like
> > it
> > should be a more general purpose utility module that everyone will need
> > (whether or not
> > they use the places API.).
> 
> True. But mainly you will have to use it when you wanna interact with Places
> functions. I've separated it in its own module. I think it will be fine to call
> it utils in general.

Yeah, that's true, it's mainly a places thing, but there might be other uses for it down the road.  I like the new utils module.  I think it will be very useful.
> > >+}
> > This function depends on bookmarks.html existing.  If that file doesn't exist,
> > how will it handle that case?  I believe as written the code will throw, and if
> > a module throws, does that mean that every testcase depending on this module
> > will
> > automatically fail?
> 
> AFAIK MozMill test require a proper Firefox installation and uses a profile. If
> a user hasn't messed-up with his default settings, this file should always be
> present.
> 
> If we don't fail here other tests will probably fail when they expect a default
> set.
This is true. Let's leave it as is.

Thanks for the patch. r=ctalbert

Comment 17

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

Attachment: Patch v3.1 (checked-in)

As checked-in. I added a further comment to update the bookmarks name. The test has to be updated when bug 474667 is fixed.

Comment 18

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

http://hg.mozilla.org/qa/mozmill-tests/rev/e583d3079dad

I'll leave it open until the remaining issues are fixed:
* menu clicks will work
* bookmark name can be changed by controller.type
* editBookmarkPanel exposes initialized property

Comment 19

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 20

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

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 21

[u279076]

This is an old bug. Henrik, are the issues in comment 18 remaining to be resolved?

Comment 22

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

Someone should investigate if the dependencies as given in comment 18 have been solved. Unassigning myself.

Comment 23

[u279076]

Vlad, can you come up with a proof-of-concept test which checks for the dependencies in comment 18?

Thanks

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> http://hg.mozilla.org/qa/mozmill-tests/rev/e583d3079dad
> 
> I'll leave it open until the remaining issues are fixed:
> * menu clicks will work
Do we want to add a bookmark via Menu/Bookmarks -> Bookmark This Page entry?
> * bookmark name can be changed by controller.type
This is doable 
> * editBookmarkPanel exposes initialized property
I don't think we have this yet, but then again, I can probably be mistaking

Comment 25

[u279076]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #24)
> Do we want to add a bookmark via Menu/Bookmarks -> Bookmark This Page entry?

Yes.

> > * editBookmarkPanel exposes initialized property
> I don't think we have this yet, but then again, I can probably be mistaking

What is this needed for specifically?

Comment 26

[Balazs Juhasz (:balazs)]

Issues mentioned in comment 18 have been solved.

* bookmark name can be changed by controller.type
* editBookmarkPanel exposes initialized property
These two issues are already fixed in the test also.

* menu clicks will work
This is still referenced inside the test as bug 474486, but the referenced bug has been already solved.
Clicking via menu now is possible:
  controller.click(new elementslib.Elem(controller.menus.bookmarksMenu["Mozilla"]));

I will create a patch which will include this new click-method.

Comment 27

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

That's great to hear and good that we can finish off this bug soon. Thanks Balazs!

Comment 28

[Balazs Juhasz (:balazs)]

Addig the following line
  controller.click(new elementslib.Elem(controller.menus.bookmarksMenu["Mozilla"]));
results in error:
  "could not find element Elem instance."

Since controllers.menus is deprecated, I tried with solution:
  controller.mainMenu.click("[label='Mozilla']");
which resulted in:
  "Menu entry '[label='Mozilla']' not found."

Finally, I tried to add:
  controller.mainMenu.click("#bookmarksMenu");
  controller.mainMenu.click("[label='Mozilla']");
which works, however we should avoid clicking selecting Bookmarks menu.

In bug 484486 it is mentioned that dynamic menu entries are handled by fakeOpenPopup event, but I could not get it working.

Comment 29

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

I think you wanted to reference bug 474486. But I'm not sure as of now why it doesn't work. It might be that we are missing to call fakeOpenPopup for top-level menu element?

Comment 30

[Balazs Juhasz (:balazs)]

Sorry, yes I wanted to reference bug 474486.

I tried to force fakeOpenPopup by calling the following command before clicking, but still not works:
  controller.mainMenu.open("#main-menubar");

After some debugging with DOM inspector, if found that popup.allowevents changed to popup.Allowevents, so the following line gives undefined:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L295

I tried with the corrected property name also, but fakeOpenPopup still not called because popup.Allowevents is always set to "false".

Comment 31

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

Yes, that might not work. The property we are looking for here is not allowevents nor Allowevents, but allowEvents. It seems to be a wrapper to get and set the value of the allowevents attribute:

https://developer.mozilla.org/en-US/docs/XUL/Property/allowEvents

Does that help? Or do we still not populate the bookmarks menu correctly? Above you also referenced Mozmill 1.5, but does it also happen with 2.0? Version 1.5 is literally dead now.

Comment 32

[Balazs Juhasz (:balazs)]

Yes I've tried exactly with allowEvents and not Allowevents (just wrote it wrong here in comment).
But popup.allowEvents always has a "false" value, so fakeOpenPopup never called.

Same issues with Mozmill 2 master:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L224

Comment 33

[Balazs Juhasz (:balazs)]

Attachment: Check new bookmark via menu-click - v1

For this patch to work mozmill version >= 2.1 is neccessary.

Notes:
During test runs sometimes I got failed results in Nightly build. (28.0a1, 29-10-2013). The root cause everytime is that the page is not renamed to "Mozilla" before bookmarking, but the page name is combined with "Mozilla" ("MozillaMozilla Contribute").

Comment 34

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

Comment on attachment 824276 [details] [diff] [review]
Check new bookmark via menu-click - v1

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

Sorry for the bit of delay but given that we cannot land the patch yet, I pushed it back a bit. Some minutes ago I cherry-picked the fix from bug 583379 for hotfix-2.0, so with the upcoming release of Mozmill 2.0.1, we can land it once it's live on production.

But lets do a proper review now so that we are ready and only have to land the patch. Please see my comments inline.

::: tests/functional/testBookmarks/testAddBookmarkToMenu.js
@@ +48,5 @@
>    controller.click(doneButton);
>  
> +  // Open a new tab to check if new bookmark is working
> +  tabBrowser.openTab("menu");
> +  controller.waitForPageLoad();

Why do we have to open a new tab for this test? It would be enough if we load 'about:blank' in the current tab before clicking on the bookmark entry. We would have the same result.

Comment 35

[Balazs Juhasz (:balazs)]

Attachment: Check new bookmark via menu-click - v2

Good to hear! Testing in new tab is not needed, so I updated the patch to use the recommended 'about:blank' instead.

However, I still have some failed testruns as mentioned in my previous comment.
It looks like a timing issue. If I try to add controller.sleep(10) before typing the name of the bookmark, it is Passed every time.

I think we have this issue also with the actual test. It just not pop-ups because we are not checking the bookmark's name with the actual test (only the URL).

Comment 36

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

Comment on attachment 831689 [details] [diff] [review]
Check new bookmark via menu-click - v2

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

All looks fine except for the failure cases you are mentioning. If we want to get in this patch, the test has to pass at every time. If you really have to add a sleep call of 10ms for it to work, we most likely are missing to wait for an event when the dialog is ready. We might be able to get this information by asking Marco Bonardo (mak77).

Comment 37

[Balazs Juhasz (:balazs)]

Hello Marco,

After opening "Edit bookmark panel" we wait for overlay to appear (this._controller.window.top.StarUI._overlayLoaded) and then we want to type the name of the new bookmark ("Mozmill"). Unfortunately, it looks like _overlayLoaded is sets before the textbox.text for bookmark's name is selected, thus we sometimes gets a concatenated name of "Mozilla"+"Page title" instead of just "Mozilla".

Do you know if there is an event which we could wait to be sure that the text is selected?

Comment 38

[Marco Bonardo [:mak]]

you may try to run after popupshown, see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#204

Comment 39

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

The Mozmill project is dead. So we should check what could be remain useful for such a test. 

Marco, what do you think? I could imagine to test the keyboard shortcut, which would also cover other locales. Do you have anything else?

Comment 40

[Marco Bonardo [:mak]]

that sounds OK to me, we already have tests for the dialog internals, and in general we don't test much keyboard shortcuts.
I'd not add further stuff for now.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.