Closed Bug 486490 Opened 11 years ago Closed 10 years ago

--browser-chrome Mochitests on Fennec [bookmarks]

Categories

(Firefox for Android Graveyard :: Bookmarks, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zadkiel.m, Assigned: hnsbstn)

Details

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121622 Ubuntu/8.10 (intrepid) Firefox/3.0.5
Build Identifier: 

--browser-chrome Mochitests on Fennec [bookmarks]

Reproducible: Always
checks to see if a bookmark is add
Attachment #370628 - Flags: review?(mark.finkle)
Comment on attachment 370628 [details] [diff] [review]
--browser-chrome Mochitest Fennec [bookmarks]

Good stuff, but maybe some more tests?

You have a title in your HTML, so that should be used by the "star" code as the name of the bookmark. Let's check that too.

title = PlacesUtils.bookmarks.getItemTitle(check_bookmark)

You could also test simple editing of bookmarks in this test file too.

Calling | window.BrowserUI.doCommand("cmd_star"); | a second time while the blank page is still loaded will display a bookmark editing sheet (try it manually). You can use | document.getElementById("id-of-placeitem") |  to get the special element we have to edit the bookmark. Change the name, save it and then test the title again.

example:
placeitem.stopEditing(true);

Will save the bookmark with any edits. Have a go and see what you can add.
Attachment #370628 - Flags: review?(mark.finkle) → review-
using document.getAnonymousElementByAttribute(this, "anonid" , "name"); to get the textbox element keep a getting null/undefined. 
ref: 
http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#467
Here are some additional tests I think we should consider automating as well:

** Add a bookmark and verifies URI exists (done)
** Verify panning in the bookmark page
** Test adding, editing, deleting tags
** Test adding, moving, editing names, deleting folders
** Test adding, editing url, editing title, deleting bookmarks
** Test adding, moving, editing, deleting subfolders
** Verify editing bookmark via star icon (instead of bookmark icon)
** Verify clicking on a bookmark loads the web page
** Verify softkeyboard launches for all text fields

Adding to this bug for now, we can split to additional bug(s) as necessary.
added edit bookmark title, edit bookmark url, add bookmark tags, edit bookmark tags, delete bookmark tags, (not sure how to delete bookmarks)
Attachment #373711 - Flags: review?(mark.finkle)
Comment on attachment 373711 [details] [diff] [review]
 --browser-chrome Mochitest Fennec [bookmarks]

You're getting the hang of it, but we need to adjust the tests a bit.

Currently you are testing that the values in the edit boxes have changed. However, what we really want to test is that the data in the bookmark is saved correctly to the Places system.

You can change this code to change the title, URL and tags in the edit boxes. Then force a "save". Then check the values using the Places API, like you did for the initial bookmark.

You can force a save by getting the "done" button and calling button.click() for example.
Attachment #373711 - Flags: review?(mark.finkle) → review-
Assignee: nobody → hnsbstn
The patch is for implementing tests to verify adding, editing, and removing bookmark.

The patch modifies the previously written tests such that tests are done by simulating user using the functionality through the UI, while verifications are done by looking at user input data from the Places system.
Attachment #370628 - Attachment is obsolete: true
Attachment #373711 - Attachment is obsolete: true
Comment on attachment 379883 [details] [diff] [review]
Patch that adds tests verifying adding, editing, and removing existing bookmark

>+gTests.push({
>+  desc: "Test editing URI of existing bookmark",
>+
>+  run: function() {
>+    chromeWindow.BrowserUI.showBookmarks();  
>+    chromeWindow.BookmarkList.toggleManage();

You use a setTimeout in the test below to wait for the buttons. Do you need it here?

>+gTests.push({
>+  desc: "Test editing title of existing bookmark",
>+  
>+  run: function() {
>+    var newtitle = "Changed Title";
>+    chromeWindow.BrowserUI.showBookmarks();  
>+    chromeWindow.BookmarkList.toggleManage();

Same question

>+gTests.push({
>+  desc: "Test removing existing bookmark",
>+  
>+  run: function() {
>+    chromeWindow.BrowserUI.showBookmarks();
>+    chromeWindow.BookmarkList.toggleManage();
>+    // Need to give a delay to wait for the close button image to be ready 
>+    setTimeout(this.verify, 100);

setTimeouts are fragile in tests, especially in mobile tests since the devices can run these tests _much_ slower than desktop.

Does using | executeSoon | work here too? That would be better imo. If it does not work, I'd bump up the timeout to 500 maybe.

>+// Helpers
>+function uri(spec) {
>+  return ioService.newURI(spec, null, null);
>+}
>\ No newline at end of file

Add a newline at the end here

Overall this is great. Just check the setTimeout issue and we'll get an r+ here and check it in.
Attachment #379883 - Flags: review-
1. No need to listen to any event for the buttons as there is no image to be loaded and button.click() works without errors.
2. Replaced setTimeout by using addEventListener and removeEventListener on the 'onload' event to make sure the image is loaded prior to clicking.
3. Added new line at the end.
Hans - Can you post a full patch again? attachment 380191 [details] [diff] [review] appears to be an interdiff.
Update to the previous changes as full patch

1. No need to listen to any event for the buttons as there is no image to be
loaded and button.click() works without errors.
2. Replaced setTimeout by using addEventListener and removeEventListener on the
'onload' event to make sure the image is loaded prior to clicking.
3. Added new line at the end.
Attachment #379883 - Attachment is obsolete: true
Attachment #380191 - Attachment is obsolete: true
Full patch for implementation of these test cases:

** Add a bookmark and verifies URI exists (done)
** Test adding, editing, deleting tags
** Test adding, moving, editing names, deleting folders
** Test adding, editing url, editing title, deleting bookmarks
** Test adding, moving, editing, deleting subfolders
** Verify editing bookmark via star icon (instead of bookmark icon)
** Verify clicking on a bookmark loads the web page
Attachment #380241 - Attachment is obsolete: true
An updated full patch of the tests to fix an error with running tests with fennec 1.9.2a1pre. Particularly replacing the use of PlacesUtils.bookmarks.bookmarksMenuFolder with PlacesUtils.bookmarks.unfiledBookmarksFolder to fix the error.
Attachment #384919 - Attachment is obsolete: true
verified recent fix works with fresh hg update from m-c/m-b.

Did a quick once over of the tests, these cover the scenarios outlined here to give us a great start on getting coverage.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 385250 [details] [diff] [review]
fix to previous patch

looking for review of this test patch.
Attachment #385250 - Flags: review?(mark.finkle)
Attachment #385250 - Flags: review?(mark.finkle) → review+
pushed: https://hg.mozilla.org/mobile-browser/rev/60487d979343
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.