Mozmill test for (un)pinning app tabs

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: ashughes, Assigned: Shriram (irc: Mavericks))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-functional][mozmill-apptabs])

Attachments

(1 attachment, 18 obsolete attachments)

3.47 KB, patch
geo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 535820 [details]
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?
Attachment #535820 - Flags: feedback+
(Reporter)

Comment 2

7 years ago
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.
Attachment #535820 - Attachment mime type: application/octet-stream → text/plain
Attachment #535820 - Flags: feedback+ → feedback?(anthony.s.hughes)
(Reporter)

Updated

7 years ago
Attachment #535820 - Flags: feedback?(anthony.s.hughes) → feedback+
(Assignee)

Comment 3

7 years ago
Created attachment 535857 [details]
WIP Test pinning and unpinning tab
Attachment #535820 - Attachment is obsolete: true
Attachment #535857 - Flags: feedback?
(Reporter)

Comment 4

7 years ago
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.
Attachment #535857 - Flags: feedback? → feedback+
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!
(Reporter)

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
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?
(Reporter)

Comment 8

7 years ago
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
(Assignee)

Comment 9

7 years ago
Created attachment 537798 [details]
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.
Attachment #535857 - Attachment is obsolete: true
Attachment #537798 - Flags: feedback?

Updated

7 years ago
Attachment #537798 - Flags: feedback? → feedback?(anthony.s.hughes)
(Reporter)

Comment 10

7 years ago
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.
Attachment #537798 - Flags: feedback?(anthony.s.hughes) → feedback-
(Assignee)

Comment 11

7 years ago
Created attachment 538004 [details]
Tests pinning and unpinning a tab

I've made appropriate changes, and added a few asserts.
Attachment #537798 - Attachment is obsolete: true
Attachment #538004 - Flags: feedback?(anthony.s.hughes)
(Reporter)

Comment 12

7 years ago
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.
Attachment #538004 - Flags: feedback?(anthony.s.hughes) → feedback-
(Assignee)

Comment 13

7 years ago
Created attachment 538238 [details]
Tests pinning and unpinning tab

Refactored the two asserts, and added appropriate message for each of the asserts.
Modified the comments section.
Attachment #538004 - Attachment is obsolete: true
Attachment #538238 - Flags: feedback?(anthony.s.hughes)
(Reporter)

Comment 14

7 years ago
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.
Attachment #538238 - Flags: feedback?(anthony.s.hughes) → feedback-
(Assignee)

Comment 15

7 years ago
Created attachment 538433 [details]
Tests pinning and unpinning tab

Made changes to the asserts, and used TabBrowser
Attachment #538238 - Attachment is obsolete: true
Attachment #538433 - Flags: feedback?(anthony.s.hughes)
(Assignee)

Comment 16

7 years ago
Created attachment 538478 [details]
Tests pinning and unpinning tab

Re-worded / Rectified the message in each of the assert.
Attachment #538433 - Attachment is obsolete: true
Attachment #538433 - Flags: feedback?(anthony.s.hughes)
Attachment #538478 - Flags: feedback?(anthony.s.hughes)
(Reporter)

Comment 17

7 years ago
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.
Attachment #538478 - Flags: feedback?(anthony.s.hughes) → feedback-
(Assignee)

Comment 18

7 years ago
Created attachment 538584 [details]
Tests pinning and unpinning tab

Removed usage of prototype, and a couple of asserts
Attachment #538478 - Attachment is obsolete: true
Attachment #538584 - Flags: feedback?(anthony.s.hughes)
(Reporter)

Comment 19

7 years ago
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'"
Attachment #538584 - Flags: feedback?(anthony.s.hughes) → feedback-
(Assignee)

Comment 20

7 years ago
Created attachment 539717 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing
Attachment #538584 - Attachment is obsolete: true
Attachment #539717 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 22

7 years ago
Created attachment 539722 [details] [diff] [review]
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
Attachment #539717 - Attachment is obsolete: true
Attachment #539717 - Flags: review?(anthony.s.hughes)
Attachment #539722 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 23

7 years ago
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...
Attachment #539722 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 24

7 years ago
Created attachment 539976 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Made the appropriate changes.
Attachment #539722 - Attachment is obsolete: true
Attachment #539976 - Flags: review?(anthony.s.hughes)
(Assignee)

Comment 25

7 years ago
Created attachment 541007 [details]
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.
Attachment #539976 - Attachment is obsolete: true
Attachment #539976 - Flags: review?(anthony.s.hughes)
Attachment #541007 - Flags: feedback?(anthony.s.hughes)
(Assignee)

Updated

7 years ago
Depends on: 666209
(Assignee)

Updated

7 years ago
No longer depends on: 666209
Depends on: 666209
(Reporter)

Comment 26

7 years ago
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.
Attachment #541007 - Flags: feedback?(anthony.s.hughes)
(Reporter)

Comment 27

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

Comment 28

7 years ago
Created attachment 541281 [details] [diff] [review]
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 )
Attachment #541007 - Attachment is obsolete: true
Attachment #541281 - Flags: review?(anthony.s.hughes)
(Reporter)

Comment 29

7 years ago
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.
Attachment #541281 - Flags: review?(gmealer)
Attachment #541281 - Flags: review?(anthony.s.hughes)
Attachment #541281 - Flags: review+
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..."
(Assignee)

Comment 31

7 years ago
Created attachment 542554 [details] [diff] [review]
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.
Attachment #541281 - Attachment is obsolete: true
Attachment #541281 - Flags: review?(gmealer)
Attachment #542554 - Flags: review?(gmealer)
(Assignee)

Comment 32

7 years ago
Created attachment 542555 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing
Attachment #542555 - Flags: review?
(Assignee)

Comment 33

7 years ago
Created attachment 542556 [details] [diff] [review]
Patch for adding (un)pinning tabs test to testTabbedBrowsing

Added a line just above the assert for unpinning app tab
Attachment #542554 - Attachment is obsolete: true
Attachment #542555 - Attachment is obsolete: true
Attachment #542554 - Flags: review?(gmealer)
Attachment #542555 - Flags: review?
Attachment #542556 - Flags: review?(gmealer)
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 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.
Attachment #542556 - Flags: review?(gmealer) → review-
(Assignee)

Comment 36

7 years ago
Created attachment 543103 [details] [diff] [review]
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.
Attachment #542556 - Attachment is obsolete: true
Attachment #543103 - Flags: review?(gmealer)
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.
Attachment #543103 - Flags: review?(gmealer) → review-
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!
(Assignee)

Comment 39

7 years ago
(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"
(Reporter)

Comment 40

7 years ago
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.
(Assignee)

Comment 41

7 years ago
Created attachment 543367 [details] [diff] [review]
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.
Attachment #543103 - Attachment is obsolete: true
Attachment #543367 - Flags: review?(gmealer)
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() {
Attachment #543367 - Flags: review?(gmealer) → review-

Comment 43

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/15285213
(Assignee)

Comment 44

7 years ago
Created attachment 543580 [details] [diff] [review]
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.
Attachment #543367 - Attachment is obsolete: true
Attachment #543580 - Flags: review?(gmealer)
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.
Attachment #543580 - Flags: review?(gmealer) → review+
Keywords: checkin-needed
(Reporter)

Comment 46

7 years ago
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)
Attachment #543580 - Attachment description: Patch for adding (un)pinning tabs test to testTabbedBrowsing → Patch for pinning tabs [checked-in]
(Reporter)

Comment 47

7 years ago
Landed across all branches. Thanks Shriram for the excellent work on this test.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 48

7 years ago
:) 
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.
(Assignee)

Comment 49

7 years ago
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.
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill-functional][mozmill-apptabs]
(Reporter)

Updated

7 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.