Closed Bug 981520 Opened 7 years ago Closed 7 years ago

Test failure 'Edit Bookmarks Panel has been opened' in testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

defect

Tracking

(firefox27 unaffected, firefox28 unaffected, firefox29 fixed, firefox30 fixed, firefox31 fixed, firefox-esr24 unaffected)

RESOLVED FIXED
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: andrei, Assigned: danisielm)

References

()

Details

(Whiteboard: [mozmill-test-failure][Australis:P-])

Attachments

(2 files, 6 obsolete files)

The UI for Bookmarks has changed in Nightly.
This is making one of our endurance tests to fail.

We'll need a regressor identified and have a fix ready.
Cosmin, can you please handle this?

Sample report:
http://mozmill-daily.blargon7.com/#/endurance/report/f02addc77637daff0089973ee4000226
Attached patch skip_patch (obsolete) — Splinter Review
This reproduces every time, I will search for regression.
Attachment #8388402 - Flags: review?(andrei.eftimie)
Attached patch skip_patchSplinter Review
I removed the name of the test from the skip message.
Attachment #8388402 - Attachment is obsolete: true
Attachment #8388402 - Flags: review?(andrei.eftimie)
Attachment #8388427 - Flags: review?(andrei.eftimie)
Comment on attachment 8388427 [details] [diff] [review]
skip_patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/2a0f61676022 (default)
Attachment #8388427 - Flags: review?(andrei.eftimie)
Attachment #8388427 - Flags: review+
Attachment #8388427 - Flags: checkin+
There is a new animation that happens when we bookmark a page. This prevents us from hitting the bookmark button again. 
I can reproduce this manually: if we hit the bookmark button too fast, the event is prevented. 
If I add a sleep before the click action on the bookmark button it works. 
Also if I call the bookmark panel via menu, I don't have to add the sleep.
> controller.mainMenu.click("#menu_bookmarkThisPage");
I guess we should either wait for animation to finish or relay on something else for opening the panel. I would wait for animation as this is closer to users experience.
https://hg.mozilla.org/qa/mozmill-tests/file/2a0f61676022/firefox/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js#l56

I would do a regression range anyway, and return with the exact culprit.
I made the regression range on mozilla-inbound this is the pushlog  
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81c5d568d949&tochange=af1a435e117a 
For me it seems to be introduced by bug 970013.
This is the pushlog from central:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ff96e428da76&tochange=0dc1be930880
I will prepare a patch with an open method in which we would wait for the animation event.
(In reply to Cosmin Malutan from comment #5)
> For me it seems to be introduced by bug 970013.

Not sure about that. There are two other bugs in that range which could have caused this. Please check those via the fx-team tinderbox builds.

Gijs, do you have an idea what could have causes this problem for us in clicking the bookmark button?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Cosmin Malutan from comment #5)
> > For me it seems to be introduced by bug 970013.
> 
> Not sure about that. There are two other bugs in that range which could have
> caused this. Please check those via the fx-team tinderbox builds.
> 
> Gijs, do you have an idea what could have causes this problem for us in
> clicking the bookmark button?

I expect that it really is bug 970013. See bug 981548. For now, adding a setTimeout for 1.5 seconds would be the most surefire way of avoiding the issue. Alternatively, use a shortcut to open the panel?
Blocks: 970013
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][Australis:P-]
(In reply to :Gijs Kruitbosch from comment #8)
> I expect that it really is bug 970013. See bug 981548. For now, adding a
> setTimeout for 1.5 seconds would be the most surefire way of avoiding the
> issue. Alternatively, use a shortcut to open the panel?

I would be happier with an event we could wait for. We could indeed use the keyboard shortcut, but such kind of handling should be part of the shortcut related tests in Mozmill. By default we are trying to use the elements directly via mouse actions.
Attached patch patch_v1.0 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #9)
> I would be happier with an event we could wait for. We could indeed use the
> keyboard shortcut, but such kind of handling should be part of the shortcut
> related tests in Mozmill. By default we are trying to use the elements
> directly via mouse actions.
If I add an event listener for animationend event to occur, the click event is still prevented because the dom attribute that sets the css style is still there. We would have to wait for the attribute to be removed again. I used the mutation observes for watching this.
I would add a TODO comment and add the bug 981548, so we can rollback if this issue get's fixed.
Attachment #8389071 - Flags: feedback?(hskupin)
Attachment #8389071 - Flags: feedback?(andrei.eftimie)
The aurora build is also affected. Failed with Aurora en-US on mac 10.6
http://mozmill-daily.blargon7.com/#/endurance/report/d8c9b09561f242bc8489be43d61c0f95

The skip patch applies cleanly on aurora branch to.
(In reply to Cosmin Malutan from comment #11)
> The aurora build is also affected. Failed with Aurora en-US on mac 10.6
> http://mozmill-daily.blargon7.com/#/endurance/report/
> d8c9b09561f242bc8489be43d61c0f95
> 
> The skip patch applies cleanly on aurora branch to.

This is because the patch that caused the issue isn't there yet, and the patch that fixes it is still on fx-team and not on m-c or m-a yet.
Comment on attachment 8389071 [details] [diff] [review]
patch_v1.0

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

I think that looks fine, but if a patch is in the works to get the underlying problem fixed, I would prefer to wait for it. We can re-enable the test once it has been landed on m-c and later on m-a.
Attachment #8389071 - Flags: feedback?(hskupin)
Attachment #8389071 - Flags: feedback?(andrei.eftimie)
Attachment #8389071 - Flags: feedback+
I have tested this with the latest nightly, and now it opens the panel, but when we ran whit more than one iteration, it fails to add the bookmark on second time.

This happens because now that it doesn't prevent the click, we add and remove the bookmark while still animating, and the second time we iterate we are still animating and this time it won't add the bookmark anymore.

Adding a sleep or applying the fix patch prevents this.

http://mozmill-crowd.blargon7.com/#/endurance/report/20537b179864872c8bd47eacd42af136
http://mozmill-crowd.blargon7.com/#/endurance/report/20537b179864872c8bd47eacd42ae074
http://mozmill-crowd.blargon7.com/#/endurance/report/20537b179864872c8bd47eacd4299e78
Comment on attachment 8392980 [details] [diff] [review]
patch_v1.1

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

::: firefox/lib/toolbars.js
@@ +615,5 @@
>      this._controller.type(this.urlbar, text);
>    },
>  
>    /**
> +   * Set's an mutation observer to wait for it then clicks on star button

Nit: "Sets a"

@@ +622,5 @@
> +    var self = { started: false, ended: false };
> +    var bookmarksMenuButton = this.getElement({type: "bookmarksMenuButton"});
> +    var window = bookmarksMenuButton.getNode().ownerDocument.defaultView;
> +
> +    var mutationObserver = new window.MutationObserver(function (mutations) {

aMutations

@@ +623,5 @@
> +    var bookmarksMenuButton = this.getElement({type: "bookmarksMenuButton"});
> +    var window = bookmarksMenuButton.getNode().ownerDocument.defaultView;
> +
> +    var mutationObserver = new window.MutationObserver(function (mutations) {
> +      mutations.forEach(function (mutation) {

aMutation

@@ +627,5 @@
> +      mutations.forEach(function (mutation) {
> +        // We have to wait until the attribute has been added then removed
> +        // Only waiting for animationend event doesn't help as it will take
> +        // a different action for changing the css style and therefore enabling
> +        // the button

To summarize better:
// For changing the CSS style and enabling the button there's a different action
// besides animationend event
// We have to wait until the attribute has been added then removed

@@ +635,5 @@
> +        else {
> +          self.ended = !mutation.target.hasAttribute("notification");
> +        }
> +      });
> +    });

I'd add a blank line here for separating blocks.

@@ +641,5 @@
> +                             {attributes: true, attributeFilter: ["notification"]});
> +    this.getElement({type: "starButton"}).click();
> +    try {
> +      assert.waitFor(function () {
> +        return self.ended;

We can use => here.

::: firefox/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +43,5 @@
>  
>      assert.waitFor(function () {
>        return places.isBookmarkStarButtonReady(controller);
>      });
> +    // Click on bookmark button and the waits for animation

"Click on the bookmark button and wait for the animation"

@@ +67,5 @@
>  
>      enduranceManager.addCheckpoint("Bookmark has been removed");
>    });
>  }
> +

The skip lines weren't removed. And please also reflect that in the manifest file.
Attachment #8392980 - Flags: review?(andrei.eftimie)
Attachment #8392980 - Flags: review?(andreea.matei)
Attachment #8392980 - Flags: review-
Attached patch patch_v1.2.patch (obsolete) — Splinter Review
Updated patch with the requested changes.
Attachment #8389071 - Attachment is obsolete: true
Attachment #8392980 - Attachment is obsolete: true
Attachment #8393415 - Flags: review?(andrei.eftimie)
Attachment #8393415 - Flags: review?(andreea.matei)
Assignee: cosmin.malutan → daniel.gherasim
Comment on attachment 8393415 [details] [diff] [review]
patch_v1.2.patch

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

Looks good to me, just one small edit mentioned inline.

Since we are adding a new method to a library, please ask a review from Henrik with the mentioned change.

::: firefox/lib/toolbars.js
@@ +646,5 @@
> +      assert.waitFor(() => self.ended);
> +    }
> +    finally {
> +      mutationObserver.disconnect();
> +      delete mutationObserver;

This doesn't do much of anything. Please remove this line.
Attachment #8393415 - Flags: review?(andrei.eftimie)
Attachment #8393415 - Flags: review?(andreea.matei)
Attachment #8393415 - Flags: review-
Attached patch patch_v1.3.patch (obsolete) — Splinter Review
Thanks a lot.
Asked Henrik for final review.
Attachment #8393415 - Attachment is obsolete: true
Attachment #8393521 - Flags: review?(hskupin)
Comment on attachment 8393521 [details] [diff] [review]
patch_v1.3.patch

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

::: firefox/lib/toolbars.js
@@ +618,5 @@
>    /**
> +   * Bookmark a page using the star button
> +   * Also waits for the animation event that occurs to finish
> +   */
> +  bookmarkWithAnimation : function locationBar_bookmarkWithAnimation () {

nit: no space behind the function name

@@ +621,5 @@
> +   */
> +  bookmarkWithAnimation : function locationBar_bookmarkWithAnimation () {
> +    var self = { started: false, ended: false };
> +    var bookmarksMenuButton = this.getElement({type: "bookmarksMenuButton"});
> +    var window = bookmarksMenuButton.getNode().ownerDocument.defaultView;

I would use `this.controller.window.document.defaultView` here.

@@ +630,5 @@
> +        // action besides animationend event
> +        // We have to wait until the attribute has been added then removed
> +
> +        if (!self.started) {
> +          self.started = aMutation.target.getAttribute("notification") === "finish";

nit: for better readability please put brackets around the comparison.

@@ +640,5 @@
> +    });
> +
> +    mutationObserver.observe(bookmarksMenuButton.getNode(),
> +                             {attributes: true, attributeFilter: ["notification"]});
> +    this.getElement({type: "starButton"}).click();

I would like to see a callback here for the user action. There might be different path to trigger this code.

@@ +643,5 @@
> +                             {attributes: true, attributeFilter: ["notification"]});
> +    this.getElement({type: "starButton"}).click();
> +    try {
> +      assert.waitFor(() => self.ended);
> +    }

The try has to go around the action AND the call to observe(). Otherwise we might leak.
Attachment #8393521 - Flags: review?(hskupin) → review-
Attached patch patch_v1.4.patch (obsolete) — Splinter Review
Updated patch with the small changes asked.
Attachment #8393521 - Attachment is obsolete: true
Attachment #8393993 - Flags: review?(andrei.eftimie)
Attachment #8393993 - Flags: review?(andreea.matei)
Comment on attachment 8393993 [details] [diff] [review]
patch_v1.4.patch

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

One more change before I can land this.

::: firefox/lib/toolbars.js
@@ +615,5 @@
>      this._controller.type(this.urlbar, text);
>    },
>  
>    /**
> +   * Bookmark a page using the star button

Please update this comment, it isn't correct anymore since we added a callback method and may have different ways besides the star button.
Attachment #8393993 - Flags: review?(andrei.eftimie)
Attachment #8393993 - Flags: review?(andreea.matei)
Attachment #8393993 - Flags: review-
Attached patch patch_v1.5.patchSplinter Review
Updated the comment, Thanks.
Attachment #8393993 - Attachment is obsolete: true
Attachment #8394074 - Flags: review?(andrei.eftimie)
Attachment #8394074 - Flags: review?(andreea.matei)
Comment on attachment 8394074 [details] [diff] [review]
patch_v1.5.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/816b91838a1b (default)

We need backporting for this.
Attachment #8394074 - Flags: review?(andrei.eftimie)
Attachment #8394074 - Flags: review?(andreea.matei)
Attachment #8394074 - Flags: review+
The patch applies cleanly for aurora & beta and we can backport this.

For ESR24 & RELEASE we don't have the new animation so we don't want to backport on those branches.

Testrun reports:
http://mozmill-crowd.blargon7.com/#/endurance/reports?app=All&branch=All&platform=All&from=2014-03-21&to=2014-03-21
http://hg.mozilla.org/qa/mozmill-tests/rev/a55f16b590f2 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/db77334d4a00 (beta)

We're done here then, all endurance tests are enabled :)
Thanks Cosmin and Daniel!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.