Failed for 'subject.visible == subject.expectedVisibility' in testGoButton.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: whimboo, Assigned: davehunt)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Module:    /testAwesomeBar/testGoButton.js
Test:      testGoButtonOnTypeOnly    
Failure:   controller.assertJS: Failed for 'subject.visible == subject.expectedVisibility' 
Branches:  default
Platforms: Linux

This failure has been started to occur on 110104, so it's fairly new. Aaron, can you investigate?

Updated

8 years ago
Assignee: nobody → dave.hunt

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 years ago
Created attachment 503156 [details] [diff] [review]
Patch v1 - (default)

Failure was related to the awesomebar autocomplete list being left open from the previous test. The attached patch adds a close method to the location bar class, and add the call to each of the tests that left the list open. The fact that this issue only occurred on Linux may be due to a genuine bug, which I will investigate next.
Attachment #503156 - Flags: review?(hskupin)
Attachment #503156 - Flags: review?(aaron.train)
(Assignee)

Updated

8 years ago
Attachment #503156 - Attachment is obsolete: true
Attachment #503156 - Flags: review?(hskupin)
Attachment #503156 - Flags: review?(aaron.train)
(Assignee)

Comment 2

8 years ago
Created attachment 503160 [details] [diff] [review]
Patch v1.1 - (default)

Fixed a typo causing a poorly formatted comment.
Attachment #503160 - Flags: review?(hskupin)
Attachment #503160 - Flags: review?(aaron.train)
Comment on attachment 503160 [details] [diff] [review]
Patch v1.1 - (default)

Patch looks fine. Question to raise though, would it be better to place the calls to closeAutocompletePopup() in the tests tearDownModule functions instead of inside the tests themselves. I've assumed that all admin/cleanup work goes into that bucket. Henrik?
Attachment #503160 - Flags: review?(aaron.train) → review+
(Reporter)

Comment 4

8 years ago
Comment on attachment 503160 [details] [diff] [review]
Patch v1.1 - (default)

>@@ -121,16 +121,19 @@ var testCheckItemHighlight = function() 
[..]
>+  // Close the autocomplete list
>+  locationBar.closeAutocompletePopup();

As Aaron has already pointed out, we definitely have to check that the popup has been closed at the end of the test. In case like this it will still not be closed if a test right before this call fails. It absolutely has to be part of the teardownModule function. Therefore leave those instances and simply add another call to teardownModule, which will be the forced closing.

>+  closeAutocompletePopup : function autoCompleteResults_closeAutocompletePopup() {
>+    if (this.autoCompleteResults.isOpened) {
>+      this._controller.keypress(this.urlbar, "VK_ESCAPE", {});
>+      this._controller.waitFor(function () {
>+        return !this.autoCompleteResults.isOpened;
>+      }, "Autocomplete list should not be open.");
>+    }
>+  },

Hm, we already have the toggleAutocompletePopup function in the locationBar class. It really looks like that we should have separate open and close methods for these two actions. In case of close I really like the way to use Escape. It gives us much more safety. Toggling via the history button is a rare case and will probably only be tested by a small number of tests. And those tests should get the button element from the locationBar class and click them themselves.

Also I wonder why this function is part of the locationBar class and not the autoCompleteResults class, which would make it much more obvious to use. Such a move would require checks for existing tests but will be a great improvement.
Attachment #503160 - Flags: review?(hskupin) → review-
(Assignee)

Updated

8 years ago
Attachment #503160 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Created attachment 503501 [details] [diff] [review]
Patch v2 - (default)

Moved the close function into the autoCompleteResults class. Also moved the call to this to the teardownModule of necessary tests. Finally, I removed the unused toggleAutocomplete method as it's unused and doesn't fit in with our future plans for this class.
Attachment #503501 - Flags: review?(hskupin)
(Reporter)

Comment 6

8 years ago
Comment on attachment 503501 [details] [diff] [review]
Patch v2 - (default)

>+var teardownModule = function() {
>+  locationBar.autoCompleteResults.close();

As discussed on IRC we should enforce the closing of the popup inside of the teardownModule call. Just specify a parameter for it in .close(). Beside that also place this call right after a test in the real test function. The teardownModule call should only make sure that the popup has really been closed.

>+  close : function autoCompleteResults_close() {
>+    if (this.isOpened) {
>+      this._controller.keypress(locationBar.urlbar, "VK_ESCAPE", {});
>+      this._controller.waitFor(function () {
>+        return !this.isOpened;
>+      }, "Autocomplete list should not be open.");
>+    }

Lets add a forced closing of the popup via a parameter. In this mode we don't synthesize any mouse click or keypress, but call https://developer.mozilla.org/en/XUL/menupopup#m-hidePopup directly. If we don't do this the popup could still remain open.
Attachment #503501 - Flags: review?(hskupin) → review-
(Assignee)

Updated

8 years ago
Attachment #503501 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
Created attachment 503813 [details] [diff] [review]
Patch v3 - (default)

Reimplemented the close at the end of the necessary tests, and added a force close to the teardownModule as suggested.
Attachment #503813 - Flags: review?(hskupin)
(Reporter)

Comment 8

8 years ago
Comment on attachment 503813 [details] [diff] [review]
Patch v3 - (default)

>@@ -96,14 +100,16 @@ var testAccessLocationBarHistory = funct
> 
>   // Finally - Check that the mozilla page was loaded by verifying the
>   // Mozilla logo exists
>   var mozillaLogo = new elementslib.ID(controller.tabs.activeTab, "mozilla_logo");
>   controller.waitForElement(mozillaLogo, TIMEOUT, 100);
> 
>   // Check that the URL in the awesomebar matches the last LOCAL_TEST_PAGE
>   locationBar.contains(LOCAL_TEST_PAGES[2]);
>+
>+  locationBar.autoCompleteResults.close();

This test doesn't have to close the popup. It's already done with hitting the Enter key.

>@@ -85,14 +89,16 @@ var testEscape = function() {
>   // After the second Escape press, confirm the locationbar returns to the current page url
>   controller.keypress(locationBar.urlbar, 'VK_ESCAPE', {});
>   controller.assertJS("subject.contains('" + LOCAL_TEST_PAGES[1] + "') == true", locationBar);
>+
>+  locationBar.autoCompleteResults.close();

Same here.

With this change r=me. Please upload the updated version, so I can checkin the patch. Also please backport to older branches and attach patches as needed.
Attachment #503813 - Flags: review?(hskupin) → review+
(Assignee)

Updated

8 years ago
Attachment #503813 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 503837 [details] [diff] [review]
Patch v3.1 - (default) [checked-in]

Removed unnecessary calls to close the autocomplete popup within the tests.
Attachment #503837 - Flags: review?(hskupin)
(Reporter)

Comment 10

8 years ago
Comment on attachment 503837 [details] [diff] [review]
Patch v3.1 - (default) [checked-in]

I will check it in.
Attachment #503837 - Flags: review?(hskupin) → review+
(Reporter)

Comment 11

8 years ago
Comment on attachment 503837 [details] [diff] [review]
Patch v3.1 - (default) [checked-in]

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/21e68f29c4ac
Attachment #503837 - Attachment description: Patch v3.1 - (default) → Patch v3.1 - (default) [checked-in]
(Assignee)

Updated

8 years ago
Attachment #503837 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Created attachment 503840 [details] [diff] [review]
Patch v3.1 - (1.9.2) [checked-in]

Backport for branch mozilla1.9.2
Attachment #503840 - Flags: review?(hskupin)
(Reporter)

Updated

8 years ago
Attachment #503837 - Attachment is obsolete: false
(Assignee)

Updated

8 years ago
Attachment #503837 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #503840 - Attachment is obsolete: true
Attachment #503840 - Flags: review?(hskupin)
(Assignee)

Comment 13

8 years ago
Created attachment 503844 [details] [diff] [review]
Patch v3.1 - (1.9.1) [checked-in]

Backport for branch mozilla1.9.1
Attachment #503844 - Flags: review?(hskupin)
(Assignee)

Updated

8 years ago
Attachment #503840 - Attachment is obsolete: false
Attachment #503840 - Flags: review?(hskupin)
(Assignee)

Updated

8 years ago
Attachment #503837 - Attachment is obsolete: false
(Reporter)

Comment 14

8 years ago
Comment on attachment 503840 [details] [diff] [review]
Patch v3.1 - (1.9.2) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e08f3808cc52 (1.9.2)
Attachment #503840 - Flags: review?(hskupin) → review+
(Reporter)

Updated

8 years ago
Attachment #503840 - Attachment description: Patch v3.1 - (1.9.2) → Patch v3.1 - (1.9.2) [checked-in]
(Reporter)

Comment 15

8 years ago
Comment on attachment 503844 [details] [diff] [review]
Patch v3.1 - (1.9.1) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/ad5a94deb3c1
Attachment #503844 - Attachment description: Patch v3.1 - (1.9.1) → Patch v3.1 - (1.9.1) [checked-in]
Attachment #503844 - Flags: review?(hskupin) → review+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.