629106 - Mozmill test for Panorama search tabs from other windows

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Litmus ID:
https://litmus.mozilla.org/show_test.cgi?id=13771

Comment 1

[adam.fishhook+bugzilla]

Attachment: Automation Script for Test 13771

Comment 2

[adam.fishhook+bugzilla]

Attachment: WIP Automated Test Script

Made changes after initial review.

Comment 3

[u279076]

Comment on attachment 518723 [details]
WIP Automated Test Script

> * Contributor(s):
> * Zaki Juhari <[e-mail]>

You should add "(original author)" after your email address.

>const SEARCH_URL = LOCAL_TEST_PAGE;

This is redundant, simply refer to LOCAL_TEST_PAGE wherever SEARCH_RULE is needed.

>var setupModule = function() {

Please use "function setupModule(module) {"

>  controller = mozmill.newBrowserController(); // Force Mozmill to open a new Browser Window and get a controller for it.

Please place the comment a line above (not inline).

>var teardownModule = function() {

Please use "function teardownModule(module) {"

>  if (controller && controller.window)
>    controller.window.close();
>    
>  if (controller2 && controller2.window)
>    controller2.window.close();
>}

Please add a comment describing what you are checking for here.

>var testSearchFilter = function() {

Please use "function testSearchFilter() {"
Also, please add a /* */ style comment describing the test

>  // Head to Yahoo, and wait for page to load

Comment needs to be updated since you aren't going to Yahoo

>  controller.open(SEARCH_URL);
>  controller.waitForPageLoad();

Simply use LOCAL_TEST_PAGE.

>  var SEARCH_TITLE = controller.window.document.title;

Only constants should be named in ALL_CAPS.
Also, don't declare a variable until you are going to use it.
  
>  // Search for Yahoo

Please update this comment

>  var searchButton= activeTabView.getElement({type:'search_button'});
>  var searchBox = activeTabView.getElement({type:'search_box'});

You should not try to get the search_box until after you've clicked search_button.

>  // Type "Yahoo" for search

Please update this comment

>  // Check that the Search Box field contains the right search string
>  controller2.assertValue(searchBox, SEARCH_STRING);

You don't need to do this since controller.type() is an assertion.

>  
>  // Check that matching tabs appear in Tabs From Other Windows
>  controller2.assert(function() {
>    searchMatch = activeTabView.getElement({type:"search_match"});
>    searchMatch_title = searchMatch.getNode().textContent;

Both of these should be preceded with "var".
Also, you should declare searchMatch outside of the assertion.

>    return searchMatch_title == SEARCH_TITLE;

Make sure you use the triple operand: ===
Also, you can simpify this to "searchMatch.getNode().textContent === SEARCH_TITLE"
   
>  controller2.assert(function() {
>    return activeWin.document.title == SEARCH_TITLE;

Make sure you use the triple operand: ===

>    },"Searched-for tab has now been displayed: got "+ activeWin.document.title +" , expected " + SEARCH_TITLE + ".");

Make sure you include single quotes in your error message for got and expected values:
" got '" + variable + "', expected '" + variable + "'");

Comment 4

[adam.fishhook+bugzilla]

Attachment: Revised WIP Automation Test Script

Made changes as advised.

Comment 5

[u279076]

(In reply to comment #4)
> Created attachment 519431 [details]
> Revised WIP Automation Test Script
> 
> Made changes as advised.

Please remember to set the mime type to "text/plain" when attaching any .js files to a bug.  Thanks.

Comment 6

[u279076]

Comment on attachment 519431 [details]
Revised WIP Automation Test Script

>function teardownModule(module) {
>  // Check that controller and it's corresponding browser exists before closing
>  if (controller && controller.window)
>    controller.window.close();
>    
>  // Similar as above, this then completes the closing of both new browser windows we opened for the test
>  if (controller2 && controller2.window)
>    controller2.window.close();
>}

You could simplify this down to a single comment for both IFs:
"Make sure both windows are closed before exiting the test"

>/*
> * Test Searching of Tabs across two browser windows
> */

Please add a * to the first line: /**
    
>  // Open a new browser window
>  controller.waitFor(function () {
>    controller2 = mozmill.newBrowserController();
>    return !!controller2;
>  }, "A new browser window has been opened");

I'm a bit confused with this. In the setup module, you set controller to be a new window (should it not refer to the main window)? Then again you set controller2 to refer to a new window (conceivably this would mean having 3 windows with no controller pointing to the main window)?
Also, controller2 should be declared outside the waitFor, otherwise you could get many windows.
  
>  // Enter Tabview
>  activeTabView.open();

Can you reword this comment to "Open the Tab Groups view"?

>  var searchButton= activeTabView.getElement({type:'search_button'});
>  
>  // Get the Search Button element and click
>  controller2.click(searchButton);

Can you put these two lines of code together under the comment?

>  // Get the document title of the first browser to check with search results
>  var SEARCH_TITLE = controller.window.document.title;

I don't think this variable is necessary, simply use controller.window.document.title in the assert.
Also, ALL_CAPS is reserved for constants -- this variable is not a constant.

>  // Check that the Search Match element is accurate
>  controller2.assert(function() {
>    return searchMatch.getNode().textContent === SEARCH_TITLE;
>    });

Make sure the closing brace is inline with the 'c' in controller.
Also, you forgot to include an assertion message.

Finally, the next time, please attach an actual patch (not the .js file itself).  If you need help with this, please just ask.

Thanks.

Comment 7

[adam.fishhook+bugzilla]

(In reply to comment #6)

> >  // Open a new browser window
> >  controller.waitFor(function () {
> >    controller2 = mozmill.newBrowserController();
> >    return !!controller2;
> >  }, "A new browser window has been opened");
> 
> I'm a bit confused with this. In the setup module, you set controller to be a
> new window (should it not refer to the main window)? Then again you set
> controller2 to refer to a new window (conceivably this would mean having 3
> windows with no controller pointing to the main window)?
> Also, controller2 should be declared outside the waitFor, otherwise you could
> get many windows.

I wrote the test script, with the impression that there might be windows already opened. Hence, I opted to set controllers to new browser windows. 

Also, the litmus test says to open a new browser window. By using newBrowserController, I was trying to ensure that we open a new browser window regardless if there is already a window available.

I might be interpreting the litmus test scenario wrongly. Please advice on how it should be.
Scenario 1 - During the test, only two windows will be available. i.e all previous windows will be closed first, 

Scenario 2 - During the test, regardless of number of windows, we start by opening up a new browser window (as we are required to open a new browser, with a single tab to navigate to the test site)

> 
> >  // Get the document title of the first browser to check with search results
> >  var SEARCH_TITLE = controller.window.document.title;
> 
> I don't think this variable is necessary, simply use
> controller.window.document.title in the assert.
> Also, ALL_CAPS is reserved for constants -- this variable is not a constant.

I apologize for using caps here. It escaped my noticed. 

The reason why I created the variable search_title was more so for clarity. It'll allow other readers to understand that we are checking against the search_title. It is used more than once, and especially in the last assertion, I thought it'll provide a better picture as opposed to activeWin.document.title === controller.window.document.title

But of course, it could be changed. What say you?

Comment 8

[u279076]

(In reply to comment #7)
> I wrote the test script, with the impression that there might be windows
> already opened. Hence, I opted to set controllers to new browser windows. 

We should never have more than the main window at the beginning of a test.  Any test which opens a new window needs to take care of making sure that window is closed before it destructs.

controller should always refer to the main window.
controllerN should always refer to N other windows.


> The reason why I created the variable search_title was more so for clarity.
> It'll allow other readers to understand that we are checking against the
> search_title.

That's fair -- simply use searchTitle instead of SEARCH_TITLE.  I think this should be ok.

Comment 9

[adam.fishhook+bugzilla]

Attachment: Automation Test Script (Revision 2)

Made corrections and advised changes.
Primarily in how the setupModule was written.

Comment 10

[u279076]

Comment on attachment 520472 [details] [diff] [review]
Automation Test Script (Revision 2)

Be sure to add a newline to the end of your test.  With that change r=me.

Over to Henrik for follow up. Adam, please wait for Henrik's review before making any further changes.

Thanks.

Comment 11

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

Comment on attachment 520472 [details] [diff] [review]
Automation Test Script (Revision 2)

>+++ b/tests/functional/testTabView/testSearchFilter.js
>+ * Portions created by the Initial Developer are Copyright (C) 2010

Please change it to 2011.

>+ * Contributor(s):
>+ * Zaki Juhari <[e-mail]> (original author)

Add two spaces for indentation.

>+const TIMEOUT = 5000;

That's not needed anymore.

>+function teardownModule(module) {
>+  //Make sure both windows are closed before exiting the test
>+  if (controller && controller.window)
>+    controller.window.close();

Why do you also close the initial browser window?

>+  // Open a new browser window
>+  controller2 = mozmill.newBrowserController();

We do not want to use that method of mozmill. Please check the testTabbedBrowsing/testNewWindow.js test how to create a new window via the UI.

>+  // Check that the Search Match element is accurate
>+  controller2.assert(function() {

nit: add a space after function and the brackets.

>+    return searchMatch.getNode().textContent === searchTitle;
>+  },"Main search result: got '" + searchMatch.getNode().textContent + "' expected '" + searchTitle + "'.");

What is searchMatch in detail? Is it the first result of another window? What happens when multiple results have been found? Also just a nit bug keep our style like ' - got 'xyz', expected 'yzx'.

>+  mozmill.utils.waitFor(function () {
>+    return activeWin.documentLoaded;
>+  }, "Window content has been loaded.");

There is no need for this waitFor. We only switch between windows but do not create a new one.

>+  controller2.assert(function() {

nit: space after function

>+    return activeWin.document.title === searchTitle;
>+  },"Searched-for tab has now been displayed: got '"+ activeWin.document.title +"' , expected '" + searchTitle + "'.");

To make this test 100% match the Litmus test we should have two initial pages loaded. The search term should contain the title of the non-focused tab of the first window. Clicking this entry should bring the initial window and the background tab into focus.

Comment 12

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

Bug 836758 will remove Panorama from Firefox soon and make it available as add-on. That means no new tests are necessary. Closing as WONTFIX.