503411 - [mozmill] - Test the safebrowsing warning page

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aakash Desai [:aakashd]]

Attachment: testSafeBrowsing/testSafeBrowsingWarningPages.js

The test script runs through attack site and web forgery warning pages and tests the elements (get me out here, report and ignore warning buttons) of those pages. 


Litmus test case represented:
 *  Testcase ID #6988 - Test the safebrowsing warning page

Comment 1

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

Comment on attachment 387757 [details]
testSafeBrowsing/testSafeBrowsingWarningPages.js

> * The Original Code is Mozilla Mozmill Test Code.

Please remove Mozilla from that line. I missed that in all your former tests and already have updated them.

>var testSafeBrowsingWarningPages = function() {
>  // Go to Mozilla's its-a-trap page to bring up the Safebrowsing warning page
>  var badSites = ['http://www.mozilla.com/firefox/its-a-trap.html','http://www.mozilla.com/firefox/its-an-attack.html'];

Can you please wrap the second site to the next line. That makes it more readable. And you can remove the comment. It doesn't make sense at this place.

>    // Go to one of the mozilla phishing protection test page
>    controller.open(badSites[i]);
>    controller.waitForPageLoad();

I know a way how we can improve the waitForPageLoad call until this function is fixed for error pages. You can add two parameters. The second one will be the timeout. So set this value to lets say 1000. That should be enough. And please also correct the comment. Just say something like "Open one of the test phishing pages".

>    // Test the getMeOutButton
>    checkGetMeOutButton();	
[...]
>    // Test the reportButton
>    checkReportButton();

Can you please use the real text which is shown on that buttons?

>    // Test the ignoreWarning button
>    checkIgnoreButton(badSites[i]);

This one is a link and not a button.

>var checkGetMeOutButton = function() {

Can you please add a js doc comment in-front of all helper functions which explain their function.

>  // Instantiate the getMeOutButton that will be tested
>  var getMeOutButton = new elementslib.ID(controller.tabs.activeTab, "getMeOutButton");

Nit: We do not have to comment nearly each line of the code. IMO this one is self-explanatory.

>  // Wait for the getMeOutButton to be safely loaded on the warning page  
>  controller.waitForElement(getMeOutButton);
>  
>  // Test the elements asserted above
>  controller.click(getMeOutButton);

Please use waitThenClick here.

>  // Determine the default home page url is displayed in the location bar
>  var defaultHomePage = UtilsAPI.getProperty("resource:/browserconfig.properties", "browser.startup.homepage");
>  if ( locationBar.getNode().value.indexOf('mozilla') == -1) {
>    throw "Failed: getMeOutButton send user to a non-Mozilla related website.";

Given that we cannot use the default home page for a value check we should pull all the parameters from the default home page and use them for the comparison with the location bar, e.g. "client=firefox-a&rls=org.mozilla:en-US:official". So we can also check for the correct locale. You can use the split function here.

>var checkReportButton = function() {
>
>  // Instantiate the reportButton that will be tested
>  var reportButton = new elementslib.ID(controller.tabs.activeTab, "reportButton");
>
>  // Wait for the reportButton to be safely loaded onto the warning page
>  controller.waitForElement(reportButton);
>	
>  // Test the Report button
>  controller.click(reportButton);

See the above comment.

>  // Instantiate the required preferences and nodes for verification
>  var phishingUrl = PrefsAPI.preferences.getPref("browser.safebrowsing.warning.infoURL", "");
>  var malwareUrl = PrefsAPI.preferences.getPref("browser.safebrowsing.malware.reportURL", "");
>  var phishingElement = new elementslib.Link(controller.tabs.activeTab, "What is a Web Forgery? What is Phishing?");
>  var malwareElement = new elementslib.ID(controller.tabs.activeTab, "date");
[...]
>  phishingUrl = phishingUrl.replace(/%LOCALE%/g, locale);
>  malwareUrl = malwareUrl.replace(/%LOCALE%/g, locale);
>  malwareUrl = malwareUrl.replace(/%NAME%/g, "Firefox");  

I would really like to see that we only fetch the information for the type of page we wanna test. I have a proposal which will follow below.

>  // Switch LOCALE and NAME identifiers with actual locale of browser and Firefox being used
>  var locale = PrefsAPI.preferences.getPref("general.useragent.locale", "");

You can move that assignment to setupModule as "locale = PrefsAPI...". So you will have access to it via "locale" in each function and we only have to pull it once.

>  if ( locationBar.getNode().value.toLowerCase() == phishingUrl.toLowerCase() ) {
>     controller.assertNode(phishingElement);
>  } else if ( locationBar.getNode().value.toLowerCase().indexOf(malwareUrl.toLowerCase()) != -1 ) {
>     controller.assertNode(malwareElement);
>  } else {
>    throw "Failed: reportButton goes to the following improper address: " + locationBar.getNode().value.toLowerCase() + ", but should go to: " + malwareUrl.toLowerCase();
>  }
>}

I don't think that we should go this way to determine which type of forgery page we test. We should supply an argument to this function. In the simplest form just use the index of the for loop. Otherwise create a hash like we do in the test for checking the preferences dialog panes and add the type together with the url as an array entry. 

>var checkIgnoreButton = function(securityPage) {

Nit: That's a link and not a button and the argument should be called aURL.

>  // Instantiate the ignoreWarningButton that will be tested
>  var ignoreButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
>
>  // Wait for the ignoreButton to be safely loaded on the warning page
>  controller.waitForElement(ignoreButton);
>
>  // Click on the ignore warning button to go to the phishing site
>  controller.click(ignoreButton);

Same as above.

>  // Verify the warning page buttons are not visible and the location bar displays the correct url  
>  var locationBar = new elementslib.ID(controller.window.document, "urlbar");  
>
>  controller.assertValue(locationBar, securityPage);
>  controller.assertNodeNotExist(ignoreButton);
>  controller.assertNode(new elementslib.ID(controller.tabs.activeTab, "main-feature"));

You talk about buttons in the comment. Just update the comment because we only check the ignore button here which is fine.

And one additional comment to the code style. There are still a lot of hard tabs in that file. You should exchange those with soft tabs (blanks) and remove blanks from the end of the line. Some editors have an option for that in their preferences.

Nice job for the first round, Aakash!

Comment 2

[Aakash Desai [:aakashd]]

>var checkGetMeOutButton = function() {

what is an js doc comment?

>  // Determine the default home page url is displayed in the location bar
>  var defaultHomePage = UtilsAPI.getProperty("resource:/browserconfig.properties", "browser.startup.homepage");
>  if ( locationBar.getNode().value.indexOf('mozilla') == -1) {
>    throw "Failed: getMeOutButton send user to a non-Mozilla related website.";



default homepage for nightlies and betas are different. There are not any elements that are the same as well as parameters other than "mozilla". this is why it was used. Besides, this approach works. Don't change it.


>  // Instantiate the required preferences and nodes for verification
>  var phishingUrl = PrefsAPI.preferences.getPref("browser.safebrowsing.warning.infoURL", "");
>  var malwareUrl = PrefsAPI.preferences.getPref("browser.safebrowsing.malware.reportURL", "");
>  var phishingElement = new elementslib.Link(controller.tabs.activeTab, "What is a Web Forgery? What is Phishing?");
>  var malwareElement = new elementslib.ID(controller.tabs.activeTab, "date");
[...]
>  phishingUrl = phishingUrl.replace(/%LOCALE%/g, locale);
>  malwareUrl = malwareUrl.replace(/%LOCALE%/g, locale);
>  malwareUrl = malwareUrl.replace(/%NAME%/g, "Firefox");  


locale is only used once in the test case and it's in the checkReportButton() function. There's no point to move it to setupModule for this testscript.

>  if ( locationBar.getNode().value.toLowerCase() == phishingUrl.toLowerCase() ) {
>     controller.assertNode(phishingElement);
>  } else if ( locationBar.getNode().value.toLowerCase().indexOf(malwareUrl.toLowerCase()) != -1 ) {
>     controller.assertNode(malwareElement);
>  } else {
>    throw "Failed: reportButton goes to the following improper address: " + locationBar.getNode().value.toLowerCase() + ", but should go to: " + malwareUrl.toLowerCase();
>  }
>}


Please take a look at the preferences next time before reviewing because those links are different than the ones we use as variables. They're dependent on locales and this method works perfectly fine unless you can offer code that works better (pending a review from me).


>  // Instantiate the ignoreWarningButton that will be tested
>  var ignoreButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
>
>  // Wait for the ignoreButton to be safely loaded on the warning page
>  controller.waitForElement(ignoreButton);
>
>  // Click on the ignore warning button to go to the phishing site
>  controller.click(ignoreButton);


The element is called "ignoreWarningButton" from Firefox code itself. I'm not going to go against Firefox code. I don't want to change it because it's correct.

Comment 3

[Aakash Desai [:aakashd]]

Attachment: testSafeBrowsing/testSafeBrowsingWarningPages.js

Made changes to comments, naming of functions, waitThenClick, sleep(1000)'s and verified spacing syntax (no hard tabs left seen on mozmill IDE and textEdit on osx).

See comment above this one for more analysis of why other things were not changed.

Comment 4

[Aakash Desai [:aakashd]]

Attachment: testSafeBrowsing/testWarningPages.js

Ack, sorry about that. Changed comments to "in the notification bar" to "on the warning page" and removed "SafeBrowsing" from the name of the testscript.

Comment 5

[Aakash Desai [:aakashd]]

Attachment: testSafeBrowsing/testWarningPages.js

This is bad, but I forgot to change the title of the testscript to be in-line with its filename.

Comment 6

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

(In reply to comment #2)
> >var checkGetMeOutButton = function() {
> 
> what is an js doc comment?

See http://jsdoc.sourceforge.net/#usage or the other tests.

> >  // Determine the default home page url is displayed in the location bar
> >  var defaultHomePage = UtilsAPI.getProperty("resource:/browserconfig.properties", "browser.startup.homepage");
> >  if ( locationBar.getNode().value.indexOf('mozilla') == -1) {
> >    throw "Failed: getMeOutButton send user to a non-Mozilla related website.";
> 
> default homepage for nightlies and betas are different. There are not any
> elements that are the same as well as parameters other than "mozilla". this is
> why it was used. Besides, this approach works. Don't change it.

Ah that's true. I forgot that part. Given that fact we should do more as only check if "mozilla" is contained in the url. We can do the following:

1. Get the value of the location bar
2. Load the default home page
3. Check the current location with the value from step 1

> locale is only used once in the test case and it's in the checkReportButton()
> function. There's no point to move it to setupModule for this testscript.

All calls to PrefsAPI.preferences.getPref are executed twice because the checkReportButton is called twice from within the test function. We should really try to limit those calls to setupModule and tearDownModule. It will make clear which prefs we are using in the test without having to read the whole test before. Clint, what would you say? 

> Please take a look at the preferences next time before reviewing because those
> links are different than the ones we use as variables. They're dependent on
> locales and this method works perfectly fine unless you can offer code that
> works better (pending a review from me).

Right. And you should not use the variables. When the testing links are retrieved inside setupModule together with the locale you can create an associative array with the type of page and the url to check. I don't feel well to differentiate the checks by using the url. We should tell the function which type of page has to be checked.

Comment 7

[Aakash Desai [:aakashd]]

All of these reviews are based off of your personal preferences on what you think is a proper testcase rather than if something works or not. I really don't want to block development of mozmill testscripts if they work (and this one does) especially when we're trying to build our testscript coverage and do have the ability to go back and create better verification methodologies in our scripts later.

Again, I don't want to block my productivity because of these nits, Henrik. Also, please - or + this review.

Comment 8

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

Comment on attachment 389239 [details] [diff] [review]
testSafeBrowsing/testWarningPages.js

Aren't all those tests already covered by the test for the notification bar? If a step is missing we should enhance this test instead of having another complete test which takes a bit to finish.

Comment 9

[Aakash Desai [:aakashd]]

The other test covers the functionality of the buttons on the notification bar. This one covers the buttons on the warning page itself. They're two different testcases and testscripts, actually.

Comment 10

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

Attachment: Patch for all locales

Ok, in favor of the results server we should have this Litmus test as its own Mozmill test. Lets get it in...

I have updated the test so it will work for each locale now. I also did a couple of necessary updates. Please check if you are ok with it.

Comment 11

[Aakash Desai [:aakashd]]

Comment on attachment 394292 [details] [diff] [review]
Patch for all locales

I'm good with the changes. A quick run works for me as well. r+

Comment 12

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

Landed as http://hg.mozilla.org/qa/mozmill-tests/rev/b9390df59175

Comment 13

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

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.