479500 - Mozmill smoketest case writing assigned to Tracy

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Tracy Walker [:tracy]]

The following are test cases are Tracy's responsibility to write.  Tracked here: http://spreadsheets.google.com/ccc?key=pAP5Y5AH3-Tl-wRoNgBujUQ&inv=twalker%40mozilla.com&t=9216424536268796727

Filing this bug to track review and check-in of each test case as I complete them.

Comment 1

[Tracy Walker [:tracy]]

Attachment: Topsite Yahoo test case

Comment 2

[Tracy Walker [:tracy]]

Attachment: Home Button test case

Comment 3

[Tracy Walker [:tracy]]

Attachment: Open New Window test case

Comment 4

[Tracy Walker [:tracy]]

Attachment: New Tab test case

Comment 5

[Tracy Walker [:tracy]]

Attachment: Change Home Page and Restore to Default Home Page

Comment 6

[Tracy Walker [:tracy]]

Attachment: Navigate FTP test case

Comment 7

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

Comment on attachment 363361 [details]
Topsite Yahoo test case

>  // check for the yahoo logo
>let yahooLogo = new elementslib.ID(controller.tabs.activeTab, "ylogo");

Looks like the indentation was lost?

>  controller.waitForElement(yahooLogo);
>  controller.assertNode(yahooLogo);

What about using the same delayedAssertNode function I use in my Amazon test?
http://hg.mozilla.org/qa/mozmill-tests/rev/ff329b4c0661#l53

Comment 8

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

Comment on attachment 363362 [details]
Home Button test case

>
>var testHomeButton = function () {

Could you put a comment in-front of the function which tells about the test and the litmus test id? See http://hg.mozilla.org/qa/mozmill-tests/rev/ff329b4c0661#l78

>  // click the home button and check the default home page is loaded
>  controller.click(new elementslib.ID(controller.window.document, "home-button"));
>  controller.waitForPageLoad(controller.tabs.activeTab);
>  var searchField = new elementslib.ID(controller.tabs.activeTab, "sf");
>  controller.waitForElement(searchField);
>  controller.assertNode(searchField);
>}

Mmh, where do you check that the default home page is loaded? Shouldn't you better compare the loaded url with the one is given in the preference dialog?

Comment 9

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

Comment on attachment 363364 [details]
Open New Window test case

>  // checks that the new window is opened with homepage
>  var searchField = new elementslib.ID(controllerA.tabs.activeTab, "q");
>  controllerA.waitForElement(searchField);
>  controllerA.assertNode(searchField);

Same missing comparison with the preferences home page setting. I think we should better compare the url instead of the text field.

Comment 10

[cmtalbert]

Comment on attachment 363361 [details]
Topsite Yahoo test case

Functionally the test looks good, taking into account the comments from Henrik.  I have a couple of style nits in addition however that I'd like to see addressed.

This is the wrong style of license header to use - you can use the one that begins with *'s for javascript: http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

>// The Initial Developer of the Original Code is
>// Adam Christian.
I think you are the original developer of this test case, not Adam.

>  // check for the yahoo logo
>let yahooLogo = new elementslib.ID(controller.tabs.activeTab, "ylogo");
Style nit: up to this point you were using ' quotes.  Here you use " quotes.  They are both valid and mean the same thing in JS, but try to only use one style per file to make it easier to read.

>  // check that the search field exists
>let searchField = new elementslib.ID(controller.tabs.activeTab, "p");
>  controller.waitForElement(searchField);
>  controller.assertNode(searchField);
Style nit: I think Henrik mentioned this, but keep the indentation consistent at 2 space indenting. 

Going to minus this one.

Comment 11

[cmtalbert]

Comment on attachment 363366 [details]
Change Home Page and Restore to Default Home Page


>var testChangeHomePage = function () {
>  // ensure the current page is not the default homepage
>  controller.open('http://www.mozilla.org');
>  controller.waitForPageLoad(controller.tabs.activeTab);
>  // set the homepage to the current page
>  var prefController = mozmill.getPreferencesController();
>  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));
>  prefController.click(new elementslib.ID(prefController.window.document, "useCurrent"));
>  // looks bizarre, but this is what recorder gave me for an sending esc keypress to the Pref window
>  prefController.keypress(new elementslib.ID(prefController.window.document, "useCurrent"),27,false,false,false,false);
This code here is a bit difficult to parse visually. Would it be ok to have a space between the comment and the lines above it?

>  // Check that the homepage was actually set to the previous
the previous what? It's a bit unclear.

Comment 12

[cmtalbert]

Comment on attachment 363367 [details]
Navigate FTP test case


>  controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "latest-trunk/"))
it's not required, but it's a good idea to include a semicolon on the last line.

Comment 13

[cmtalbert]

Overall, these look like good tests.  I'd second Henrik's suggestion to verify the URL of the home page in addition to the checks that you are already doing (if it's possible to pull the URL out of the preference system or the preference dialog and compare it to what is in the location bar field.  In addition, all the tests will need new license headers in the correct format and listing you as the original author of the code.

Comment 14

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

Ok, an easy access to the prefbranch we will get with bug 479571. Adding it to the dependency list.

Comment 15

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

Comment on attachment 363365 [details]
New Tab test case

>  // opens a new tab, by default it should open with an a blank (Untitled) page
>  controller.click(new elementslib.Elem(controller.menus.File["New Tab"]));

Please use menu id's here: controller.menus['file-menu'].menu_newNavigatorTab)

>  controller.click(new elementslib.Elem(controller.menus.File["Close Tab"]));

Same here: controller.menus['file-menu'].menu_close)

This way we have locale independent tests.

Comment 16

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

Comment on attachment 363364 [details]
Open New Window test case

Please always use id's here too:

>  controller.click(new elementslib.Elem(controller.menus.File["New Window"]));

controller.menus['file-menu'].menu_newNavigator

>  controllerA.click(new elementslib.Elem(controllerA.menus.File["Close Window"]));

controller.menus['file-menu'].menu_closeWindow

All available menu id's you can find here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc

Comment 17

[Tracy Walker [:tracy]]

(In reply to comment #16)
> (From update of attachment 363364 [details])
> Please always use id's here too:
> 
> >  controller.click(new elementslib.Elem(controller.menus.File["New Window"]));
> 
> controller.menus['file-menu'].menu_newNavigator
> 
> >  controllerA.click(new elementslib.Elem(controllerA.menus.File["Close Window"]));
> 
> controller.menus['file-menu'].menu_closeWindow
> 
> All available menu id's you can find here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc

I want to test the click event.  Calling menu items that way doesn't do that. Since we are writing these to emulate a user as closely as this tool will allow, we should be using the method that tests the click. Please see http://code.google.com/p/mozmill/wiki/MenuAPI

Comment 18

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

(In reply to comment #17)
> > >  controller.click(new elementslib.Elem(controller.menus.File["New Window"]));
> > 
> > controller.menus['file-menu'].menu_newNavigator
>
> I want to test the click event.  Calling menu items that way doesn't do that.
> Since we are writing these to emulate a user as closely as this tool will
> allow, we should be using the method that tests the click. Please see
> http://code.google.com/p/mozmill/wiki/MenuAPI

Oh, you should only exchange the function parameter. That's all. It still checks the click event:

controller.click(new elementslib.Elem(controller.menus['file-menu'].menu_newNavigator));

Comment 19

[cmtalbert]

Comment on attachment 363362 [details]
Home Button test case

Clearing the old review requests.  Please let me know when you have a new patch available.

Comment 20

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

Tracy please also update the license plate to the new format like it was done on bug 479571.

Comment 21

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

Comment on attachment 363362 [details]
Home Button test case

>// ***** BEGIN LICENSE BLOCK *****
>// Version: MPL 1.1/GPL 2.0/LGPL 2.1
>//

Can you use the comment style like we are using in the other tests already?
http://hg.mozilla.org/qa/mozmill-tests/file/fd0cd7b00bdd/firefox/bookmarks/test_addBookmarkToMenu.js

>// The Original Code is Mozilla Corporation Code.
>//
>// The Initial Developer of the Original Code is
>// Adam Christian.

Both lines are wrong. See the above link for details.

>var testHomeButton = function () {

Please add a comment right before the function so we know which Litmus test it is.

>  // ensure the current page is not the default homepage
>  controller.open('http://www.mozilla.org');
>  controller.waitForPageLoad(controller.tabs.activeTab);

Can you please check for an element in this page too due to the bug in waitForPageLoad?

>  // click the home button and check the default home page is loaded
>  controller.click(new elementslib.ID(controller.window.document, "home-button"));
>  controller.waitForPageLoad(controller.tabs.activeTab);
>  var searchField = new elementslib.ID(controller.tabs.activeTab, "sf");
>  controller.waitForElement(searchField);
>  controller.assertNode(searchField);

We should also make sure that the page we have loaded is the URL given in the preferences pane. But at least I wonder if we need this test at all in favor of attachment 363366 [details] which covers a wider area and also includes this check.

Comment 22

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

Comment on attachment 363367 [details]
Navigate FTP test case

>// ***** BEGIN LICENSE BLOCK *****
>// Version: MPL 1.1/GPL 2.0/LGPL 2.1
>//

Please correct the comment style.

>var testNavigateFTP = function () {

And add a comment before the function which includes the litmus testcase number and summary

>  // opens the mozilla.org ftp page then navigates through a couple levels.
>  controller.open('http://ftp.mozilla.org/pub/');

Please use the FTP protocol and not HTTP

>  controller.waitForPageLoad(controller.tabs.activeTab);
>  controller.click(new elementslib.Link(controller.tabs.activeTab, "firefox/"));
>  controller.waitForPageLoad(controller.tabs.activeTab);
>  controller.click(new elementslib.Link(controller.tabs.activeTab, "nightly/"));
>  controller.waitForPageLoad(controller.tabs.activeTab);
>  controller.assertNode(new elementslib.Link(controller.tabs.activeTab, "latest-trunk/"))

Here we need delayed clicks and assertNode. See my latest patch for the Amazon test on bug 476236.

Comment 23

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

Comment on attachment 363366 [details]
Change Home Page and Restore to Default Home Page

>// ***** BEGIN LICENSE BLOCK *****
>// Version: MPL 1.1/GPL 2.0/LGPL 2.1

Comment style here too.

>var testChangeHomePage = function () {

A comment before this function please.

>  // ensure the current page is not the default homepage
>  controller.open('http://www.mozilla.org');
>  controller.waitForPageLoad(controller.tabs.activeTab);

Please add a line to wait for an element due to the bug in waitForPageLoad.

>  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));

Please use the lookup method here which makes it also run with localized builds.

>  // looks bizarre, but this is what recorder gave me for an sending esc keypress to the Pref window
>  prefController.keypress(new elementslib.ID(prefController.window.document, "useCurrent"),27,false,false,false,false);

Can you use the window itself as element instead of the textfield please?

>  // Check that the homepage was actually set to the previous

previous what? Add e.g. "previous opened page"?

>var testResetHomepageDefault = function () {

A comment in-front of this function would be great too.

>  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));

See above.

>  // send esc keypress to the Pref window
>  prefController.keypress(new elementslib.ID(prefController.window.document, "restoreDefaultHomePage"),27,false,false,false,false);

See above.

>  var testLink = new elementslib.Link(controller.tabs.activeTab, "Shiretoko Project Page");
>  controller.waitForElement(testLink);
>  controller.assertNode(testLink);

Can you please use another element? This one will only work with 3.5 branch builds but not with Minefield.

Comment 24

[Tracy Walker [:tracy]]

(In reply to comment #23)
> (From update of attachment 363366 [details])
> >// ***** BEGIN LICENSE BLOCK *****
> >// Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> Comment style here too.
> 
> >var testChangeHomePage = function () {
> 
> A comment before this function please.
> 
> >  // ensure the current page is not the default homepage
> >  controller.open('http://www.mozilla.org');
> >  controller.waitForPageLoad(controller.tabs.activeTab);
> 
> Please add a line to wait for an element due to the bug in waitForPageLoad.

I don't think we should be blanket changing away from waitForPageLoad just because it fails on a few random sites.   I still maintain it's a Firefox bug that it doesn't completely resolve at sites like MSN.  Looking for elements in the loading page, will in the long run be susceptible to more breakage if/when the element changes.

If waitForPageLoad doesn't seem to work in a particular test case, then use the work-a-round.
> 
> >  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));
> 
> Please use the lookup method here which makes it also run with localized
> builds.
> 

what lookup method is that?

> >  // looks bizarre, but this is what recorder gave me for an sending esc keypress to the Pref window
> >  prefController.keypress(new elementslib.ID(prefController.window.document, "useCurrent"),27,false,false,false,false);
> 
> Can you use the window itself as element instead of the textfield please?

The comment there is incorrect.  the element is a button that must be pressed.

> 
> >  // Check that the homepage was actually set to the previous
> 
> previous what? Add e.g. "previous opened page"?
> 
> >var testResetHomepageDefault = function () {
> 
> A comment in-front of this function would be great too.
> 
> >  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));
> 
> See above.
> 
> >  // send esc keypress to the Pref window
> >  prefController.keypress(new elementslib.ID(prefController.window.document, "restoreDefaultHomePage"),27,false,false,false,false);
> 
> See above.
> 
> >  var testLink = new elementslib.Link(controller.tabs.activeTab, "Shiretoko Project Page");
> >  controller.waitForElement(testLink);
> >  controller.assertNode(testLink);
> 
> Can you please use another element? This one will only work with 3.5 branch
> builds but not with Minefield.


Also, why use delayed clicks? Are you again working around waitForPageLoad?

Comment 25

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

(In reply to comment #24)
> > >  // ensure the current page is not the default homepage
> > >  controller.open('http://www.mozilla.org');
> > >  controller.waitForPageLoad(controller.tabs.activeTab);
> > 
> > Please add a line to wait for an element due to the bug in waitForPageLoad.
> 
> I don't think we should be blanket changing away from waitForPageLoad just
> because it fails on a few random sites.   I still maintain it's a Firefox bug
> that it doesn't completely resolve at sites like MSN.  Looking for elements in
> the loading page, will in the long run be susceptible to more breakage if/when
> the element changes.

It doesn't only happen on random site. Depending on the network connection you will see this each time you open a web page. Right now I have cc'ed bzbarsky to bug 475797 and asked him how we could improve this function. If we can fix it, your concerns are correct. So leave it as it is right now.

> > >  prefController.click(new elementslib.Elem(prefController.tabs.Main.button));
> > 
> > Please use the lookup method here which makes it also run with localized
> > builds.
> > 
> 
> what lookup method is that?

See bug 488823 for details.

> > >  // looks bizarre, but this is what recorder gave me for an sending esc keypress to the Pref window
> > >  prefController.keypress(new elementslib.ID(prefController.window.document, "useCurrent"),27,false,false,false,false);
> > 
> > Can you use the window itself as element instead of the textfield please?
> 
> The comment there is incorrect.  the element is a button that must be pressed.

No, it isn't. Keycode 27 is Escape and should be better fired on the window itself.

> Also, why use delayed clicks? Are you again working around waitForPageLoad?

I thought that we have to take the opening time for additional windows into account. But it looks like that it is already done inside the get controller function. Mikeal, is that correct?

Comment 26

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

Everything has been checked in. Closing this bug.