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

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

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

Comment 1

[Andrei Eftimie]

This is not Australis related.
It fails in pre-Australis builds (fails on a recent Aurora)

Comment 2

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

Who could work on this bug?

Comment 3

[Andrei Eftimie]

Lets get this fixed.

Comment 4

[Andrei Eftimie]

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

Comment 5

[Andreea Matei [:AndreeaMatei]]

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.

Comment 6

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

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.

Comment 7

[Andrei Eftimie]

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

Comment 8

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

(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 9

[Andreea Matei [:AndreeaMatei]]

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

Comment 10

[Andrei Eftimie]

(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

Comment 11

[Andrei Eftimie]

Attachment: 3.patch

Fixed the nit.

Left all tested properties of isDisplayed each on its own line, but sorted them alphabetically.

Comment 12

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

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.

Comment 13

[Andrei Eftimie]

Attachment: 4.patch

Ok, I've grouped width & height.

Comment 14

[Andreea Matei [:AndreeaMatei]]

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)

Comment 15

[Andrei Eftimie]

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

Comment 16

[Andrei Eftimie]

Attachment: mozilla-release.patch

Patch for mozilla-release and mozilla-esr24

http://mozmill-crowd.blargon7.com/#/endurance/report/ca1869364f98dd0de71202e01ebc56e7
http://mozmill-crowd.blargon7.com/#/endurance/report/ca1869364f98dd0de71202e01ebb3be9

Comment 17

[Andreea Matei [:AndreeaMatei]]

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)