585732 - Make Mozmill-test testAddBookmarkToMenu local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testBookmarks/testAddBookmarkToMenu.js
Test-page: Use any 1 page from test-files/layout/mozilla*

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1.0 - (default)

cleanup + converts test to local data

Couple other things for future fixing: 
1. This test is broken due to Omnijar packaging changes, and is looked at in bug 592411
2. The first comment XXX about a unique name, looks like the type does this
3. The second comment XXX, bookmarks go under unsorted, so it seems like the way it uses the API now would be better to use - do you still want this?

Henrik, feedback?

Comment 2

[u279076]

Comment on attachment 473557 [details] [diff] [review]
Patch v1.0 - (default)

Based on your last comment I think you wanted feedback from Henrik.  Switching the flag.

Comment 3

[Aaron Train [:aaronmt]]

Or just take the changes to local-data in this bug and address any other issues in another bug.

Comment 4

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

Comment on attachment 473557 [details] [diff] [review]
Patch v1.0 - (default)

>-const gDelay = 0;
>+const TIMEOUT = 2000;

Please make it 5000 as we have everywhere else.

> var testAddBookmarkToBookmarksMenu = function() {
>-  var uri = UtilsAPI.createURI("http://www.mozilla.org");
>-
>   // Fail if the URI is already bookmarked
>   controller.assertJS("subject.isBookmarked == false",
>-                      {isBookmarked: PlacesAPI.bookmarksService.isBookmarked(uri)});
>+                      {isBookmarked: PlacesAPI.bookmarksService.
>+                                     isBookmarked(LOCAL_TEST_PAGE)});

This is wrong and will fail. isBookmarked expects an nsIURI instance and no string.

>   // XXX: We should give a unique name too when controller.type will send oninput events (bug 474667)

Yeah. We probably forgot to remove the comment.

>   // XXX: Until we can't check via a menu click, call the Places API function for now (bug 474486)

Please file a follow-up for that. With the dynamic menus we can check this now. Otherwise only do the local test data changes.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Patch v1.1 - (default)

Addresses comments from comment #4

Comment 6

[Aaron Train [:aaronmt]]

Anthony, would you be able to get the initial review in? Note from comment #0 this test is broken due to Omnijar packaging changes, and is looked at in bug 592411

Comment 7

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

Comment on attachment 474801 [details] [diff] [review]
Patch v1.1 - (default)

>   controller.assertJS("subject.isBookmarked == false",
>-                      {isBookmarked: PlacesAPI.bookmarksService.isBookmarked(uri)});
>+                      {isBookmarked: PlacesAPI.bookmarksService.
>+                                     isBookmarked(uri)});

*snip*

>   controller.assertJS("subject.isBookmarkInBookmarksMenu == true",
>-                      {isBookmarkInBookmarksMenu: PlacesAPI.isBookmarkInFolder(uri, PlacesAPI.bookmarksService.bookmarksMenuFolder)});
>+                      {isBookmarkInBookmarksMenu: PlacesAPI.isBookmarkInFolder(uri, 
>+                                                  PlacesAPI.bookmarksService.bookmarksMenuFolder)});

Please fix the indentation in both cases. With that r=me.

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v1.2 - (default) + [indent fix]

Comment 9

[u279076]

(In reply to comment #8)
> Created attachment 478268 [details] [diff] [review]
> Patch v1.2 - (default) + [indent fix]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/5b52ea883f05 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/ec883a6d0e4a [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/4836e7aef32c [mozilla1.9.1]

Comment 10

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

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.