Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Mozmill endurance test for Add/Remove bookmarks via the awesomebar

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [mozmill-endurance])

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

6 years ago
Create a mozmill endurance test for Add/Remove bookmarks via the awesomebar
(Assignee)

Updated

6 years ago
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/21444103
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Comment 3

6 years ago
Created attachment 576785 [details] [diff] [review]
patch v1.0 all branches

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+
(Assignee)

Comment 5

6 years ago
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
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.
(Assignee)

Comment 8

6 years ago
(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
(Assignee)

Comment 9

6 years ago
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
Attachment #576793 - Attachment is obsolete: true
Attachment #576911 - Flags: feedback?(dave.hunt)

Updated

6 years ago
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.
(Assignee)

Comment 12

6 years ago
(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
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 14

6 years ago
Created attachment 576944 [details] [diff] [review]
patch v1.3 all branches

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.
(Assignee)

Comment 16

6 years ago
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
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-
(Assignee)

Comment 18

6 years ago
Created attachment 579028 [details] [diff] [review]
patch v2.1

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-
(Assignee)

Updated

6 years ago
Depends on: 708582
(Assignee)

Comment 21

6 years ago
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
(Assignee)

Comment 22

6 years ago
Created attachment 594656 [details] [diff] [review]
patch v3.0

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-
(Assignee)

Comment 25

6 years ago
Created attachment 594965 [details] [diff] [review]
patch v4.0

Nits fixed
Attachment #594656 - Attachment is obsolete: true
Attachment #594965 - Flags: review?(dave.hunt)
(Assignee)

Comment 26

6 years ago
Created attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

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+
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 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]
(Assignee)

Comment 30

6 years ago
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.
status-firefox-esr10: --- → checkin-pending
status-firefox15: --- → fixed
status-firefox16: --- → fixed
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Backport not needed for ESR10.
status-firefox-esr10: checkin-pending → wontfix
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.
status-firefox-esr10: wontfix → affected
Whiteboard: [mozmill-endurance][mozmill-bookmarks] → [mozmill-endurance]
Dave, can you please take care of the remaining check-in on the esr10 branch?
Created attachment 664466 [details] [diff] [review]
patch (esr10)

Backport patch for mozilla-esr10
Attachment #664466 - Flags: review?(hskupin)
Attachment #664466 - Flags: review?(hskupin) → review+
Created attachment 664468 [details] [diff] [review]
patch (esr10)

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+
Backport landed for mozilla-esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/f498a8cbd133
marking fixed for esr10 based on comment 39
status-firefox-esr10: affected → fixed
You need to log in before you can comment on or make changes to this bug.