648380 - Mozmill test for (un)pinning app tabs

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

https://litmus.mozilla.org/show_test.cgi?id=12094
https://litmus.mozilla.org/show_test.cgi?id=12767

Tracking bug for the creation of an automated test for pinning/unpinning App tabs, as per the above Litmus testcases. I think it makes the most sense to cover both of these in a single test module.

Shriram, our newest Mozmill contributor, has agreed to work on this test.

Comment 1

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

For inclusion of appropriate shared module, I will have to determine the RELATIVE_ROOT which's dependent on the location of this file.
Does this go in the mozmill/tests folder?

Comment 2

[u279076]

Comment on attachment 535820 [details]
Tests pinning and unpinning tab

Make sure the mime type is set to text/plain if you are attaching a work in progress .js file. Otherwise it can't be viewed.  Also, if you are asking for feedback, the flag is feedback?

As far as the file itself is concerned, you can put it in mozmill-tests/tests/functional/testTabbedBrowsing/

RELATIVE_ROOT is no longer used, you can simply include the necessary modules. See http://hg.mozilla.org/qa/mozmill-tests/file/2853bea9812c/tests/functional/testTabbedBrowsing/testNewTab.js#l42 as an example.

Comment 3

[Shriram (irc: Mavericks)]

Attachment: WIP Test pinning and unpinning tab

Comment 4

[u279076]

Comment on attachment 535857 [details]
WIP Test pinning and unpinning tab

>function(module){
>	controller  = mozmill.getBrowserController();
>}

This should go in the function setupModule(module).

>var setupTest = function(test) {
>	var newElement = new elementslib.Elem(
>						 controller.menus['file-menu']['menu_newNavigatorTab']);
>	controller.click(newElement);
>	controller.open('http://www.nytimes.com');
>	//create a contextMenu instance
>	contextMenu = controller.getMenu("#tabContextMenu");
>}

This should be part of the test method.
newElement should be given a more meaningful name (ie. newTabMenuItem)
You should use a local test page instead of nytimes.com. Use any of the pages in mozmill-tests/data/layout/

>var teardownTest = function(test) {
>	contextMenu.close();
>	var newElement = new elementslib.Elem(controller.menus['file-menu']['menu_close']);
>	controller.click(newElement);
>	//Close the tab
>}

This should be called teardownModule(module). I think the only thing you need to do in this method should be closeAllTabs() -- check the TabBrowser shared module.

>function testTabPinningUnPinning() {
>	//select the tab
>	controller.tabs.selectTabIndex(0);
>	//get the context menu of the tab
>	contextMenu = controller.getMenu('#tabContextmenu');
>	//select (and not right click) to pin it as app tab
>	contextMenu.select('#context_pinTab');
>	//select (and not right click) to unpin it
>	contextMenu.select('#context_unpinTab');
>}

In general, setupModule() is used to instantiate the main controller and to ensure a clean state before the test starts. teardownModule() is used to ensure a clean state after the test finishes. All other code should be contained in your test method.

Comment 5

[Aaron Train [:aaronmt]]

Hi Shriram,

Not to hijack Anthony's feedback/review, but an additional but important piece missing from your posted test are the need for some assertions in order to verify that things work as expected (in your case you would probably be testing for tab count, or number of pinned tabs both before and after, etc). This ensures that the test is actually testing for expected results akin to its manual Litmus companion (https://litmus.mozilla.org/show_test.cgi?id=12767)

Keep it up!

Comment 6

[u279076]

(In reply to comment #5)
> Hi Shriram,
> 
> Not to hijack Anthony's feedback/review, but an additional but important
> piece missing from your posted test are the need for some assertions in
> order to verify that things work as expected (in your case you would
> probably be testing for tab count, or number of pinned tabs both before and
> after, etc). This ensures that the test is actually testing for expected
> results akin to its manual Litmus companion
> (https://litmus.mozilla.org/show_test.cgi?id=12767)
> 
> Keep it up!

Thanks Aaron. Yes, you definitely need to put some assertions in there. Please use existing Mozmill tests as an example.

Comment 7

[Shriram (irc: Mavericks)]

Thanks Aaron and Anthony for your feedback/support. Highly appreciated. :)

I assumed that the state of the browser would be a single window with a single tab in it which's not the case all the time, for sure.
I'm trying to think of scenarios for application of assertions.
For instance, running a series of tests could result in a browser state with possibly more tabs(pinned/unpinned).
 
Should the test accommodate difference cases with multiple tabs in a window, and  multiple windows and multiple tabs per window ?

Would it rather help to open a new window with a single tab for this test alone, and close it without bothering existing windows/tabs(if there's any before starting the test)?

Is it recommended for the test to work with the existing state of the browser?

Comment 8

[u279076]

Sorry for the late reply. This test needs only test a single scenario, as follows:

1) Assume a single window with one blank tab
2) Open a new tab and load a test page in it
3) Pin that tab -- do the required assertions
4) Unpin that tab -- do the required assertions

Thanks

Comment 9

[Shriram (irc: Mavericks)]

Attachment: Test pinning and unpinning tab

I'm yet to complete the assertion part.
It looks like the I could use the ternary operator to replace the if and place it in the assert functions.

Comment 10

[u279076]

Comment on attachment 537798 [details]
Test pinning and unpinning tab

> * Portions created by the Initial Developer are Copyright (C) 2009

The year is 2011.

>var tabs = require("../../../lib/tabs");
>const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');

Please separate these two lines with a newline.

>function setupModule(module){
>	controller  = mozmill.getBrowserController();
>	tabBrowser = new tabs.tabBrowser(controller);
>}

Proper indentation is 2 spaces, not a tab.

>/**
> * Check with the Mozmill Forum post 
> * http://groups.google.com/group/mozmill-dev/browse_thread/thread/e9c88e4be37e1579
> * regarding usage of setupTest/teardownTest
> */

Remove this comment.

>var teardownModule = function(test) {

The only parameter you should ever use in setupModule and teardownModule is "module". Also, make it a named function like you did for setupModule().

>function testTabPinningUnPinning() {

This can be simplified to testAppTabPinning. You should rename the file to the same.
Also, please add a /** */ style comment indicating what the test is checking.

>	tabBrowser.openTab({type:"newTabButton"});
>	var newTabMenuItem = new elementslib.Elem(
>						 controller.menus['file-menu']['menu_newNavigatorTab']);
>	controller.click(newTabMenuItem);

You already opened a new tab with the NewTabButton. You don't need to open another one. Two open tabs should be enough for this test.
Also, watch your spacing/indentation.

>	contextMenu = controller.getMenu("#tabContextMenu");
>	//controller.assert( );
>	if( controller.tabs.length > 1 )
>		tabBrowser.selectedIndex = controller.tabs.length-1 
>	else
>		tabBrowser.selectedIndex = 0;

I'm assuming you are trying to check if the first tab is selected. You should do this via an assert, not an if().

>	//if( !this.pinned || this.unpinned )
>	//else // Or

Again, don't use if-else, always use assert() or waitFor(). You can see some of the more recently checked-in tests for examples on how they are used.

Thanks for the work so far -- keep it up.

Comment 11

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning a tab

I've made appropriate changes, and added a few asserts.

Comment 12

[u279076]

Comment on attachment 538004 [details]
Tests pinning and unpinning a tab

>/*
> * Tests pinning and unpinning a tab successfully in a single window
> * 
> */

Comment style should be:
/**
 * Your comment
 */

>  //open a new tab

Inline comment style should be:
// Your comment

>  controller.assert(function(){ return controller.tabs.length > 1 ;});

You should include a message and wrap this into multiple lines. See the following example:
http://hg.mozilla.org/qa/mozmill-tests/file/tip/tests/functional/testTabbedBrowsing/testNewTab.js#l98

Please refactor all of your asserts using this format.

Comment 13

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

Refactored the two asserts, and added appropriate message for each of the asserts.
Modified the comments section.

Comment 14

[u279076]

Comment on attachment 538238 [details]
Tests pinning and unpinning tab

> */
>
>function testAppTabPinning() {

Please remove the newline.

>  // open a new tab
>  var newTabMenuItem = new elementslib.Elem(controller.menus['file-menu']['menu_newNavigatorTab']);
>  controller.click(newTabMenuItem);
>  controller.assert(function(){ return controller.tabs.length > 1 ;});

Use tabBrowser.openTab()

>  // assume page's loaded

Just comment what you are checking in the following code, don't write "assumes"

>  // check whether it's sucessfully pinned
>  controller.assert( function(){
>    return ((controller.tabs.length > 1) && (controller.tabs.getTab(0).pinned));
>  }, "The tab was not successfully pinned" );

A couple things to note here...

1. Only assert one thing per assert
2. controller.tabs.getTab(0) should be assigned to a variable before assert
3. controller.tabs.length > 1 should be done before this assert
4. Message format should include a "got" and "expected". For example, "The tab has been pinned - got '" + some_variable + "', expected 'expected_value'".

>  tabBrowser.selectedIndex = 0;

>  controller.tabs.selectTabIndex(tabBrowser.selectedIndex);

This line is unnecessary -- tabBrowser.selectedIndex = 0 will set the active tab to the first tab by default.

>  // check whether it's successfully unpinned
>  controller.assert( function(){
>    return ((controller.tabs.length > 1) && (controller.tabs.getTab(0).unpinned));
>  }, "The tab was not successfully unpinned" );

Same comments for previous assert apply to this one.

Comment 15

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

Made changes to the asserts, and used TabBrowser

Comment 16

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

Re-worded / Rectified the message in each of the assert.

Comment 17

[u279076]

Comment on attachment 538478 [details]
Tests pinning and unpinning tab

>  tabBrowser.openTab({type: "newTabButton"});

Simply use tabBrowser.openTab("newTabButton");

>  controller.assert(function(){ return controller.tabs.length > 1 ;});
>  tabBrowser.selectedIndex = controller.tabs.length-1;

This is not necessary as it is inherent in openTab().

>  tabBrowser.prototype.controller.open(LOCAL_TEST_PAGE);
>  tabBrowser.prototype.controller.waitForPageLoad();

Never use prototype. If you are trying to use a method in TabBrowser, simply use tabBrowser.function() or tabBrowser.variable. If you are trying to do something with the active tab, use:
var tab = tabBrowser.getTab();

>  controller.assert( function(){ return (controller.tabs.length > 1);} );

Not necessary.

Comment 18

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

Removed usage of prototype, and a couple of asserts

Comment 19

[u279076]

Comment on attachment 538584 [details]
Tests pinning and unpinning tab

>function setupModule(module){

nit: space between ) and { for all function signatures.

>  var firstTab = tabBrowser.controller.tabs.getTab(0);

Instead of using firstTab, I think you can use tabBrowser.tabs[0] in your test.

>  controller.assert( function(){ 
>    return firstTab.unpinned;
>  }, "The tab has not been unpinned - got '" + firstTab.unpinned + 
>    "', expected 'true'." );
>}

Your message should reflect a positive state:
"The tab has been unpinned - got 'value', expected 'value'"

Comment 20

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Comment 21

[Shriram (irc: Mavericks)]

as per https://developer.mozilla.org/en/Mozmill_Tests
http://beaufour.dk/jst-review/ had no issues for the moment.

Comment 22

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

It has few errors when I run it through http://beaufour.dk/jst-review/
like Opening Braces on new line(although existing files don't have brace on new line), windows line endings, etc

Comment 23

[u279076]

Comment on attachment 539722 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

>+  tabBrowser.selectedIndex = controller.tabs.length-1;

This is unnecessary as the act of clicking the newtab button makes that tab active.

>+  // check whether it's sucessfully pinned
>+  controller.assert( function(){

Space between the ){

>+  // check whether it's successfully unpinned
>+  controller.assert( function(){

Same here...

Comment 24

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Made the appropriate changes.

Comment 25

[Shriram (irc: Mavericks)]

Attachment: Tests pinning and unpinning tab

Commented the asserts, as tab.pinned or unpinned doesn't work yet for as can be seen in tabs.js 
tab.pinned or unpinned gives an 'undefined value' and fails the test.

Filed a feature request for method to replace and return boolean value  @ https://bugzilla.mozilla.org/show_bug.cgi?id=666209

Other than that, test works as expected.
Among modifications, replaced controller.length-1 with tabBrowser.length-1 in for getting the Tab.

Comment 26

[u279076]

Comment on attachment 541007 [details]
Tests pinning and unpinning tab

Canceling feedback? for now -- please re-request if you need it once the dependency bug is resolved.

Comment 27

[u279076]

Shriram, you should now be able to use tabBrowser.isAppTab(tab) in your test (providing you update your repository).

Comment 28

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Added the appropriate asserts with the appropriate API method, and couldn't find a better word/for lack of better word for desired tab ( used currentTab variable )

Comment 29

[u279076]

Comment on attachment 541281 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

This looks good to me and tests okay. Over to Geo for sr.

Comment 30

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 541281 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Mostly very minor spacing issues, one issue with the last assert comment.

>+function setupModule(module) {
>+  controller  = mozmill.getBrowserController();
>+  tabBrowser = new tabs.tabBrowser(controller);
>+}

Minor, spacing of the controller assignment.

>+function testAppTabPinning() {
...
>+  currentTab = tabBrowser.getTab(tabBrowser.length-1);

Minor, please add spaces on either side of operators (length - 1).

>+  // check whether it's sucessfully pinned
>+  controller.assert( function() {
>+    return tabBrowser.isAppTab(currentTab);
>+  }, "This tab has been pinned - got '" + tabBrowser.isAppTab(currentTab) +
>+    "', expected 'true'" );

Minor, please remove space before function() and line up the second line of the string with the first line (one more space, so " is under ").

Also, optional, but this would be a little easier to read if you assigned tabBrowser.isAppTab() to a temp variable above the assert and used it in both places.

>+  // check whether it's successfully unpinned
>+  controller.assert( function() {
>+    return !tabBrowser.isAppTab(currentTab);
>+  }, "The tab has not unpinned - got '" + !tabBrowser.isAppTab(currentTab) +
>+    "', expected 'true'" );
>+}

Same formatting needs as above, no space before function() and line up the second line of the string.

Also repeat my suggestion of moving the !tabBrowser.isAppTab(currentTab) expression out of the block by assigning to a variable above the assert. Especially with the negation, it's starting to get complex and harder to tell what's being tested.

var isUnpinned = !tabBrowser.isAppTab(currentTab);
etc.

The assert comment needs to change; it's saying the opposite of what it actually is testing. Try "The tab is unpinned - got..."

Comment 31

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Took care of the minor changes.  
The review tool @  http://beaufour.dk/jst-review/ made me add spaces etc. 
Previously, I was trying to follow spacing on the existing test files which I guess's more appropriate.  
Made it a bit easier for reading.

Comment 32

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Comment 33

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Added a line just above the assert for unpinning app tab

Comment 34

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

Comment on attachment 542556 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

>+  controller.assert(function() {
>+    return appTabState;
>+  }, "This tab has been pinned - got '" + appTabState + "', expected 'true'" );

Only a small nit I want to call out in a drive-by check. You will have to use 'function ()' - see the additional space - in both cases. You do not specify a name for it, so using 'function()' looks like you are calling a function with the name function instead of having a function definition.

Comment 35

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 542556 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Changes look fine, thanks.

I probably would have gone with assigning !tabBrowser.isAppTab(currentTab) to a very specific variable. Basically, "appTabState" isn't nearly as clear as "appTabUnpinned" or "appTabPinned". The former could mean almost anything, the latter could only mean one thing. 

Plus, these types of assertions work better if you angle things so the result you're checking is true in the good case.

Not a necessary change, though, just a hint for the future. What you have is certainly better than what it was.

Henrik's review comment is accurate, please separate "function ()" where used. My bad for missing that.

Also just noticed, you need to declare all new variables inside your test function with var. Right now, you're making everything global. We should only do that for variables that need to be shared between setup, test, and teardown.

While you're in there, can you take out the extra space on the assertion messages? It's between the final end-quote and the parenthesis:

Ex: expected 'true'" );

Otherwise, stuff looks good. r-'ing for the changes, but should be an easy r+ once they're done.

Comment 36

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Thanks so much Henrik and Geo. 
I tried follow the format in some of the existing test files.
Other than that, I renamed the variables appTabState. 
It sounds more reasonable and more specific to the task as you said. 
Took care of the extra spaces as well.

Comment 37

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 543103 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Oops, one problem in the last verification block. I must have been confusing in my prior explanation, sorry about that!

>+  var appTabUnpinned = tabBrowser.isAppTab(currentTab);

See, that doesn't work, because it'll be true when it's pinned and false when it's unpinned, which is the exact opposite of what the name says it is. You have to negate it when you assign it, so that the value matches the name.

var appTabUnpinned = !tabBrowser.isAppTab(currentTab);

Now it's true when unpinned and false when pinned.

>+  // check whether it's successfully unpinned
>+  controller.assert(function () {
>+    return !appTabUnpinned;
>+  }, "The tab is unpinned - got '" + !appTabUnpinned + "', expected 'true'");

...and since it's now true in the passing case, you don't negate it here:

  // check whether it's successfully unpinned
  controller.assert(function () {
    return appTabUnpinned;
  }, "The tab is unpinned - got '" + appTabUnpinned + "', expected 'true'");

So just needs that really quick fix.

The rest looks great, and thanks for your hard work on this.

Comment 38

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

Comment on attachment 543103 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

>+++ b/tests/functional/testTabbedBrowsing/testUnPinningAppTab.js
[..]
>+function testAppTabPinning() {

Now that we are testing pinning and unpinning of app tabs you should also update the file name accordingly.

>+  //open a new Tab, load a Test Page and wait for it to load
>+  tabBrowser.openTab("newTabButton");

In this case we should default to the default action, which means simply remove the "newTabButton" from the parameter list and leave it empty. A parameter is only used in one of our test which explicitly checks the different access points.

>+  var appTabPinned = tabBrowser.isAppTab(currentTab);
>+  // check whether it's sucessfully pinned
>+  controller.assert(function () {

nit: To enhance the reading of the code block, just put in before the appTabPinned declaration. Same for unpinned. 

Otherwise I have to second Geo. Thanks for working on it!

Comment 39

[Shriram (irc: Mavericks)]

(In reply to comment #38)
> >+++ b/tests/functional/testTabbedBrowsing/testUnPinningAppTab.js
> [..]
> >+function testAppTabPinning() {
> 
> Now that we are testing pinning and unpinning of app tabs you should also
> update the file name accordingly.

Would testAppTabPinning be a proper filename? 
Although the name doesn't cover "unpinning in it"? 
Is it acceptable if it matches with the test function name, in this case, 
"testAppTabPinning"

Comment 40

[u279076]

In general, the test method name and file name should match. testUnPinningAppTab implies you are only testing the unpinning functionality. I would opt for testTabPinning for the name of both the file and the test method.

Comment 41

[Shriram (irc: Mavericks)]

Attachment: Patch for adding (un)pinning tabs test to testTabbedBrowsing

Thanks Henrik, Anthony, for the comments. I've changed to the appropriate filename.

Geo, That was silly of me, as it defeated the intention of using the proper variable name. It's taken care of.

Comment 42

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 543367 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Shriram, code looks great to me, but can you resubmit with the function name changed to match the new filename? The correlation helps us when we examine results.

from:

> function testAppTabPinning() {

to:

function testTabPinning() {

Comment 43

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15285213

Comment 44

[Shriram (irc: Mavericks)]

Attachment: Patch for pinning tabs [checked-in]

Initially,  I wanted to change it along with the filename of the test.
I looked at some of the existing test files, and some of them are exactly the same while the others have an extra word or two in the function name.

Anyways, I've changed the function name to testTabPinning as well.

Comment 45

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 543580 [details] [diff] [review]
Patch for pinning tabs [checked-in]

r+, Looks great to me. Sorry for the delay on this review (got sidetracked over the 7/4 weekend!) and thanks for hanging tough during all the cycling back and forth.

Comment 46

[u279076]

Comment on attachment 543580 [details] [diff] [review]
Patch for pinning tabs [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d629a001d0fa (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/53aae0e049e7 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b7349a3a8941 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/e50252355bda (mozilla-release)

Comment 47

[u279076]

Landed across all branches. Thanks Shriram for the excellent work on this test.

Comment 48

[Shriram (irc: Mavericks)]

:) 
Geo: Aaah, I see. It's been a couple of years since I've the experienced the 7/4 weekend. I know that feeling. Also, I just realized why a couple of folks I met @ Westin Hotel, Hyd (I've been to recently) were overjoyed. Hope you had a lot of fun.
Anthony: Thanks for all the support from the beginning.   
Thanks to Aaron and Henrik for answering my queries, and comments. 
It's been good fun, and more so a great learning experience. 
I hope to improve upon this, and look forward to more.

Comment 49

[Shriram (irc: Mavericks)]

Thanks Alex and Vlad too, for answering my queries on Mozmill through IRC
(In reply to comment #48)
> :) 
> Geo: Aaah, I see. It's been a couple of years since I've the experienced the
> 7/4 weekend. I know that feeling. Also, I just realized why a couple of
> folks I met @ Westin Hotel, Hyd (I've been to recently) were overjoyed. Hope
> you had a lot of fun.
> Anthony: Thanks for all the support from the beginning.   
> Thanks to Aaron and Henrik for answering my queries, and comments. 
> It's been good fun, and more so a great learning experience. 
> I hope to improve upon this, and look forward to more.