Test failure "Element.waitForElement(): Element 'Selector: *[label='Test Bookmark 1']' has been found" in /testBookmarks_OpenAndClose/test1.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Andrei Eftimie, Assigned: Andrei Eftimie)

Tracking

unspecified

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox-esr17 wontfix, firefox-esr24 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
See related: bug 918820

Report:
http://mozmill-crowd.blargon7.com/#/endurance/report/456bebe92845279408c15c03e81aa7cd

This recent failure might be Australis related.
We're seeing this with mozmill 2.0
Fails consistent across OS'es
(Assignee)

Comment 1

5 years ago
This is not Australis related.
It fails in pre-Australis builds (fails on a recent Aurora)
Blocks: 918820
Who could work on this bug?
(Assignee)

Comment 3

5 years ago
Lets get this fixed.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 4

5 years ago
Created attachment 8347224 [details] [diff] [review]
1.patch

There were several issues that I've fixed here.

1. We didn't clean up properly in the following 2 tests:
> testBookmarks_OpenAllInTabs/test1.js
> testBookmarks_OpenAndClose/test1.js

We opened the Bookmarks toolbar, but never closed it.
So when the second test ran, instead of opening the Bookmarks toolbar we actually closed it.

We also had a general method used for opening the Bookmarks toolbar in toolbars.js
I changed both tests to use that method.

I also changed that method from "enable" to "toggle" since exactly the same steps can be used to close the toolbar.

I then called the toogle method again in teardownModule so we close the Bookmarks toolbar after each test.

2. After the first test has run, when gathering the testFolder with:
> var testFolder = new elementslib.Selector(controller.window.document,
>                                          ".bookmark-item[label='Test Folder']");
We actually also got back a menu entry instead of the toolbar button, I also improved the specificity of this selector to also take the type into account

3. We weren't correctly waiting for the bookmark folder element.
While we used `waitThenClick` this method only checks for the existence of the element in the DOM.
When we initially tried clicking on the element it had its width and height 0.
Waiting for ~200ms would solve this.

We have a method in utils called isDisplayed() which makes sense to use in this case.
But that method would only check for the visibility of the element.
I also enhanced that method to better take care of visibility and also check for display, width and height.

Since I also changed a method from utils, here are multiple testrun types:
2.0.2
-----
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/ca1869364f98dd0de71202e01e4a3139
L10n: http://mozmill-crowd.blargon7.com/#/l10n/report/ca1869364f98dd0de71202e01e4765e6
Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/ca1869364f98dd0de71202e01e4582aa
Remote: http://mozmill-crowd.blargon7.com/#/remote/report/ca1869364f98dd0de71202e01e4a8e48
Addons: http://mozmill-crowd.blargon7.com/#/addons/report/ca1869364f98dd0de71202e01e481804

1.5.24
------
Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/ca1869364f98dd0de71202e01e463f10
Attachment #8347224 - Flags: review?(andreea.matei)
Comment on attachment 8347224 [details] [diff] [review]
1.patch

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

::: firefox/lib/toolbars.js
@@ +643,5 @@
>   *
>   * @param {MozmillController} aController
>   *        MozMillController of the window to operate on
>   */
> +function toggleBookmarksToolbar(aController) {

Would be best to have a parameter to know in which state we are (opened/closed), not just toggle. And a check in the end.
Attachment #8347224 - Flags: review?(andreea.matei) → review-
Comment on attachment 8347224 [details] [diff] [review]
1.patch

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

I agree with Andreea's comment and have some more which you can find inline.

::: firefox/lib/utils.js
@@ +465,5 @@
> +      var width = parseInt(style.getPropertyValue('width'), 10);
> +      var height = parseInt(style.getPropertyValue('height'), 10);
> +
> +      visible = (visibility !== 'hidden') &&
> +                (visibility !== 'collapse') &&

Why can't this be left at '=== visible'?

::: firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js
@@ +49,5 @@
>   * Test open all bookmarks in tabs
>   */
>  function testOpenAllBookmarksInTabs() {
>    var testFolder = new elementslib.Selector(controller.window.document,
> +                                            "toolbarbutton.bookmark-item[label='Test Folder']");

The bookmark item entry should be moved to the toolbars library later once we have a navigationbar class. A follow-up bug would be nice to have.

::: firefox/tests/endurance/testBookmarks_OpenAndClose/test1.js
@@ +46,4 @@
>                                                BOOKMARK_FOLDER_NAME + "']");
> +    assert.waitFor(function (){
> +      return utils.isDisplayed(controller, testFolder);
> +    }, BOOKMARK_FOLDER_NAME + " has loaded")

Or is there an event we can wait for? I highly suspect that.
Attachment #8347224 - Flags: review-
(Assignee)

Comment 7

5 years ago
Created attachment 8347275 [details] [diff] [review]
2.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #5)
> Comment on attachment 8347224 [details] [diff] [review]
> 1.patch
> 
> Review of attachment 8347224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/toolbars.js
> @@ +643,5 @@
> >   *
> >   * @param {MozmillController} aController
> >   *        MozMillController of the window to operate on
> >   */
> > +function toggleBookmarksToolbar(aController) {
> 
> Would be best to have a parameter to know in which state we are
> (opened/closed), not just toggle. And a check in the end.

Good idea. Implemented.

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 8347224 [details] [diff] [review]
> 1.patch
> 
> Review of attachment 8347224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with Andreea's comment and have some more which you can find inline.
> 
> ::: firefox/lib/utils.js
> @@ +465,5 @@
> > +      var width = parseInt(style.getPropertyValue('width'), 10);
> > +      var height = parseInt(style.getPropertyValue('height'), 10);
> > +
> > +      visible = (visibility !== 'hidden') &&
> > +                (visibility !== 'collapse') &&
> 
> Why can't this be left at '=== visible'?

Right, I can actually leave that part as it was.

> ::: firefox/tests/endurance/testBookmarks_OpenAllInTabs/test1.js
> @@ +49,5 @@
> >   * Test open all bookmarks in tabs
> >   */
> >  function testOpenAllBookmarksInTabs() {
> >    var testFolder = new elementslib.Selector(controller.window.document,
> > +                                            "toolbarbutton.bookmark-item[label='Test Folder']");
> 
> The bookmark item entry should be moved to the toolbars library later once
> we have a navigationbar class. A follow-up bug would be nice to have.

We create this bookmark in-test.
So should a `navigationbar` class handle adding/removing bookmark entries from the Bookmarks Toolbar?

> ::: firefox/tests/endurance/testBookmarks_OpenAndClose/test1.js
> @@ +46,4 @@
> >                                                BOOKMARK_FOLDER_NAME + "']");
> > +    assert.waitFor(function (){
> > +      return utils.isDisplayed(controller, testFolder);
> > +    }, BOOKMARK_FOLDER_NAME + " has loaded")
> 
> Or is there an event we can wait for? I highly suspect that.

There might be. I'll search for one.
Attachment #8347224 - Attachment is obsolete: true
Attachment #8347275 - Flags: review?(andreea.matei)
(In reply to Andrei Eftimie from comment #7)
> > The bookmark item entry should be moved to the toolbars library later once
> > we have a navigationbar class. A follow-up bug would be nice to have.
> 
> We create this bookmark in-test.
> So should a `navigationbar` class handle adding/removing bookmark entries
> from the Bookmarks Toolbar?

A class like that should handle everything which is related to this toolbar. Similar to the locationbar class. It doesn't matter how elements are getting created or when they are accessible.
Comment on attachment 8347275 [details] [diff] [review]
2.patch

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

Only styling issues remaining on my part.

::: firefox/lib/utils.js
@@ +467,5 @@
> +
> +      visible = (visibility === 'visible') &&
> +                (display !== 'none') &&
> +                (width > 0) &&
> +                (height > 0)

I'd put these last 2 on the same line to make it more compact.

::: firefox/tests/endurance/testBookmarks_OpenAndClose/test1.js
@@ +46,2 @@
>                                                BOOKMARK_FOLDER_NAME + "']");
> +    assert.waitFor(function (){

Whitespace after function ().
Attachment #8347275 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 10

5 years ago
(In reply to Andreea Matei [:AndreeaMatei] from comment #9)
> Comment on attachment 8347275 [details] [diff] [review]
> 2.patch
> 
> Review of attachment 8347275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only styling issues remaining on my part.
> 
> ::: firefox/lib/utils.js
> @@ +467,5 @@
> > +
> > +      visible = (visibility === 'visible') &&
> > +                (display !== 'none') &&
> > +                (width > 0) &&
> > +                (height > 0)
> 
> I'd put these last 2 on the same line to make it more compact.

I would leave every test on its own line.
But I think I'll sort them alphabetically.

> ::: firefox/tests/endurance/testBookmarks_OpenAndClose/test1.js
> @@ +46,2 @@
> >                                                BOOKMARK_FOLDER_NAME + "']");
> > +    assert.waitFor(function (){
> 
> Whitespace after function ().
I'll fix this shortly.

Also, I've filed bug 950666 for the creation of the bookmarksBar
(Assignee)

Comment 11

5 years ago
Created attachment 8348031 [details] [diff] [review]
3.patch

Fixed the nit.

Left all tested properties of isDisplayed each on its own line, but sorted them alphabetically.
Attachment #8347275 - Attachment is obsolete: true
Attachment #8348031 - Flags: review?(andreea.matei)
Comment on attachment 8348031 [details] [diff] [review]
3.patch

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

::: firefox/lib/utils.js
@@ +467,5 @@
> +
> +      visible = (display !== 'none') &&
> +                (height > 0) &&
> +                (visibility === 'visible') &&
> +                (width > 0)

I don't think alphabetical ordering makes sense here. At least we should have groups, and do not separate properties like height and width from each other. This only makes it harder to understand the logic later.
(Assignee)

Comment 13

5 years ago
Created attachment 8348087 [details] [diff] [review]
4.patch

Ok, I've grouped width & height.
Attachment #8348031 - Attachment is obsolete: true
Attachment #8348031 - Flags: review?(andreea.matei)
Attachment #8348087 - Flags: review?(hskupin)
Attachment #8348087 - Flags: review?(andreea.matei)
Comment on attachment 8348087 [details] [diff] [review]
4.patch

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

Looks good.
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d2f9645d3416 (default)
Attachment #8348087 - Flags: review?(hskupin)
Attachment #8348087 - Flags: review?(andreea.matei)
Attachment #8348087 - Flags: review+
status-firefox29: --- → fixed
(Assignee)

Comment 15

5 years ago
Transplanted to:
http://hg.mozilla.org/qa/mozmill-tests/rev/299fcdc38296 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/4d2c1f50dec8 (mozilla-beta)

I'll shortly upload a new patch for Release and ESR24
status-firefox26: --- → affected
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox-esr17: --- → wontfix
status-firefox-esr24: --- → affected
Comment on attachment 8348583 [details] [diff] [review]
mozilla-release.patch

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

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/7d5ddf1e77b3 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/c54b93e9ccfa (esr24)
Attachment #8348583 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox26: affected → fixed
status-firefox-esr24: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.