659893 - Mozmill test for "Check the 'Up & Coming' Add-ons module"

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Tracking bug for developing a Mozmill test for https://litmus.mozilla.org/show_test.cgi?id=15074 "Check the Up & Coming Add-ons module"

Comment 1

[u415141]

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

Comment 2

[u415141]

vlad.maniac deleted the linked story in Pivotal Tracker

Comment 3

[u415141]

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

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP v1.0

Comment 5

[u279076]

Comment on attachment 535669 [details] [diff] [review]
WIP v1.0

>+ * Contributor(s):
>+ *   Vlad Maniac <[e-mail]>

Add "(original author)" after your email address

>+  discovery.waitForPageLoad();
>+
>+
>+  controller.assert(function () {

Only one line of separation between code blocks is necessary.

>+    return discovery.getSections().length === 6;

The expected value should probably be contained in a CONSTANT in the test.

>+  }, "There have to be 6 different sections - got '" +
>+     discovery.getSections().length + "', expected '6'.");

A better message here would be "Discovery pane has loaded all sections - got, expected"

>+  // Check if the Up and Coming section exists 
>+  controller.assert(function () {
>+    return typeof(upComing) == 'object';
>+  }, "There is an Up & Coming section - got '" +
>+     typeof(upComing) + "', expected 'object'."); 

Is there no property or attribute we can be checking here, instead of just that an "object" exists?

>+  // Check if there are 5 addons listed in Up and Coming Pane 
>+  var addonCount = discovery.getElements({type: "upAndComing_addons", parent: upComing}); 
>+  controller.assert(function () {
>+    return addonCount.length === 5;
>+  }, "There have to be 5 different sections - got '" +
>+     addonCount.length + "', expected '5'.");

Same suggestions as with the previous assert...better message, 5 as a CONSTANT.

>+  controller.sleep(2000);

Any situation where a sleep() is needed you should try to use a waitFor() instead. We typically only use sleep() as a last resort. What exactly are you waiting for here?

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

(In reply to comment #5)
> Comment on attachment 535669 [details] [diff] [review] [review]
> WIP v1.0
> 
> >+ * Contributor(s):
> >+ *   Vlad Maniac <[e-mail]>
> 
> Add "(original author)" after your email address
> 
> >+  discovery.waitForPageLoad();
> >+
> >+
> >+  controller.assert(function () {
> 
> Only one line of separation between code blocks is necessary.
> 
> >+    return discovery.getSections().length === 6;
> 
> The expected value should probably be contained in a CONSTANT in the test.
> Yes, final version will have this

> >+  }, "There have to be 6 different sections - got '" +
> >+     discovery.getSections().length + "', expected '6'.");
> 
> A better message here would be "Discovery pane has loaded all sections -
> got, expected"
> 
Thanks for the suggestion, will modify!
> >+  // Check if the Up and Coming section exists 
> >+  controller.assert(function () {
> >+    return typeof(upComing) == 'object';
> >+  }, "There is an Up & Coming section - got '" +
> >+     typeof(upComing) + "', expected 'object'."); 
> 
> Is there no property or attribute we can be checking here, instead of just
> that an "object" exists?
There is no need to check props, attributes. the test asks to look if this pane exists, this is properly tested if checked for object. Maybe hskupin can give us a clean eye's opinion here? 
> 
> >+  // Check if there are 5 addons listed in Up and Coming Pane 
> >+  var addonCount = discovery.getElements({type: "upAndComing_addons", parent: upComing}); 
> >+  controller.assert(function () {
> >+    return addonCount.length === 5;
> >+  }, "There have to be 5 different sections - got '" +
> >+     addonCount.length + "', expected '5'.");
> 
> Same suggestions as with the previous assert...better message, 5 as a
> CONSTANT.
> 
> >+  controller.sleep(2000);
> 
> Any situation where a sleep() is needed you should try to use a waitFor()
> instead. We typically only use sleep() as a last resort. What exactly are
> you waiting for here?
sleep() is indeed bad practice. this is temp, until final patch for fixing waitFor() lands. 

Will continue due to your suggestions. Thanks for the feedback Anthony!

Comment 7

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

(In reply to comment #6)
> > >+  controller.assert(function () {
> > >+    return typeof(upComing) == 'object';
> > >+  }, "There is an Up & Coming section - got '" +
> > >+     typeof(upComing) + "', expected 'object'."); 
> > 
> > Is there no property or attribute we can be checking here, instead of just
> > that an "object" exists?
> There is no need to check props, attributes. the test asks to look if this
> pane exists, this is properly tested if checked for object. Maybe hskupin
> can give us a clean eye's opinion here? 

You cannot check for object because it will always be true. In this case upComing is a Mozmill element. So you will have to call asserNode. But, keep in mind that the box could still be embedded but made invisible. So check the conditions and run a negative test to ensure such an assert will work.

> > >+  // Check if there are 5 addons listed in Up and Coming Pane 
> > >+  var addonCount = discovery.getElements({type: "upAndComing_addons", parent: upComing}); 

The name of the new element should not contain 'Count' because that's not what you are getting here. These are simple 'addons'.

Also check the other expected behaviors. There are some more to test.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP v1.1

WIP v1.1 does not include iterations b, c, and d from expected results in https://litmus.mozilla.org/show_test.cgi?id=15074
DOES: 

1. Verify if the Up and Coming section exists and it is visible 
2. Verify there are 5 addons featured under this list

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP v2.0

We still have a //TODO. Listening for a hover event is rocket science in Discovery Pane. Otherwise it's done

Comment 10

[u279076]

Comment on attachment 536584 [details] [diff] [review]
WIP v2.0

>+  am = new addons.AddonsManager(controller);
>+  tabBrowser = new tabs.tabBrowser(controller);

Please put a new line in here

>+
>+function testUpAndComing() {
>+  am.open();

Please add a comment about what this test is checking

>+  // Select the Get Add-ons pane
>+  am.setCategory({category: am.getCategoryById({id: "discover"})});
>+  var discovery = am.discoveryPane;
>+  discovery.waitForPageLoad();

Please break this into two code blocks "Select pane" and "wait for pane to load"

>+  controller.assert(function () {
>+    return discovery.getSections().length === 6;
>+  }, "There have to be 6 different sections - got '" +
>+     discovery.getSections().length + "', expected '6'.");

Please change this message to "All sections of Discovery Pane are loaded - got 'value', expected '6'"

>+  var upComing = discovery.getSection("up-and-coming");
>+  
>+  // Check if the Up and Coming section exists 

Please move the var under the comment

>+  controller.assert(function () {
>+    return controller.assertNode(upComing);
>+  }, "There is an Up and Coming section - got '" +
>+     controller.assertNode(upComing) + "', expected 'true'.");

Please reword to "Up & Coming section exists"

>+  // Check if the Up and Coming section is visible 
>+  var displayVisible = (upComing.getNode().style["display"] !== 'none');
>+  controller.assert(function () {
>+    return displayVisible;
>+  }, "The Up and Coming section is visible - got '" +
>+     displayVisible + "', expected 'true'.");

The previous assert may be unnecessary. If the Up&Coming section is visible, it exists inherently. However, it might be valuable to check the negative case: the section exists but is not visible. Henrik, any opinions here?
 
>+  // Check if there are 5 addons listed in Up and Coming Pane 
>+  var addon = discovery.getElements({type: "upAndComing_addons", parent: upComing}); 
>+  controller.assert(function () {
>+    return addon.length === 5;
>+  }, "There have to be 5 different sections - got '" +
>+     addon.length + "', expected '5'."); 

Reword this to "All sections of Up&Coming Pane have been loaded"


>+   // Check There is a "See All" link which loads the /extensions page
>+   var all = discovery.getElement({type: "upAndComing_seeAllLink", parent: upComing});
>+   controller.click(all); 

Please separate var and click() with a new line.

>+   controller.waitForPageLoad();
>+   controller.waitFor(function () {

Please separate waitForPageLoad() and waitFor() with a new line.

>+     return locationBar.value === "https://addons.mozilla.org/en-US/firefox/extensions/";

The URL should be defined as a constant at the top of this test.

>+    }, "Location bar should contain the link to AMO " +
>+   	locationBar.value + ", expected " + "https://addons.mozilla.org/en-US/firefox/extensions/");

Please reword to "Location Bar contains AMO URL"
  
>+  // Clicking on any of the addons will load its details page
>+  discovery.waitForPageLoad();
>+  var secondAddon = addon[1]; 
>+  controller.click(secondAddon);   
>+  discovery.waitForPageLoad();  
>+  var addonLearnMore = discovery.getElement({type: "addon_learnMoreButton"}); 

Please separate function calls and var lines by a new line.

>+  controller.assert(function () {
>+    return controller.assertNode(addonLearnMore);
>+  }, "The 'learn more' button exists on the page - got '" +
>+     controller.assertNode(addonLearnMore) + "', expected 'true'.");
>+
>+  var displayButtonVisible = (addonLearnMore.getNode().style["display"] !== 'none');
>+  controller.assert(function () {
>+    return displayButtonVisible;
>+  }, "The 'learn more' button is visible - got '" +
>+     displayButtonVisible + "', expected 'true'.");

As with before, I'm not sure we need to check for button.exists when button.visible proves the former. Henrik, please advise.

Comment 11

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

Comment on attachment 536584 [details] [diff] [review]
WIP v2.0

Please dont' ask for feedback when you have already answered in detail. I will read through the comments in either way.

Comment 12

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

(In reply to comment #10)
> >+  controller.assert(function () {
> >+    return controller.assertNode(upComing);
> >+  }, "There is an Up and Coming section - got '" +
> >+     controller.assertNode(upComing) + "', expected 'true'.");
> 
> Please reword to "Up & Coming section exists"
> 
> >+  // Check if the Up and Coming section is visible 
> >+  var displayVisible = (upComing.getNode().style["display"] !== 'none');
> >+  controller.assert(function () {
> >+    return displayVisible;
> >+  }, "The Up and Coming section is visible - got '" +
> >+     displayVisible + "', expected 'true'.");
> 
> The previous assert may be unnecessary. If the Up&Coming section is visible,
> it exists inherently. However, it might be valuable to check the negative
> case: the section exists but is not visible. Henrik, any opinions here?

We should remove the first assert, because the section will always be existent. It's only hidden if nothing should show up. The style check is important.

> >+  controller.assert(function () {
> >+    return addon.length === 5;
> >+  }, "There have to be 5 different sections - got '" +
> >+     addon.length + "', expected '5'."); 
> 
> Reword this to "All sections of Up&Coming Pane have been loaded"

This comment would be wrong because it mixes-up the different identifiers. It should better look like: "A given number of add-ons are visible..." 

> >+   // Check There is a "See All" link which loads the /extensions page
> >+   var all = discovery.getElement({type: "upAndComing_seeAllLink", parent: upComing});
> >+   controller.click(all); 

We separate blocks of code which do not belong to each other. In this case it would look strange if we separate it with a new line.

> >+     return locationBar.value === "https://addons.mozilla.org/en-US/firefox/extensions/";
> 
> The URL should be defined as a constant at the top of this test.

Also this URL is not locale aware, so we can't use it that way. You should better use utils.assertLoadedUrlEqual with "https://addons.mozilla.org/firefox/extensions/" which also takes care of the redirect.

> >+  var displayButtonVisible = (addonLearnMore.getNode().style["display"] !== 'none');
> >+  controller.assert(function () {
> >+    return displayButtonVisible;
> >+  }, "The 'learn more' button is visible - got '" +
> >+     displayButtonVisible + "', expected 'true'.");
> 
> As with before, I'm not sure we need to check for button.exists when
> button.visible proves the former. Henrik, please advise.

As above sounds like a candidate which is probably covered by a Selenium test. We need to get David and Krupa involved.

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

Attachment: WIP 2.1

New upload with requested changes.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 536839 [details] [diff] [review]
WIP 2.1

Review of attachment 536839 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 15

[u279076]

Comment on attachment 536839 [details] [diff] [review]
WIP 2.1

>+  locationBar =  new toolbars.locationBar(controller);
>+
>+}

Please remove the new line.

>+/** 
>+  * This function checks-up the Up and Coming section:
>+  * 1. Tests if the section exists and is visible
>+  * 2. Tests that a given number of addons are available 
>+  * 3. Tests the "See All" link 
>+  * 4. Verifies that click-ing and addon link redirects to AMO
>+  */

Please simplify this comment to a single statement, ie. "Tests the Up & Coming module of the AOM Discovery Pane"

>+  controller.assert(function () {
>+    return discovery.getSections().length === 6;
>+  }, "All sections of Discovery Pane are loaded - got '" +
>+     discovery.getSections().length + "', expected '6'.");

Please add a comment indicating what you are checking
  
>+  // Check if there are 5 addons listed in Up and Coming Pane 
>+  var addon = discovery.getElements({type: "upAndComing_addons", parent: upComing}); 
>+  controller.assert(function () {
>+    return addon.length === 5;
>+  }, "A given number of add-ons are visible - got '" +
>+     addon.length + "', expected '5'."); 

It would be helpful to capture '5' in a constant, probably in the shared module, called UP_AND_COMING_SIZE or something like that.

>+   /** 
>+   *TODO: 
>+   *Check if mouse hover on a particular add-on, 
>+   *the background color lightens for that add-on
>+   */
>+
>+  /** 
>+  * Click on an add-on will load its details page 
>+  * 1. Click the addon link, wait for the page to load
>+  * 2. Verify if the "Learn More button" is present on the page 
>+  * and visible. this way we know we are on detailView page  
>+  */

This might be covered by a Selenium test. Let me check with the WebQA team.

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

Thanks for feedback Anthony! i will postpone work for this bug until we have answers from webQA

Comment 17

[u279076]

Ccing Krupa and David Burns on this bug.

Krupa and Dave, any thoughts on whether the following can/is/should be covered by a Selenium test:

>+   /** 
>+   *TODO: 
>+   *Check if mouse hover on a particular add-on, 
>+   *the background color lightens for that add-on
>+   */
>+
>+  /** 
>+  * Click on an add-on will load its details page 
>+  * 1. Click the addon link, wait for the page to load
>+  * 2. Verify if the "Learn More button" is present on the page 
>+  * and visible. this way we know we are on detailView page  
>+  */

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

No add-on installation required, so i guess this would be a WONTFIX from the mozmill side? and taken care of by the webQA team?

Comment 19

[u279076]

After consulting with WebQA, it's been determined that only tests which test installing add-ons need to be automated in Mozmill. Resolving WONTFIX.

Comment 20

[u415141]

Anthony Hughes deleted the linked story in Pivotal Tracker