Last Comment Bug 705122 - Mozmill endurance test for Add/Remove bookmarks via the awesomebar
: Mozmill endurance test for Add/Remove bookmarks via the awesomebar
Status: VERIFIED FIXED
[mozmill-endurance]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
Mentors:
Depends on: 708582
Blocks: 629065
  Show dependency treegraph
 
Reported: 2011-11-24 06:12 PST by Maniac Vlad Florin (:vladmaniac)
Modified: 2012-12-11 11:11 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
patch v1.0 all branches (4.25 KB, patch)
2011-11-24 09:12 PST, Maniac Vlad Florin (:vladmaniac)
dave.hunt: feedback+
Details | Diff | Review
patch v1.1 all branches (4.22 KB, patch)
2011-11-24 10:04 PST, Maniac Vlad Florin (:vladmaniac)
anthony.s.hughes: review-
Details | Diff | Review
patch v1.2 all branches (8.51 KB, patch)
2011-11-25 05:44 PST, Maniac Vlad Florin (:vladmaniac)
dave.hunt: feedback-
Details | Diff | Review
patch v1.3 all branches (4.46 KB, patch)
2011-11-25 08:13 PST, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Review
patch v2.0 (7.89 KB, patch)
2011-11-28 04:59 PST, Maniac Vlad Florin (:vladmaniac)
anthony.s.hughes: review-
Details | Diff | Review
patch v2.1 (7.89 KB, patch)
2011-12-05 04:13 PST, Maniac Vlad Florin (:vladmaniac)
anthony.s.hughes: review+
hskupin: review-
Details | Diff | Review
patch v3.0 (4.30 KB, patch)
2012-02-06 01:15 PST, Maniac Vlad Florin (:vladmaniac)
anthony.s.hughes: review+
dave.hunt: feedback-
Details | Diff | Review
patch v4.0 (8.32 KB, patch)
2012-02-07 02:14 PST, Maniac Vlad Florin (:vladmaniac)
no flags Details | Diff | Review
patch v4.1 [landed] (4.32 KB, patch)
2012-02-07 02:24 PST, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review+
Details | Diff | Review
patch (esr10) (3.00 KB, patch)
2012-09-25 05:56 PDT, Dave Hunt (:davehunt)
hskupin: review+
Details | Diff | Review
patch (esr10) (4.55 KB, patch)
2012-09-25 06:13 PDT, Dave Hunt (:davehunt)
hskupin: review+
Details | Diff | Review

Description Maniac Vlad Florin (:vladmaniac) 2011-11-24 06:12:58 PST
Create a mozmill endurance test for Add/Remove bookmarks via the awesomebar
Comment 1 Maniac Vlad Florin (:vladmaniac) 2011-11-24 06:15:17 PST
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/21444103
Comment 2 Maniac Vlad Florin (:vladmaniac) 2011-11-24 06:30:11 PST
Approach for this test will be: 

Add/remove bookmark via awesomebar

    Main Test Checkpoints

    Open page

    Add bookmark using button in awesomebar

    Remove bookmark using button in awesomebar

    Teardown Module

    Force close tabs
Comment 3 Maniac Vlad Florin (:vladmaniac) 2011-11-24 09:12:03 PST
Created attachment 576785 [details] [diff] [review]
patch v1.0 all branches

Initial patch
Comment 4 Dave Hunt (:davehunt) 2011-11-24 09:35:32 PST
Comment on attachment 576785 [details] [diff] [review]
patch v1.0 all branches

>+function testAddRemoveBookmarkViaStar() {

I would suggest naming this testAddRemoveBookmarkViaAwesomeBar

>+  enduranceManager.run(function () {
>+    var URI = utils.createURI(LOCAL_TEST_PAGE); 
>+
>+    // Open URI and wait until it has been finished loading
>+    enduranceManager.addCheckpoint("Load a web-page from local test folder");
>+    controller.open(URI.spec);
>+    controller.waitForPageLoad();
>+    enduranceManager.addCheckpoint("Web page has been loaded");

The open page is not part of the test snippet. Let's move this to the setupModule. Also, what's the reason for the URI? We can pass a string to the controller.open() method as we have done elsewhere?
Comment 5 Maniac Vlad Florin (:vladmaniac) 2011-11-24 10:04:22 PST
Created attachment 576793 [details] [diff] [review]
patch v1.1 all branches

With Dave's feedback addressed, attaching initial patch for review. 

Will obsolete v1.0 the moment Anthony agrees with the changes
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 10:41:22 PST
Comment on attachment 576793 [details] [diff] [review]
patch v1.1 all branches

Please remember to set the requestee field -- I would have missed this if it weren't for my morning coffee.

>+function testAddRemoveBookmarkViaAwesomeBar() {
>+  enduranceManager.run(function () {
>+    // Open test page and wait until it has been finished loading
>+    enduranceManager.addCheckpoint("Load a web-page from local test folder");
>+    controller.open(LOCAL_TEST_PAGE);
>+    controller.waitForPageLoad();
>+    enduranceManager.addCheckpoint("Web page has been loaded");

I believe one of the things Dave was asking for previously was that we don't need to have a checkpoint for loading the testpage. The purpose of this test is to gather metrics on adding/removing bookmarks. Dave can correct me if I am wrong, but I think we only need checkpoints for the specific data we are trying to collect with a given test. As such, you can move opening the test page to before enduranceManager.run().

Again, Dave can correct me if I am wrong.

>+    // Trigger editBookmarksPanel and remove bookmark
>+    controller.click(starButton);

I'm wondering if we should have an assertion to verify the bookmark as been added before starting the checkpoint? Otherwise we might be gather data on a false case.

>+    controller.click(removeBookmark); 
>+    enduranceManager.addCheckpoint("Bookmark has been removed");

The same goes for removing a bookmark.

Dave, please respond to my comments inline, thanks.
Comment 7 Dave Hunt (:davehunt) 2011-11-24 11:00:06 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #6)
> I believe one of the things Dave was asking for previously was that we don't
> need to have a checkpoint for loading the testpage. The purpose of this test
> is to gather metrics on adding/removing bookmarks. Dave can correct me if I
> am wrong, but I think we only need checkpoints for the specific data we are
> trying to collect with a given test. As such, you can move opening the test
> page to before enduranceManager.run().
> 
> Again, Dave can correct me if I am wrong.

You're exactly right. The opening of the page should be in the setup method and have no checkpoints associated.

> >+    // Trigger editBookmarksPanel and remove bookmark
> >+    controller.click(starButton);
> 
> I'm wondering if we should have an assertion to verify the bookmark as been
> added before starting the checkpoint? Otherwise we might be gather data on a
> false case.

I'm happy with that suggestion.

> >+    controller.click(removeBookmark); 
> >+    enduranceManager.addCheckpoint("Bookmark has been removed");
> 
> The same goes for removing a bookmark.

Also happy with this.
Comment 8 Maniac Vlad Florin (:vladmaniac) 2011-11-25 01:59:26 PST
(In reply to Dave Hunt (:davehunt) from comment #7)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #6)
> > I believe one of the things Dave was asking for previously was that we don't
> > need to have a checkpoint for loading the testpage. The purpose of this test
> > is to gather metrics on adding/removing bookmarks. Dave can correct me if I
> > am wrong, but I think we only need checkpoints for the specific data we are
> > trying to collect with a given test. As such, you can move opening the test
> > page to before enduranceManager.run().
> > 
> > Again, Dave can correct me if I am wrong.
> 
> You're exactly right. The opening of the page should be in the setup method
> and have no checkpoints associated.
> 
> > >+    // Trigger editBookmarksPanel and remove bookmark
> > >+    controller.click(starButton);
> > 
> > I'm wondering if we should have an assertion to verify the bookmark as been
> > added before starting the checkpoint? Otherwise we might be gather data on a
> > false case.
> 
> I'm happy with that suggestion.
> 
> > >+    controller.click(removeBookmark); 
> > >+    enduranceManager.addCheckpoint("Bookmark has been removed");
> > 
> > The same goes for removing a bookmark.
> 
> Also happy with this.

Heh, I was prepared - my initial test had those assertions and was thinking of proposing them. Fix patch will follow today
Comment 9 Maniac Vlad Florin (:vladmaniac) 2011-11-25 05:44:07 PST
Created attachment 576911 [details] [diff] [review]
patch v1.2 all branches

Fixed

Dave - does this patch satisfy our needs? please submit your feedback - then I'll ask for r from Anthony
Comment 10 Dave Hunt (:davehunt) 2011-11-25 07:19:13 PST
Comment on attachment 576911 [details] [diff] [review]
patch v1.2 all branches

Please just update your previous patch and mark it as obsolete.

>+var {assert} = require("../../../lib/assertions"); 

It's been a while since I worked in mozmill-tests that aren't against the new API. This line looks unfamiliar, is it something we need for asserts now?

>+    // Wait for the bookmark event
>+    controller.waitFor(function () {
>+      return places.bookmarksService.isBookmarked(URI) == true;

Do we need the == true? Isn't isBookmarked expected to return true?

>+    }, "Bookmark event finished");
>+ 
>+    // Verify the bookmark was created
>+    assert.ok(places.bookmarksService.isBookmarked(URI), "The bookmark was created");
>+    enduranceManager.addCheckpoint("Bookmark has been created");

Do we need both the wait and the assert?

>+    // Trigger editBookmarksPanel and remove bookmark
>+    controller.click(starButton);
>+
>+    controller.waitFor(function() {
>+      return controller.window.top.StarUI._overlayLoaded == true;

Again, do we need the == true here?

>+    }, "Edit Bookmarks Panel has been loaded");
>+  
>+    var removeBookmark = new elementslib.ID(controller.window.document, 
>+                                            "editBookmarkPanelRemoveButton");
>+
>+    controller.click(removeBookmark); 
>+
>+    // Verify the bookmark was removed
>+    assert.ok(!places.bookmarksService.isBookmarked(URI), "The bookmark was removed");
>+    enduranceManager.addCheckpoint("Bookmark has been removed");
>+  });
>+}
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-25 07:48:27 PST
Dave, that is the correct way to use asserts; we also have expects for non-fatal assertions: hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js

Remus, I'm not sure why waitFor is necessary here. Either use a waitFor or an Assert, not both.
Comment 12 Maniac Vlad Florin (:vladmaniac) 2011-11-25 07:58:22 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Dave, that is the correct way to use asserts; we also have expects for
> non-fatal assertions:
> hg.mozilla.org/qa/mozmill-tests/file/default/lib/assertions.js
> 
> Remus, I'm not sure why waitFor is necessary here. Either use a waitFor or
> an Assert, not both.

Guys, the waitFor makes sure the bookmark event took place - sometimes the star event is lazy we cannot risk a timeout failure in the expense of a couple of extra code lines. Makes the test more robust IMO
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-25 08:05:45 PST
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #12)
> Guys, the waitFor makes sure the bookmark event took place - sometimes the
> star event is lazy we cannot risk a timeout failure in the expense of a
> couple of extra code lines. Makes the test more robust IMO

Yes, but a waitFor is a fatal assert. I don't understand why you are running waitFor() and then assert() afterward. The second assert() is redundant and unnecessary in my opinion.
Comment 14 Maniac Vlad Florin (:vladmaniac) 2011-11-25 08:13:57 PST
Created attachment 576944 [details] [diff] [review]
patch v1.3 all branches

Fixed to use the waitFor. removed the extra assert
Comment 15 Henrik Skupin (:whimboo) 2011-11-25 10:15:34 PST
Comment on attachment 576944 [details] [diff] [review]
patch v1.3 all branches

>+      return places.bookmarksService.isBookmarked(URI) == true;
>+    }, "The bookmark was created");

No, we don't need a check for true here. It's already a boolean which gets returned by the isBookmarked method.

>+    controller.waitFor(function() {
>+      return controller.window.top.StarUI._overlayLoaded == true;
>+    }, "Edit Bookmarks Panel has been loaded");

Same here.

>+    var removeBookmark = new elementslib.ID(controller.window.document, 
>+                                            "editBookmarkPanelRemoveButton");

Please keep in mind that we do not want to instantiate elements from tests. So please add a new type to the API.
Comment 16 Maniac Vlad Florin (:vladmaniac) 2011-11-28 04:59:47 PST
Created attachment 577224 [details] [diff] [review]
patch v2.0

We do not have a UI map for Places API right now. 
Started a draft method called getElements (the same we do with add-ons) and used nodeCollector to get the wanted element. 

All other changes are addressed, but i believe we'll want more than that for the API. 

Also keep in mind that the goal here is getting as much coverage as we can for endurance
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-02 16:59:01 PST
Comment on attachment 577224 [details] [diff] [review]
patch v2.0

>+    enduranceManager.addCheckpoint("Bookmark has been created");

Let's refer to awesomebar in the message:
"Bookmark added via the Awesomebar"
Comment 18 Maniac Vlad Florin (:vladmaniac) 2011-12-05 04:13:43 PST
Created attachment 579028 [details] [diff] [review]
patch v2.1

nit addressed
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-06 14:44:49 PST
Comment on attachment 579028 [details] [diff] [review]
patch v2.1

This looks okay to me. Henrik, since this patch has changes to the Places API, can you please review?

Thanks
Comment 20 Henrik Skupin (:whimboo) 2011-12-07 04:27:06 PST
Comment on attachment 579028 [details] [diff] [review]
patch v2.1

>diff --git a/lib/places.js b/lib/places.js
[..]
>+    // Edit Bookmarks Panel 
>+    case "EditPanel_removeButton":
>+      nodeCollector.queryNodes("#editBookmarkPanelRemoveButton");
>+      break;
>+  }

Not sure why this has to go into places which is the library. I would rather see it as part of the toolbar module.

>+    // Wait for the bookmark event
>+    controller.waitFor(function () {
>+      return places.bookmarksService.isBookmarked(URI);
>+    }, "The bookmark was created");

nit: You are not waiting for an event here but polling the bookmarks service if such a bookmark has been added.

>+    controller.waitFor(function() {

nit: missing a space.

>+      return controller.window.top.StarUI._overlayLoaded;
>+    }, "Edit Bookmarks Panel has been loaded");

Beside the getElement stuff I would think we should add new API for the panel in the toolbar module. You should have created a separate bug for that in the appropriate component.

>+    controller.click(removeBookmark); 
>+
>+    // Verify the bookmark was removed
>+    assert.ok(!places.bookmarksService.isBookmarked(URI), "The bookmark was removed");
>+    enduranceManager.addCheckpoint("Bookmark has been removed");

Why no waitFor? The removal of bookmarks is asynchronous, so our tests can fail if it takes longer to remove the bookmark from the database.
Comment 21 Maniac Vlad Florin (:vladmaniac) 2011-12-08 04:38:18 PST
We will add the edit bookmarks panel to the toolbar API the address to this tests needs 
Thanks for r Henrik. I liked your proposals
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-02-06 01:15:38 PST
Created attachment 594656 [details] [diff] [review]
patch v3.0

Shared module dependency landed. 

We have a prettier test
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-06 21:41:11 PST
Comment on attachment 594656 [details] [diff] [review]
patch v3.0

Looks good to me. Over to Dave for a quick feedback check.
Comment 24 Dave Hunt (:davehunt) 2012-02-07 02:10:06 PST
Comment on attachment 594656 [details] [diff] [review]
patch v3.0

>+++ b/tests/endurance/testBookmarks_addRemoveBookmarkViaAwesomeBar/test1.js

Can we rename this directory to testBookmarks_AddAndRemoveBookmarkViaAwesomeBar

>+ * The Initial Developer of the Original Code is Vlad Maniac.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.

Nit: It's 2012 now :)

With those two nits addressed this looks good to me.
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-02-07 02:14:56 PST
Created attachment 594965 [details] [diff] [review]
patch v4.0

Nits fixed
Comment 26 Maniac Vlad Florin (:vladmaniac) 2012-02-07 02:24:09 PST
Created attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

Previous patch broken - clean one up! Sorry Dave :(
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-07 13:48:52 PST
Comment on attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3d813a0cbbeb (default)
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-07 13:49:34 PST
Vlad, please verify this test passes in tomorrow's testrun so I can port the patch to the other branches.
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-17 14:49:16 PST
Comment on attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/bf6dd2f7eea6 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/c1b152f7b054 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/26493086f803 (mozilla-release)

Vlad, please verify this with tomorrow's test results.
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-02-20 02:33:49 PST
Passes 

http://mozmill-release.blargon7.com/#/endurance/report/55d601cc2aabfac28f59060a84abd3c9

Thanks for landing Anthony
Comment 31 Henrik Skupin (:whimboo) 2012-09-19 02:08:30 PDT
This test is missing on the esr10 branch. Dave, can you please make sure we get it landed there together with the skip patch in a single push? Thanks.
Comment 32 Dave Hunt (:davehunt) 2012-09-19 02:41:22 PDT
Backport not needed for ESR10.
Comment 33 Dave Hunt (:davehunt) 2012-09-19 02:42:21 PDT
Sorry, mid-air collision. To give more context, this test fails on mozilla-esr10 with "locationBar.editBookmarksPanel is undefined". This is a feature that we do not have on esr10.
Comment 34 Henrik Skupin (:whimboo) 2012-09-22 03:34:56 PDT
We can land on esr10 once the remaining patch on bug 708582 landed on that branch. See for more details over there.
Comment 35 Henrik Skupin (:whimboo) 2012-09-25 00:29:06 PDT
Dave, can you please take care of the remaining check-in on the esr10 branch?
Comment 36 Dave Hunt (:davehunt) 2012-09-25 05:56:23 PDT
Created attachment 664466 [details] [diff] [review]
patch (esr10)

Backport patch for mozilla-esr10
Comment 37 Dave Hunt (:davehunt) 2012-09-25 06:13:06 PDT
Created attachment 664468 [details] [diff] [review]
patch (esr10)

Sorry, previous patch was missing necessary manifest changes.
Comment 38 Henrik Skupin (:whimboo) 2012-09-25 06:16:04 PDT
Comment on attachment 664468 [details] [diff] [review]
patch (esr10)

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

Hm, the patch header looks strange. But otherwise good to go.
Comment 39 Dave Hunt (:davehunt) 2012-09-25 06:55:56 PDT
Backport landed for mozilla-esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/f498a8cbd133
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2012-12-11 11:11:01 PST
marking fixed for esr10 based on comment 39

Note You need to log in before you can comment on or make changes to this bug.