Closed Bug 779127 Opened 7 years ago Closed 7 years ago

Mozmill test for Switch to Tab feature

Categories

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

defect

Tracking

(firefox18 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- fixed

People

(Reporter: AndreeaMatei, Assigned: AndreeaMatei)

References

()

Details

(Whiteboard: s=121001 u=new c=tabbrowser p=1)

Attachments

(2 files, 16 obsolete files)

3.75 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
1.92 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
This is the tracking bug for a new mozmill test for switching tabs
https://moztrap.mozilla.org/manage/case/327/

It is going to follow the steps in MozTrap: 
1. Open three new tabs in Firefox
2. Navigate to three different pages 
3. Focus the location bar
4. Type a word from the title of one of the other pages into the location bar and select the item with Switch to Tab
5. Verify the switch is made and the page is not loaded again
Assignee: nobody → andreea.matei
I was trying to give this test to a volunteer contributor. Would it be possible for you to take something else?
Sure, no problem.
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #1)
> I was trying to give this test to a volunteer contributor. Would it be
> possible for you to take something else?

This is obsolete now. Andreea, if you don't mind you can continue to work on this test.
OS: Linux → All
Hardware: x86 → All
Attached patch patch v1 (all branches) (obsolete) — Splinter Review
Followed moztrap steps. 
For the tricky part which is selecting the "Swith to tab" item from the results list, I've looked for the element with the "actiontype" attribute (it's value is 'switchtab'). After switching, we verify the location bar contains the string typed before.
In the end, we check that there are only the 3 pages opened from the start.
Attachment #649285 - Flags: review?(alex.lakatos)
Comment on attachment 649285 [details] [diff] [review]
patch v1 (all branches)

>diff --git a/tests/functional/testAwesomeBar/testSuggestBookmarks.js b/tests/functional/testAwesomeBar/testSuggestBookmarks.js
Why are you changing testSuggestBookmarks.js in this bug? If you need to edit it, open a new bug.

>+function testSwitchToTabs() {
>+  for each(var page in LOCAL_TEST_PAGES) {
>+    controller.open(page.url);
>+    controller.waitForPageLoad();
>+    tabBrowser.openTab();
>+  }
>+
>+  tabBrowser.closeTab();
I don't like this here. There should be a more elegant way to do it.

>+  for each(page in LOCAL_TEST_PAGES) {
>+		// Type the strings from LOCAL_TEST_PAGES in the location bar
>+		locationBar.focus({type: "shortcut"});
>+		locationBar.type(page.string);
The indentation is way off here. Please correct it.

>+    // Go through all results and click the one with 'actiontype' attribute
>+    for(var index = 0; index < autoCompleteLength; index++) {
>+      var richlistItem = locationBar.autoCompleteResults.getResult(index);
You can use i here instead of index. Saves space.

>+        expect.ok(locationBar.contains(page.string), 
>+				          "Location bar should contain the page string");
>+			}
The indentation is way off here. Please correct it.
Attachment #649285 - Flags: review?(alex.lakatos) → review-
Attached patch patch v1.1 (all branches) (obsolete) — Splinter Review
Sorry about that, I've changed the settings for my editor.
Added an if for the issue with last tab to close.
Attachment #649285 - Attachment is obsolete: true
Attachment #649595 - Flags: review?(alex.lakatos)
Status: NEW → ASSIGNED
Comment on attachment 649595 [details] [diff] [review]
patch v1.1 (all branches)

>+const LOCAL_TEST_PAGES = [
>+   {url: LOCAL_TEST_FOLDER + 'mozilla_community.html', string: "community"},
>+   {url: LOCAL_TEST_FOLDER + 'mozilla_contribute.html', string: "contribute"},
>+   {url: LOCAL_TEST_FOLDER + 'mozilla_governance.html', string: "governance"}
You have three spaces here. Please have 2, as the style guide indicates

>+/*
>+ * Test Switch to Tab feature 
>+ */
>+function testSwitchToTab() {
>+  for(var i = 0; i < LOCAL_TEST_PAGES.length; i++) {
>+    controller.open(LOCAL_TEST_PAGES[i].url);
>+    controller.waitForPageLoad();
>+
>+    if(i != 2) 
>+      tabBrowser.openTab();
>+  }
I'd rather have here 
  for(var i = 0; i < LOCAL_TEST_PAGES.length; i++) {
    if(i > 0) {
      tabBrowser.openTab();
    }
    controller.open(LOCAL_TEST_PAGES[i].url);
    controller.waitForPageLoad();
for consistency with http://hg.mozilla.org/qa/mozmill-tests/file/c9cc7d5a8269/tests/endurance/testBookmarks_OpenAndClose/test1.js#l45

>+        expect.ok(locationBar.contains(page.string), 
>+                  "Location bar should contain the page string");
You should add a check here to see if the current tab has the right index.
Attachment #649595 - Flags: review?(alex.lakatos) → review-
Attached patch patch v2 (all branches) (obsolete) — Splinter Review
Addressed all requests.
Added a check for the current selected index and also edited manifest.ini with the moztrap_case.
Attachment #649595 - Attachment is obsolete: true
Attachment #650026 - Flags: review?(alex.lakatos)
Comment on attachment 650026 [details] [diff] [review]
patch v2 (all branches)

(In reply to Andreea Matei [:AndreeaMatei] from comment #8)
> Created attachment 650026 [details] [diff] [review]
> patch v2 (all branches)
> 
> Addressed all requests.
> Added a check for the current selected index and also edited manifest.ini
> with the moztrap_case.
That got reverted in bug 757916. Please have it at the end of the file instead.
Attachment #650026 - Flags: review?(alex.lakatos)
Attachment #650026 - Flags: review-
Attached patch patch v2.1 (all branches) (obsolete) — Splinter Review
Added moztrap id at the end of the test.
Attachment #650026 - Attachment is obsolete: true
Attachment #650150 - Flags: review?(alex.lakatos)
Comment on attachment 650150 [details] [diff] [review]
patch v2.1 (all branches)

Looks ok. Over to Dave/Henrik for additional review.
Attachment #650150 - Flags: review?(hskupin)
Attachment #650150 - Flags: review?(dave.hunt)
Attachment #650150 - Flags: review?(alex.lakatos)
Attachment #650150 - Flags: review+
Comment on attachment 650150 [details] [diff] [review]
patch v2.1 (all branches)

>+  for(var i = 0; i < LOCAL_TEST_PAGES.length; i++) {

nit: missing spaces after for.

>+    if(i > 0) {
>+      tabBrowser.openTab();
>+    }

Why is that if necessary. It doesn't matter when we open another empty tab. Also I would make use of forEach as we do in other tests.

>+  for (var page = 0; page < LOCAL_TEST_PAGES.length; page++) {

Same as above. forEach please.

>+    // Get the visible results from the autocomplete list
>+    var autoCompleteList = locationBar.autoCompleteResults.getElement({type:"results"});

Can't you make use of autoCompleteResults.visibleResults()? Also this should be put after the isOpened waitFor call.

>+    controller.waitFor(function() {

nit: missing space between function and the opening bracket.

>+    for(i = 0; i < autoCompleteList.getNode().getNumberOfVisibleRows(); i++) {
>+      var richlistItem = locationBar.autoCompleteResults.getResult(i);

We should be able to re-use the elements got from visibleResults().

>+
>+      if(richlistItem.getNode().hasAttribute("actiontype")) {

Does actiontype only indicate the switch-to-tab feature? Which value does it have in this case?

>+        expect.ok(locationBar.contains(LOCAL_TEST_PAGES[page].string), 
>+                  "Location bar should contain the page string");

I wouldn't use contains() here but do a expect.match() call. The thing we will not see any details of the expected and target string. Also before we click we probably should check the auto-complete entry for the string too. 

>+        expect.equal(tabBrowser.selectedIndex, page, 
>+                     "Current tab has the expected index");

I would better assert that the right url is displayed via utils.assertUrlEqual().

>+   // Check the number of pages loaded
>+   expect.equal(tabBrowser.length, LOCAL_TEST_PAGES.length, 
>+                "Only the required test pages have been loaded");

Why do we need that? What else should have opened another tab?

Also, has been done a check which of those tests are already covered by the browser chrome tests?
Attachment #650150 - Flags: review?(hskupin)
Attachment #650150 - Flags: review?(dave.hunt)
Attachment #650150 - Flags: review-
Attached patch patch v3 (all branches) (obsolete) — Splinter Review
I have made all the requested changes.

The if for opening new tab was because a blank tab will remain opened after loading all three pages. And that needs to be closed so we type the first string in the last loaded page.
Changing to forEach, I added tabBrowser.closeTab() to handle this issue.

The "actiontype" attribute is present only on Switch To Tab items and it's value is "switchtab".
Attachment #650150 - Attachment is obsolete: true
Attachment #651752 - Flags: review?(hskupin)
Attachment #651752 - Flags: review?(dave.hunt)
Comment on attachment 651752 [details] [diff] [review]
patch v3 (all branches)

>+  tabBrowser = new tabs.tabBrowser(controller);  

Remove trailing whitespace.

>+  for each (var page in LOCAL_TEST_PAGES) {
>+    controller.open(page.url);
>+    controller.waitForPageLoad();
>+    tabBrowser.openTab();
>+  }

Henrik actually suggested using forEach as shown below.

LOCAL_TEST_PAGES.forEach(function (page) {
  controller.open(page.url);
  controller.waitForPageLoad();
  tabBrowser.openTab();
});

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/forEach

>+  tabBrowser.closeTab();

I think Henrik's point is that we are not concerned with this empty tab being open. It should not adversely affect the test. We are more concerned with such things in endurance tests because we are consciously wanted to accumulate a specific number of entities.

>+  for each (page in LOCAL_TEST_PAGES) {

As above, use forEach.

>+    // Go through all results and click the one with 'actiontype' attribute
>+    for each (var richlistItem in autoCompleteList) {

If this is an array then we can use forEach here too.

>+      if(richlistItem.getNode().hasAttribute("actiontype")) {

Missing space after if.

Also, given your comment that the value is "switchtab", I feel that would be a better indication that this is the correct item.

>+        expect.match(page.string, /[\w+(?!\_)]/.exec(locationBar.value), 
>+                     "Location bar should contain the page string");

Message should be "Location bar contains " + page.string

>+        // Check the right url is displayed
>+        utils.assertLoadedUrlEqual(controller, page.url);

This comment is not needed, the following line of code is descriptive enough.
Attachment #651752 - Flags: review?(hskupin)
Attachment #651752 - Flags: review?(dave.hunt)
Attachment #651752 - Flags: review-
(In reply to Dave Hunt (:davehunt) from comment #14)
> >+        expect.match(page.string, /[\w+(?!\_)]/.exec(locationBar.value), 
> >+                     "Location bar should contain the page string");
> 
> Message should be "Location bar contains " + page.string

That's not necessary. The match() function takes care about. Also why do you run exec on it? If you want to do a full match use equal, otherwise we have an expect.contains() method.
Attached patch patch v3.1 (obsolete) — Splinter Review
Addressed all requests.
As discussed on IRC, I used expect.contain() instead of expect.match().
Also used forEach and the value of "actiontype" attribute.
Attachment #651752 - Attachment is obsolete: true
Attachment #652392 - Flags: review?(hskupin)
Attachment #652392 - Flags: review?(dave.hunt)
Comment on attachment 652392 [details] [diff] [review]
patch v3.1

>Bug 779127 - Mozmill test for Switch to Tab feature. r=alex.lakatos, r=dhunt, r=hskupin

Please only reference the official peers of the module as reviewer. You all mostly give feedback to each other.

>+  LOCAL_TEST_PAGES.forEach(function (page) {

Parameters should use an 'a' prefix. So please update all in this test to aPage.

>+    // Check the autocomplete list is open
>+    controller.waitFor(function () {
>+      return locationBar.autoCompleteResults.isOpened;
>+    }, "Autocomplete popup has been opened");

No need for the comment.

>+    // Get the visible results from the autocomplete list
>+    var autoCompleteList = locationBar.autoCompleteResults.visibleResults;
>+
>+    controller.waitFor(function () {
>+      return autoCompleteList.length != 0;
>+    }, "Waiting for autocomplete results to load");

No need for the comment too. Also remove the extra empty line. I think we could fail here on heavily loaded systems when the auto-complete entry isn't shown directly but takes a bit longer. Can you please test it so we can ensure we do not fail?

>+    autoCompleteList.forEach(function (richlistItem) {

'a' prefix too.

>+      var item = locationBar.autoCompleteResults.getUnderlinedText(richlistItem, 
>+                                                                   "title");
>+      if (richlistItem.getNode().getAttribute("actiontype") === "switchtab") {
>+        expect.equal(page.string, item.toString().toLowerCase(), 
>+                     "The page title matches the underlined text");
>+        controller.click(richlistItem);
>+
>+        expect.contain(locationBar.value, page.string,
>+                       "Search string in the location bar");
>+
>+        utils.assertLoadedUrlEqual(controller, page.url);
>+      }
>+    });

So what if switch to tab is not present at all? We have to fail and this doesn't happen right now.
Attachment #652392 - Flags: review?(hskupin)
Attachment #652392 - Flags: review?(dave.hunt)
Attachment #652392 - Flags: review-
Attached patch patch v4 (obsolete) — Splinter Review
Updated as requested. 
Sorry about commit message, on that week you were in PTO I had to get review from Vlad and Alex first, then Dave.

I have tested the waitFor with heavy system and it passes, probably is not an issue since is mostly just one result or max two.
Attachment #652392 - Attachment is obsolete: true
Attachment #652762 - Flags: review?(hskupin)
Attachment #652762 - Flags: review?(dave.hunt)
Comment on attachment 652762 [details] [diff] [review]
patch v4

>+    // Go through all results and click the one with 'actiontype' attribute

This doesn't reflect the code, but should probably not contain attribute details. I would suggest "...and click 'switch to tab'"

>+      } else {
>+        expect.ok(!aRichlistItem.getNode().hasAttribute("actiontype"), 
>+                  "Expected a 'Switch to tab' item to be present");

This is just checking that the current item does not have the 'actiontype' attribute, and this would likely not cause the test to fail if 'switch to tab' was not present. What we need is probably a boolean variable that is initialised with false and set to true within the if statement. Then we can simply assert that it is true after the forEach.
Attachment #652762 - Flags: review?(hskupin)
Attachment #652762 - Flags: review?(dave.hunt)
Attachment #652762 - Flags: review-
Attached patch patch v5 (obsolete) — Splinter Review
Changed the comment, added switchToTab boolean variable and checked it's value after forEach.
Attachment #652762 - Attachment is obsolete: true
Attachment #653304 - Flags: review?(hskupin)
Attachment #653304 - Flags: review?(dave.hunt)
Comment on attachment 653304 [details] [diff] [review]
patch v5

>+    controller.waitFor(function () {
>+      return locationBar.autoCompleteResults.isOpened;
>+    }, "Autocomplete popup has been opened");
>+
>+    var autoCompleteList = locationBar.autoCompleteResults.visibleResults;
>+    controller.waitFor(function () {
>+      return autoCompleteList.length != 0;
>+    }, "Waiting for autocomplete results to load");

Would we need the first waitFor(), given that we check the visibleResults later? Also I think we can run into intermittent failures with the 2nd waitFor(). Shouldn't exactly one match be displayed in the auto-complete list for the given text?

>+        controller.click(aRichlistItem);
>+
>+        expect.contain(locationBar.value, aPage.string,
>+                       "Search string in the location bar");
>+
>+        utils.assertLoadedUrlEqual(controller, aPage.url);

How fast are we here? Could this cause problems directly checking the URL? If we have a delay assertLoadedUrlEqual would load the wrong page into the currently selected tab. I think that you simple want to run a single except.contains against the full page URL here. The search string is not that necessary here because we know that it will be part of the URL.

>+    expect.ok(switchToTab, "Expected 'Switch to tab' item to be present");

Looks good. But I also think we should skip the loop after we found the first switch to tab entry. There is no need to check the remaining entries.
Attachment #653304 - Flags: review?(hskupin)
Attachment #653304 - Flags: review?(dave.hunt)
Attachment #653304 - Flags: review-
Attached patch patch v5.1 (obsolete) — Splinter Review
I have changed the second waitFor to use directly the autoCompleteResults length, unfortunately without the first waitFor the test fails to load the results. Probably it buys a little time for the results to get populated.
Tested with heavily loaded system as well. 

If we really want to get rid of this waitFor, I think we will need a "trick" like in testVisibleItemsMax test where we used a short sleep while typing the string.
We have more than one result because of the Mozilla Firefox bookmark folder which contains "community" page for example.

I had to replace the last forEach with a normal for in order to skip the loop using "break". forEach has to go through all steps, can't skip it.

When used expect.contain against the full URL page, I got a difference of "http://" which our page.url has and locationBar.value doesn't. So instead of locationBar.value we use activeTab.location.href.
Attachment #653304 - Attachment is obsolete: true
Attachment #654150 - Flags: review?(hskupin)
Attachment #654150 - Flags: review?(dave.hunt)
Comment on attachment 654150 [details] [diff] [review]
patch v5.1

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

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +39,5 @@
> +
> +  LOCAL_TEST_PAGES.forEach(function (aPage) {
> +    // Type the strings from LOCAL_TEST_PAGES in the location bar
> +    locationBar.focus({type: "shortcut"});
> +    locationBar.type(aPage.string);

Do we need to wait for aPage.string to appear in the locationBar.value as we have with other tests recently?

@@ +41,5 @@
> +    // Type the strings from LOCAL_TEST_PAGES in the location bar
> +    locationBar.focus({type: "shortcut"});
> +    locationBar.type(aPage.string);
> +
> +    controller.waitFor(function () {

I've done a bit of testing without this, and it appears that on all but the first loop it is necessary to wait for the autoCompleteList to open, as autoCompleteResults.length does not reset to 0 after the first loop.

@@ +52,5 @@
> +
> +    // Go through all results and click 'Switch to tab'
> +    var autoCompleteList = locationBar.autoCompleteResults.visibleResults;
> +    var switchToTab = false;
> +    for (i = 0; i < autoCompleteList.length; i++) {

You could use autoCompleteList.some here to stop processing once the callback returns true. See https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/some
Attachment #654150 - Flags: review?(hskupin)
Attachment #654150 - Flags: review?(dave.hunt)
Attachment #654150 - Flags: review-
Attached patch patch v5 (obsolete) — Splinter Review
Addressed all requests, also used autocompleteList.some() and it's working as expected.
Attachment #654150 - Attachment is obsolete: true
Attachment #661236 - Flags: review?(hskupin)
Attachment #661236 - Flags: review?(dave.hunt)
Comment on attachment 661236 [details] [diff] [review]
patch v5

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

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +63,5 @@
> +                     "The page title matches the underlined text");
> +        controller.click(aRichlistItem);
> +        expect.equal(controller.tabs.activeTab.location.href, aPage.url,
> +                     "Active tab url should equal the page url");
> +      }

In order to stop checking the remaining items you would need to return true here. You could do this by simply returning switchToTab, which is true once the switch tab is found. Otherwise, this patch looks great.
Attachment #661236 - Flags: review?(hskupin)
Attachment #661236 - Flags: review?(dave.hunt)
Attachment #661236 - Flags: review-
Attached patch patch v5.1 (obsolete) — Splinter Review
Thanks Dave. Updated accordingly to the last comment.
Attachment #661236 - Attachment is obsolete: true
Attachment #662457 - Flags: review?(hskupin)
Attachment #662457 - Flags: review?(dave.hunt)
Attachment #662457 - Flags: review?(hskupin)
Attachment #662457 - Flags: review?(dave.hunt)
Attachment #662457 - Flags: review?
Attached patch patch v5.2 (obsolete) — Splinter Review
Corrected the return.
Attachment #662457 - Attachment is obsolete: true
Attachment #662457 - Flags: review?
Attachment #662927 - Flags: review?(hskupin)
Attachment #662927 - Flags: review?(dave.hunt)
Comment on attachment 662927 [details] [diff] [review]
patch v5.2

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/4599129c281e (default)
Attachment #662927 - Flags: review?(hskupin)
Attachment #662927 - Flags: review?(dave.hunt)
Attachment #662927 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This fails:
http://mozmill-ci.blargon7.com/#/functional/report/fac9ae8444a3cbcc1dcf9cbaff153ec3

 The page title matches the underlined text - 'contribute' should equal 'contribut'
Active tab url should equal the page url - 'http://localhost:43336/mozilla_community.html' should equal 'http://localhost:43336/mozilla_contribute.html' 

Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/ffe37160c968
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: s=q3 u=new c=tabbrowser p=1
This bug is a quarterly goal so please finish it off by end of this week.
Attached patch patch v5.3 (obsolete) — Splinter Review
When checking the underlined string, I replaced the expect.equal with an assert.waitFor as used in the other tests from awesome bar in bug 792394. 
I believe an assert is more appropriate here instead an expect since from the report we see that after the first failure, it won't click the richlistItem so the next expect will also fail.
Tested multiple times on heavy loaded Mac OS X 10.7.4 and passed internal reviews.
Attachment #662927 - Attachment is obsolete: true
Attachment #665466 - Flags: review?(hskupin)
Attachment #665466 - Flags: review?(dave.hunt)
Comment on attachment 665466 [details] [diff] [review]
patch v5.3

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

Haven't really given a review here yet, so this test looks fine. There are some other things which slipped through and which we should fix.

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +38,5 @@
> +
> +  LOCAL_TEST_PAGES.forEach(function (aPage) {
> +    locationBar.focus({type: "shortcut"});
> +    locationBar.type(aPage.string);
> +    controller.waitFor(function () {

Please do me a favor and change the controller.waitFor() calls to assert/expect.waitFor().

@@ +54,5 @@
> +    // Go through all results and click 'Switch to tab'
> +    var autoCompleteList = locationBar.autoCompleteResults.visibleResults;
> +    var switchToTab = false;
> +    autoCompleteList.some(function (aRichlistItem) {
> +      var item = locationBar.autoCompleteResults.getUnderlinedText(aRichlistItem,

You should do `var switchToTab = autoCompleteList.some(` to get the result back if at least one item was a switchToTab entry. Btw. good call in using .some() here!

@@ +67,5 @@
> +        expect.equal(controller.tabs.activeTab.location.href, aPage.url,
> +                     "Active tab url should equal the page url");
> +      }
> +
> +      return switchToTab;

There is no need to handle switchToTab in this callback. Save the comparison result from above in a temporary variable and return it at the end.

@@ +70,5 @@
> +
> +      return switchToTab;
> +    });
> +
> +    expect.ok(switchToTab, "Expected 'Switch to tab' item to be present");

why are you making use of 'Expected' here? Please remove it.
Attachment #665466 - Flags: review?(hskupin)
Attachment #665466 - Flags: review?(dave.hunt)
Attachment #665466 - Flags: review-
Attached patch patch v5.4 (obsolete) — Splinter Review
Updated as requested. 
Passed internal reviews.
Attachment #665466 - Attachment is obsolete: true
Attachment #665884 - Flags: review?(hskupin)
Attachment #665884 - Flags: review?(dave.hunt)
Comment on attachment 665884 [details] [diff] [review]
patch v5.4

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

Looks great. Just one more thing before we can land the patch.

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +60,5 @@
> +      if (aRichlistItem.getNode().getAttribute("actiontype") === "switchtab") {
> +        switchTabPresent = true;
> +        assert.waitFor(function () {
> +          return item.toString().toLowerCase() === aPage.string;
> +        }, "The page title matches the underlined text");

How does some() behave if we assert here? Do we completely stop the test or only this iteration? I think that we even don't need the extra variable. We can return true at the end of the if condition or false at the end of the callback.
(In reply to Henrik Skupin (:whimboo) from comment #32)
> Haven't really given a review here yet, so this test looks fine. There are
> some other things which slipped through and which we should fix.

Thanks for the additional eyes on this patch - you made some good points.
Did some testing and assert completely stops the test, not just the iteration. But I'm not sure why we'd want to go forward with the rest of the test if we fail there.
We could get rid of the extra variable by returning true at the end of the if condition, since some() needs the callback to return true in order to stop the iteration.
(In reply to Andreea Matei [:AndreeaMatei] from comment #36)
> We could get rid of the extra variable by returning true at the end of the
> if condition, since some() needs the callback to return true in order to
> stop the iteration.

Right, that's what I was thinking of. Please make this change and we can get it landed. Thanks!
Attached patch patch v5.5 (obsolete) — Splinter Review
Updated as requested.
Attachment #665884 - Attachment is obsolete: true
Attachment #665884 - Flags: review?(hskupin)
Attachment #665884 - Flags: review?(dave.hunt)
Attachment #666471 - Flags: review?(hskupin)
Attachment #666471 - Flags: review?(dave.hunt)
Comment on attachment 666471 [details] [diff] [review]
patch v5.5

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

::: tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +63,5 @@
> +
> +        controller.click(aRichlistItem);
> +        expect.equal(controller.tabs.activeTab.location.href, aPage.url,
> +                     "Active tab url should equal the page url");
> +        return true;

It's not enough to return true here. You should always return a value. That means code outside of this if clause has to return false.
Attachment #666471 - Flags: review?(hskupin)
Attachment #666471 - Flags: review?(dave.hunt)
Attachment #666471 - Flags: review-
Whiteboard: s=q3 u=new c=tabbrowser p=1 → s=20121001 u=new c=tabbrowser p=1
Whiteboard: s=20121001 u=new c=tabbrowser p=1 → s=121001 u=new c=tabbrowser p=1
Returning true inside if and false outside the if clause.
Attachment #666471 - Attachment is obsolete: true
Attachment #666961 - Flags: review?(hskupin)
Attachment #666961 - Flags: review?(dave.hunt)
Attachment #666961 - Flags: review?(hskupin)
Attachment #666961 - Flags: review?(dave.hunt)
Attachment #666961 - Flags: review+
Lets hope it sticks this time. We will close as fixed once no failures can be seen in tomorrows testrun.

http://hg.mozilla.org/qa/mozmill-tests/rev/f8fc87bb2cd7 (default)
I believe we might have the same issue like on bug 792394 (which will be fixed next week), only on Mac OS X, because we use underlined here also.
Looks like it's stable enough that we can call it fixed now. The remaining issue will be taken care of in bug 792394.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Attached patch [beta, esr10, release] patch v1 (obsolete) — Splinter Review
Patch to add the test into beta, esr10 and release branches (updated with the timeout for the database).
Attachment #673241 - Flags: review?(hskupin)
Attachment #673241 - Flags: review?(dave.hunt)
Comment on attachment 673241 [details] [diff] [review]
[beta, esr10, release] patch v1

We don't want this patch yet until it is still failing. Please add the proposed 4s wait and then we can backport a combined patch.
Attachment #673241 - Flags: review?(hskupin)
Attachment #673241 - Flags: review?(dave.hunt)
Attachment #673241 - Flags: review-
Adding timeout sleep in order to help adding the history items to the database.
Attachment #673588 - Flags: review?(hskupin)
Attachment #673588 - Flags: review?(dave.hunt)
Attachment #673588 - Flags: review?(hskupin)
Attachment #673588 - Flags: review?(dave.hunt)
Attachment #673588 - Flags: review+
Attachment #673588 - Attachment description: patch v1 → Sleep for places v1
Attachment #666961 - Attachment description: patch v6 → patch v6 [checked-in]
Comment on attachment 673241 [details] [diff] [review]
[beta, esr10, release] patch v1

We do not want to backport new tests except for endurance tests.
Attachment #673241 - Attachment is obsolete: true
Comment on attachment 673588 [details] [diff] [review]
Sleep for places v1 [checked-in]

http://hg.mozilla.org/qa/mozmill-tests/rev/2e61e9030e3d (default)

If that works we want to get this backported to the aurora branch.
Attachment #673588 - Attachment description: Sleep for places v1 → Sleep for places v1 [checked-in]
(In reply to Henrik Skupin (:whimboo) from comment #48)
> http://hg.mozilla.org/qa/mozmill-tests/rev/2e61e9030e3d (default)
> 
> If that works we want to get this backported to the aurora branch.

Given that it makes sense to have this sleep on aurora too, I have transplanted the patch:

http://hg.mozilla.org/qa/mozmill-tests/rev/5bac83f2625c (aurora)
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.