705122 - Mozmill endurance test for Add/Remove bookmarks via the awesomebar

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Create a mozmill endurance test for Add/Remove bookmarks via the awesomebar

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/21444103

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

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)]

Attachment: patch v1.0 all branches

Initial patch

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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)]

Attachment: 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

[u279076]

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] [he/him] ⌚BST]

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

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

Attachment: 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] [he/him] ⌚BST]

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

[u279076]

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)]

(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

[u279076]

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

Attachment: patch v1.3 all branches

Fixed to use the waitFor. removed the extra assert

Comment 15

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

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)]

Attachment: 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

[u279076]

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)]

Attachment: patch v2.1

nit addressed

Comment 19

[u279076]

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][⌚️UTC+1]]

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)]

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)]

Attachment: patch v3.0

Shared module dependency landed. 

We have a prettier test

Comment 23

[u279076]

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] [he/him] ⌚BST]

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)]

Attachment: patch v4.0

Nits fixed

Comment 26

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.1 [landed]

Previous patch broken - clean one up! Sorry Dave :(

Comment 27

[u279076]

Comment on attachment 594967 [details] [diff] [review]
patch v4.1 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/3d813a0cbbeb (default)

Comment 28

[u279076]

Vlad, please verify this test passes in tomorrow's testrun so I can port the patch to the other branches.

Comment 29

[u279076]

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)]

Passes 

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

Thanks for landing Anthony

Comment 31

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

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] [he/him] ⌚BST]

Backport not needed for ESR10.

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

Dave, can you please take care of the remaining check-in on the esr10 branch?

Comment 36

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: patch (esr10)

Backport patch for mozilla-esr10

Comment 37

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: patch (esr10)

Sorry, previous patch was missing necessary manifest changes.

Comment 38

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

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] [he/him] ⌚BST]

Backport landed for mozilla-esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/f498a8cbd133

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

marking fixed for esr10 based on comment 39