Closed Bug 705122 Opened 8 years ago Closed 8 years ago

Mozmill endurance test for Add/Remove bookmarks via the awesomebar

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox-esr10 fixed)

VERIFIED FIXED
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox18 --- fixed
firefox-esr10 --- fixed

People

(Reporter: vladmaniac, Assigned: vladmaniac)

References

Details

(Whiteboard: [mozmill-endurance])

Attachments

(2 files, 9 obsolete files)

Create a mozmill endurance test for Add/Remove bookmarks via the awesomebar
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/21444103
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
Attached patch patch v1.0 all branches (obsolete) — Splinter Review
Initial patch
Attachment #576785 - Flags: feedback?(dave.hunt)
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?
Attachment #576785 - Flags: feedback?(dave.hunt) → feedback+
Attached patch patch v1.1 all branches (obsolete) — Splinter Review
With Dave's feedback addressed, attaching initial patch for review. 

Will obsolete v1.0 the moment Anthony agrees with the changes
Attachment #576793 - Flags: review?
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.
Attachment #576793 - Flags: review? → review-
Whiteboard: [mozmill-endurance][mozmill-bookmarks]
(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.
(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
Attached patch patch v1.2 all branches (obsolete) — Splinter Review
Fixed

Dave - does this patch satisfy our needs? please submit your feedback - then I'll ask for r from Anthony
Attachment #576793 - Attachment is obsolete: true
Attachment #576911 - Flags: feedback?(dave.hunt)
Blocks: 629065
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");
>+  });
>+}
Attachment #576911 - Flags: feedback?(dave.hunt) → feedback-
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.
(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
Attachment #576785 - Attachment is obsolete: true
(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.
Attached patch patch v1.3 all branches (obsolete) — Splinter Review
Fixed to use the waitFor. removed the extra assert
Attachment #576911 - Attachment is obsolete: true
Attachment #576944 - Flags: review?(anthony.s.hughes)
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.
Attached patch patch v2.0 (obsolete) — Splinter Review
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
Attachment #576944 - Attachment is obsolete: true
Attachment #576944 - Flags: review?(anthony.s.hughes)
Attachment #577224 - Flags: review?(anthony.s.hughes)
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"
Attachment #577224 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v2.1 (obsolete) — Splinter Review
nit addressed
Attachment #577224 - Attachment is obsolete: true
Attachment #579028 - Flags: review?(anthony.s.hughes)
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
Attachment #579028 - Flags: review?(hskupin)
Attachment #579028 - Flags: review?(anthony.s.hughes)
Attachment #579028 - Flags: review+
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.
Attachment #579028 - Flags: review?(hskupin) → review-
Depends on: 708582
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
Attached patch patch v3.0 (obsolete) — Splinter Review
Shared module dependency landed. 

We have a prettier test
Attachment #579028 - Attachment is obsolete: true
Attachment #594656 - Flags: review?(anthony.s.hughes)
Comment on attachment 594656 [details] [diff] [review]
patch v3.0

Looks good to me. Over to Dave for a quick feedback check.
Attachment #594656 - Flags: review?(anthony.s.hughes)
Attachment #594656 - Flags: review+
Attachment #594656 - Flags: feedback?(dave.hunt)
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.
Attachment #594656 - Flags: feedback?(dave.hunt) → feedback-
Attached patch patch v4.0 (obsolete) — Splinter Review
Nits fixed
Attachment #594656 - Attachment is obsolete: true
Attachment #594965 - Flags: review?(dave.hunt)
Previous patch broken - clean one up! Sorry Dave :(
Attachment #594965 - Attachment is obsolete: true
Attachment #594965 - Flags: review?(dave.hunt)
Attachment #594967 - Flags: review?(dave.hunt)
Attachment #594967 - Flags: review?(dave.hunt) → review+
Keywords: checkin-needed
Comment on attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3d813a0cbbeb (default)
Attachment #594967 - Attachment description: patch v4.1 → patch v4.1 [landed:default]
Vlad, please verify this test passes in tomorrow's testrun so I can port the patch to the other branches.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
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.
Attachment #594967 - Attachment description: patch v4.1 [landed:default] → patch v4.1 [landed]
Passes 

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

Thanks for landing Anthony
Status: RESOLVED → VERIFIED
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.
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.
We can land on esr10 once the remaining patch on bug 708582 landed on that branch. See for more details over there.
Whiteboard: [mozmill-endurance][mozmill-bookmarks] → [mozmill-endurance]
Dave, can you please take care of the remaining check-in on the esr10 branch?
Attached patch patch (esr10) (obsolete) — Splinter Review
Backport patch for mozilla-esr10
Attachment #664466 - Flags: review?(hskupin)
Attachment #664466 - Flags: review?(hskupin) → review+
Attached patch patch (esr10)Splinter Review
Sorry, previous patch was missing necessary manifest changes.
Attachment #664466 - Attachment is obsolete: true
Attachment #664468 - Flags: review?(hskupin)
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.
Attachment #664468 - Flags: review?(hskupin) → review+
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.