Closed Bug 732353 Opened 13 years ago Closed 6 years ago

Disable all Discovery Pane tests due to unpredictable web dependencies

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: vladmaniac, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint])

Attachments

(2 files, 50 obsolete files)

patch v10.0 default
(deleted), patch
Details | Diff | Splinter Review
patch v10.0 beta
(deleted), patch
Details | Diff | Splinter Review 
We need to skip the current discovery pane tests located in 

  hg.mozilla.org/qa/mozmill-tests/remote 

The remote/ folder will contain the following tests 

Litmus ID 15117	
Open https://addons-dev.allizom.org/en-US/firefox/addon/romanian-language-pack/
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Litmus ID 17355	
Open https://addons-dev.allizom.org/en-US/firefox/addon/status-watch/?src=search
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Litmus ID 15115	
Open https://addons-dev.allizom.org/en-US/firefox/addon/lavafox-v1-purple/?src=cb-dl-rating
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Litmus ID 15116	
Open https://addons-dev.allizom.org/en-US/firefox/addon/romanian-spellchecking-diction/
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Litmus ID 7936	
Open https://addons-dev.allizom.org/en-US/firefox/addon/fotofox/contribute/roadblock/?src=search&version=2.1
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"

Litmus ID 17356	
Open https://addons.mozilla.org/en-US/firefox/addon/memory-restart/?src=search
Click on "Add to Firefox"
Click "allow"
Verify that a window opens with a button that says "Install Now"
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Why? Please give detailed information why such a replacement is necessary.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Why? Please give detailed information why such a replacement is necessary.

Web Qa needs us to do the above tests under the remote/ directory 
We are skipping all discovery pane tests because we need to have a clean testrun there. 

This was discussed with Dave Hunt, Anthony and Marlena. The request came from Marlena through Anthony.
Attached patch skip patch v1.0 (obsolete) (deleted) — — Splinter Review 
Skip the failing discovery pane tests
Attachment #602295 - Flags: review?(anthony.s.hughes)
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #3)
> Created attachment 602295 [details] [diff] [review]
> skip patch v1.0
> 
> Skip the failing discovery pane tests

Please check in this patch on the default branch for now
I'm still not sure why those have to be replaced by (which) new ones? Again, please give detailed information about which new tests you are talking about.
Well, sorry those are listed... but as I can see those are not related to the discovery pane. So why have to replace the discovery pane tests?
(In reply to Henrik Skupin (:whimboo) from comment #6)
> Well, sorry those are listed... but as I can see those are not related to
> the discovery pane. So why have to replace the discovery pane tests?

Skipping the discovery pane tests for now is a good idea for two reasons: 

1. They are failing due to disc pane changes, and more, they are affected by the modal dialog error like the add-ons tests 

2. We need to have a clear remote test-run for the amo add-on installation tests
I still don't understand what you are meaning with 'replace' in the summary. We do not want to replace those tests. You haven't answered my question yet. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #8)
> I still don't understand what you are meaning with 'replace' in the summary.
> We do not want to replace those tests. You haven't answered my question yet.
> Thanks.

'Replace' would be a forced term then. Given the reasons in comment 7, we need to disable those tests
So please don't use it in the future then. Updating summary to reflect what really happens here and needs to be fixed.
OS: Linux → All
Hardware: x86 → All
Summary: Skip all remote tests currently in hg and replace them with the new ones, requested by webQa team → Failure in remote tests by switching the category in the Add-ons Manager (Category has been changed)
Whiteboard: [mozmill-test-failure]
Vlad, I would rather these be handled all in separate bugs.

1) A bug to disable Discovery Pane tests because of their unreliability
2) A bug for each new test we want to add

Please file the new bugs and close out this one.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> 1) A bug to disable Discovery Pane tests because of their unreliability

Whereby this should probably simply to fix. One patch will solve any failures.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Vlad, I would rather these be handled all in separate bugs.
> 
> 1) A bug to disable Discovery Pane tests because of their unreliability
> 2) A bug for each new test we want to add
> 
> Please file the new bugs and close out this one.

That was my initial intention, I am going to file bugs and provide patches as we go, 
please see bug 732357 for an example. 

This bug will disable the failing disc pane tests.
Summary: Failure in remote tests by switching the category in the Add-ons Manager (Category has been changed) → Failure in Discovery Pane remote tests | Category has been changed
Before I move forward with this review, we have some open issues to resolved here. I want to know the implications of this bug on the following unresolved failures:

bug 676533 - testDiscoveryPane_UpAndComingModule
bug 679330 - "aElement is undefined" in Discovery Pane tests
bug 677189 - testDiscoveryPane_installCollectionAddon
bug 688146 - Backout tests for deleted Discovery Pane panels
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Before I move forward with this review, we have some open issues to resolved
> here. I want to know the implications of this bug on the following
> unresolved failures:
> 
> bug 676533 - testDiscoveryPane_UpAndComingModule

This is the Modal dialog known issue

> bug 679330 - "aElement is undefined" in Discovery Pane tests

Right now there is a "Pick of the Month" pane in the collections pane but the add-on listed there is compatible only with firefox-release , so in theory we'd have to skip the test for default, aurora, beta

> bug 677189 - testDiscoveryPane_installCollectionAddon

Again, this is the Modal dialog known issue

> bug 688146 - Backout tests for deleted Discovery Pane panels

I was proposing to backout the tests where the panel was removed from disc pan. IMO its no use having those in hg. 

In conclusion, i propose disabling all discovery pane tests at the moment, so we could let webqa use the clean remote folder temporarily to gather their results. 
Then we can enable them again and decide further once they are eligible and once we decide a priority for the discovery pane tests.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #15)
> > bug 676533 - testDiscoveryPane_UpAndComingModule
> 
> This is the Modal dialog known issue
>
> > bug 677189 - testDiscoveryPane_installCollectionAddon
> 
> Again, this is the Modal dialog known issue

This is the same issue -- lets get it into one bug. Does this *only* affect DP tests? 

> > bug 679330 - "aElement is undefined" in Discovery Pane tests
> 
> Right now there is a "Pick of the Month" pane in the collections pane but
> the add-on listed there is compatible only with firefox-release , so in
> theory we'd have to skip the test for default, aurora, beta

Does this bug only affect the "PotM" pane? The wording seems to indicate this affects *all* DP tests.

> > bug 688146 - Backout tests for deleted Discovery Pane panels
> 
> I was proposing to backout the tests where the panel was removed from disc
> pan. IMO its no use having those in hg. 

So why have these not yet been removed?

> In conclusion, i propose disabling all discovery pane tests at the moment,
> so we could let webqa use the clean remote folder temporarily to gather
> their results. 

I would suggest that we clean up the open issues before disabling all the Discovery Pane tests. Whether that means backing out code, fixing code, or simply resolving bugs WONT FIX.
There is no sense in trying to fix issues in those tests because of the following reason: 

1. The panes are changing rapidly and we cannot keep up, this would mean to monitor only those tests and leave functional and endurance on the backlog - this does not make any sense at the moment. 

2. The Modal dialog issue is still pending for a solution and all tests involving add-on installation are affected. 

I've set wontfix status to the bugs which involve panes which are currently removed. There's no data to work with. 

Based on these strong reasons, please approve skipping all the discovery pane tests as a temporarily solution.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #17)
> 1. The panes are changing rapidly and we cannot keep up, this would mean to
> monitor only those tests and leave functional and endurance on the backlog -
> this does not make any sense at the moment. 

Thanks Vlad. I think we've now WONTFIXed any open Discovery Pane failure bugs that were lying around. In the future, I recommend against developing tests for features which depend on ever-changing web content. In fact, I would be in favour of giving WebQA their own branch in mozmill-tests to develop tests against their own features and giving them the necessary training to succeed (but that's a discussion for another time/place).

> 2. The Modal dialog issue is still pending for a solution and all tests
> involving add-on installation are affected. 

Let's keep this open and investigate if there is an event we can be listening for instead of waiting a set amount of time.

> Based on these strong reasons, please approve skipping all the discovery
> pane tests as a temporarily solution.

I will review this patch now, thanks.
Summary: Failure in Discovery Pane remote tests | Category has been changed → Disable all Discovery Pane tests due to unpredictable web dependencies
Comment on attachment 602295 [details] [diff] [review]
skip patch v1.0

>Bug 732353 - Skip all remote tests currently in hg and replace them with the new ones, requested by webQa team. r=ashughes

>+// Bug 732353 - Skip all remote tests currently in hg and 
>+//              replace them with the new ones, requested by webQa team
>+setupModule.__force_skip__ = "Bug 732353 - Skip all remote tests currently in hg and " + 
>+                             "replace them with the new ones, requested by webQa team";
>+teardownModule.__force_skip__ = "Bug 732353 - Skip all remote tests currently in hg and " + 
>+                                "replace them with the new ones, requested by webQa team";

Please change the message to reflect the bug summary. In other words, we are disabling these tests really because the content of the Discovery Pane is beyond reasonable expectation for automation; not because we want a clean testrun for new tests.
Attachment #602295 - Flags: review?(anthony.s.hughes) → review-
Attached patch [default] disable v1.1 [landed] (obsolete) (deleted) — — Splinter Review 
Fixed 

Please check-in the patch only on default branch
Attachment #602295 - Attachment is obsolete: true
Attachment #604332 - Flags: review?(anthony.s.hughes)
Attached patch [aurora] disable v1.0 [landed] (obsolete) (deleted) — — Splinter Review 
Please check-in for mozilla-aurora branch.
Attachment #604335 - Flags: review?(anthony.s.hughes)
Attached patch [beta] disable v1.0 [landed] (obsolete) (deleted) — — Splinter Review 
Skipping tests for branch mozilla-beta
Attachment #604339 - Flags: review?(anthony.s.hughes)
Attached patch [release] disable v1.0 [landed] (obsolete) (deleted) — — Splinter Review 
Skipping remote tests for mozilla-release branch
Attachment #604340 - Flags: review?(anthony.s.hughes)
Attached patch [esr] disable v1.0 [landed] (obsolete) (deleted) — — Splinter Review 
Skipping remote tests for branch mozilla-esr10
Attachment #604341 - Flags: review?(anthony.s.hughes)
We are having separate skip patches for each branch because files under /remote/restartTests differ from one branch to another.
Comment on attachment 604332 [details] [diff] [review]
[default] disable v1.1 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a33fadfc0405 (default)
Attachment #604332 - Attachment description: [default branch] skip patch v1.1 → [default] disable v1.1 [landed]
Attachment #604332 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 604335 [details] [diff] [review]
[aurora] disable v1.0 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/bc0759de6769 (mozilla-aurora)
Attachment #604335 - Attachment description: [aurora] skip patch v1.0 → [aurora] disable v1.0 [landed]
Attachment #604335 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 604339 [details] [diff] [review]
[beta] disable v1.0 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/723270439eab (mozilla-beta)
Attachment #604339 - Attachment description: [beta] skip patch v1.0 → [beta] disable v1.0 [landed]
Attachment #604339 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 604340 [details] [diff] [review]
[release] disable v1.0 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c2a020134201 (mozilla-release)
Attachment #604340 - Attachment description: [release] skip patch v1.0 → [release] disable v1.0 [landed]
Attachment #604340 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 604341 [details] [diff] [review]
[esr] disable v1.0 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/eb9f3d1acecc (mozilla-esr10)
Attachment #604341 - Attachment description: [esr] skip patch v1.0 → [esr] disable v1.0 [landed]
Attachment #604341 - Flags: review?(anthony.s.hughes) → review+
Blocks: 732913, 732357, 733367
Please verify that these patches have not introduced any regressions.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
http://mozmill-release.blargon7.com/#/remote/reports?branch=All&platform=All&from=2012-03-11&to=2012-03-12

Verified
Status: RESOLVED → VERIFIED
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
So when we don't want to have those tests we should remove them from the repository. If there are intents to re-enable do not close such a bug.

I just want to let you know that skipping all the tests causes an exit value of 1 by the testrun script now. This marks the testrun in Jenkins as failed. So a dummy test should have been added when all tests are getting skipped.

Also I'm not sure I buy the decision made here. That's why I reopen this bug now.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
> Also I'm not sure I buy the decision made here. That's why I reopen this bug
> now.

As for the decision made here, its very simple. There are two major reasons why did this: 

1. There were lots of orange failures like "Modal dialog has been found and processed" which hasn't got any fix because we cannot reproduce it locally. (you know the story) 

2. We haven't got the resources to maintain such tests because the disc pane can change weekly and can bust all the testrun. We talked to webqa and marketing, which handles the pane management and they say that the strategy is to change them (add new, delete old, change order, etc) to be consistent with marketing strategy.
Given the future volatility of the pane content I say lets just back them out. I'm not sure why I didn't consider backing them out before; maybe it never crossed my mind.

Henrik, is this an okay solution for you?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #35)
> Given the future volatility of the pane content I say lets just back them
> out. I'm not sure why I didn't consider backing them out before; maybe it
> never crossed my mind.
> 
> Henrik, is this an okay solution for you?

1. Fastest solution is to back them out 
2. Still, if we want the tests that bad, and they are valuable, we can develop an algorithm to test add-on installation regardless of the information in the panes, but it would mean a complex test/api with some order of code complexity larger than O(n) = log or 1, as we tried so far in the development.  

Let Henrik decide and webqa IMHO. I'm just standing by for the decision
I would say someone of you has to ask WebQA for a final decision. If those tests are really not maintainable due to the fast pace of replacing content, we should remove them.

In any case some of the tests are not bound to a specific add-on or page, so I don't see that it would take too much efforts to maintain at least those. Rapid changing panels should be removed.
(In reply to Henrik Skupin (:whimboo) from comment #37)
> I would say someone of you has to ask WebQA for a final decision. If those
> tests are really not maintainable due to the fast pace of replacing content,
> we should remove them.
> 

I will step in the webQa meeting and raise the issue to Stephen Donner. 
The tests we want to remove, as for the rapid panel changing reason, are: 

* /remote/restartTests/testDiscoveryPane_installPickOfMonthAddon
* /remote/restartTests/testDiscoveryPane_FirstTimeModule
* /remote/restartTests/testDiscoveryPane_installCollectionAddon

> In any case some of the tests are not bound to a specific add-on or page, so
> I don't see that it would take too much efforts to maintain at least those.
> Rapid changing panels should be removed.

We can maintain the following: 

* /remote/restartTests/testDiscoveryPane_UpAndComingModule
* /remote/restartTests/testDiscoveryPane_installAddonWithEULA
* /remote/restartTests/testDiscoveryPane_FeaturedModule

So the action item here remains for me to get the decision from webQA and based on that we move fwd
re enable tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon

Changes:
* Style guide changes
* Updated test to make usage of the assertions.js module 
* Removed 'disabled' flag in manifest
* Removed forced positioning to the getAddons pane since now we are waiting for that category in the addonsManager.open() method and have increased timeout if the case is to open Disc Pane

If we need to obsolete disabling patches please ask so in a comment 

Note: this patch was tested for default branch and has to be enabled there first in order to monitor the test a bit before transplanting
Attachment #640164 - Flags: review?(hskupin)
Comment on attachment 640164 [details] [diff] [review]
[default branch] re enable testDiscoveryPane_installPickOfMonthAddon patch v.10

>-// XXX: Bug 657492
>-//      Skip because the Mozilla's pick of the month add-on is not compatible with 
>-//      Nightly builds 
>-setupModule.__force_skip__ = "Bug 657492 - 'Pick of the Month' add-ons " + 
>-                             "are only compatible with Release and Beta builds";
>-teardownModule.__force_skip__ = "Bug 657492 - 'Pick of the Month' add-ons " + 
>-                                "are only compatible with Release and Beta builds";

Why are we going to enable this test for default? As the referenced bug states there are compatibility problems for default and aurora. Have those been fixed with the default by compatible feature?
Comment on attachment 640164 [details] [diff] [review]
[default branch] re enable testDiscoveryPane_installPickOfMonthAddon patch v.10

Please re-request review once my question has been answered.
Attachment #640164 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #40)
> Comment on attachment 640164 [details] [diff] [review]
> [default branch] re enable testDiscoveryPane_installPickOfMonthAddon patch
> v.10
> 
> >-// XXX: Bug 657492
> >-//      Skip because the Mozilla's pick of the month add-on is not compatible with 
> >-//      Nightly builds 
> >-setupModule.__force_skip__ = "Bug 657492 - 'Pick of the Month' add-ons " + 
> >-                             "are only compatible with Release and Beta builds";
> >-teardownModule.__force_skip__ = "Bug 657492 - 'Pick of the Month' add-ons " + 
> >-                                "are only compatible with Release and Beta builds";
> 
> Why are we going to enable this test for default? As the referenced bug
> states there are compatibility problems for default and aurora. Have those
> been fixed with the default by compatible feature?

Yes
Attachment #640164 - Flags: review?(hskupin)
re enables testDiscoveryPane_installAddonWithEULA
Attachment #641403 - Flags: review?(hskupin)
Attachment #641403 - Flags: review?(dave.hunt)
Attachment #640164 - Flags: review?(dave.hunt)
Can you provide test results that show these tests passing across the various platforms? Thanks.
Attachment #640164 - Flags: review?(hskupin)
Attachment #640164 - Flags: review?(dave.hunt)
Attachment #641403 - Flags: review?(hskupin)
Attachment #641403 - Flags: review?(dave.hunt)
(In reply to Dave Hunt (:davehunt) from comment #44)
> Can you provide test results that show these tests passing across the
> various platforms? Thanks.

Results can be found http://mozmill-crowd.blargon7.com/#/remote/reports?branch=All&platform=All&from=2012-07-13&to=2012-07-13 

"Connection on services.addons.mozilla.org" error is sometimes causing our "Modal Dialog has been found and processed" failure. This is on the "onDownloadCancelled" event and we need to catch this separately because its not an actual modal dialog failure, but a remote connection failure which causes our installation dialog to timeout. 

Cancelling review on this matter. Will follow up with better patches considering the above problems
Please enable AOM logging and check for the real failure message in the error console. If this is a problem with services.addons.mozilla.org we should get this fixed.
(In reply to Henrik Skupin (:whimboo) from comment #46)
> Please enable AOM logging and check for the real failure message in the
> error console. If this is a problem with services.addons.mozilla.org we
> should get this fixed.

This is the log: 

*** LOG addons.repository: Recreating database schema
*** LOG addons.repository: Creating database schema
*** LOG addons.xpi: Download started for https://addons.mozilla.org/firefox/downloads/latest/355386/addon-355386-latest.xpi?src=discovery-promo to file /tmp/tmp-tpc.xpi
*** LOG addons.xpi: Download of https://addons.mozilla.org/firefox/downloads/latest/355386/addon-355386-latest.xpi?src=discovery-promo completed.
*** WARN addons.xpi: Download failed: 404 Not Found
*** LOG addons.xpi: shutdown
*** LOG addons.xpi-utils: shutdown
*** LOG addons.xpi-utils: Database closed

So the download fails as shown by 
*** WARN addons.xpi: Download failed: 404 Not Found
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #47)
> *** LOG addons.repository: Recreating database schema
> *** LOG addons.repository: Creating database schema
> *** LOG addons.xpi: Download started for
> https://addons.mozilla.org/firefox/downloads/latest/355386/addon-355386-
> latest.xpi?src=discovery-promo to file /tmp/tmp-tpc.xpi
> *** LOG addons.xpi: Download of
> https://addons.mozilla.org/firefox/downloads/latest/355386/addon-355386-
> latest.xpi?src=discovery-promo completed.
> *** WARN addons.xpi: Download failed: 404 Not Found
> *** LOG addons.xpi: shutdown
> *** LOG addons.xpi-utils: shutdown
> *** LOG addons.xpi-utils: Database closed

So this is the Cheevos extension which we fail to download here. Testing the link shows me that this file is existent. So why do we get a 404 here? Seeking for feedback from Blair and Dave on this part.
Status: REOPENED → ASSIGNED
I should cc both of them.
Dave or Blair, could one of you please give us feedback regarding the issue seen in comment 48? Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #48)
> So this is the Cheevos extension which we fail to download here. Testing the
> link shows me that this file is existent. So why do we get a 404 here?
> Seeking for feedback from Blair and Dave on this part.

The "404 Not Found" is what the server returned - we don't guess at what message that should be. Maybe a hiccup on AMO? I'd recommend just re-testing. And if its still an issue, ask one of the AMO team if they can investigate.
Yeah, we had or still have this issue a lot of times. Vlad, please test it again and if it is reproducible lets get a bug filed on the AMO product.
(In reply to Henrik Skupin (:whimboo) from comment #52)
> Yeah, we had or still have this issue a lot of times. Vlad, please test it
> again and if it is reproducible lets get a bug filed on the AMO product.

Tested this today but no luck reproducing it. I think its fair to give it 24hours more. I'll try again tomorrow
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #53)
> (In reply to Henrik Skupin (:whimboo) from comment #52)
> > Yeah, we had or still have this issue a lot of times. Vlad, please test it
> > again and if it is reproducible lets get a bug filed on the AMO product.
> 
> Tested this today but no luck reproducing it. I think its fair to give it
> 24hours more. I'll try again tomorrow

So as a final result here - the error did not occur the last week. Myself and Andreea were testing with the extension logging pref on. 

Also, our ci reports from the last week do not reflect the error anymore on the MV qa box 
http://mozmill-ci.blargon7.com/#/remote/reports?branch=All&platform=All&from=2012-08-01&to=2012-08-07

Look like it was just a AMO hiccup, as Blair assumed in comment 51
We should figure out what we can do here once we have the time. For now bringing it back onto the radar.
Status: ASSIGNED → NEW
Priority: -- → P2
Assignee: vlad.mozbugs → nobody
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped] s=130114 u=failure c=private_browsing p=1
Priority: P2 → P3
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
The problems with the remote test cases are detailed below. 

Please tell me if anything else needs to be investigated here.

1) testDiscoveryPane_FeaturedModule/test1.js - am.setCategory({category: am.getCategoryById({id: "discover"})}); - returns error since no category needs to be selected now before choosing a featured addon. The test will pass if we remove this line.

2) testDiscoveryPane_FirstTimeModule/test1.js - except for the issue that is also presented in the above test case (related to category not needing to be selected which occurs in every test under remove/restartTests folder), the test will fail with error: "randomAddon is undefined". Verified this and the issue is:
- var feature = discovery.getSection("main-feature"); - main-feature does not exist anymore. It is now called main
- There seems to have been a list of addons shown in main-feature -> First time add-ons section. The section now contains only a watch the video button

3) testDiscoveryPane_installAddonWithEULA - "message": ": could not find element Selector: .install-action" due to the fact that https://services.addons.mozilla.org/en-US/firefox/discovery/addon/echofon-for-twitter/?src=discovery-featured page does not exist anymore
4) testDiscoveryPane_installCollectionAddon - fails with error "randomAddon is undefined" - Collection Addons is not displayed in the Discovery pane anymore

5) testDiscoveryPane_installPickOfMonthAddon - "message": "aElement is undefined" - in addons.js, line: "var source = /.*src=([^&]+)/.exec(aElement.getNode().href);" in getInstallSource function. The locator for install button changed to "a[class='button   add installer']", so if we change 

case "addon_installButton":
        nodeCollector.queryNodes(".install-button a"); 
TO       

case "addon_installButton":
        nodeCollector.queryNodes("a[class='button   add installer']");
in getElement function in addons.js the install button will be detected. 

Also, since the Pick of the month is actually the next link after First Time Addons now, we also need to remove these lines:
for (var i = 0; i < CLICK_COUNT; i++) {
    controller.click(nextLink);
    controller.sleep(TIMEOUT_SWITCH);
  }

6) testDiscoveryPane_UpAndComingModule - "message": "aElement is undefined".
This is the same issue as with testDiscoveryPane_installPickOfMonthAddon. The locator for install button changed. So performing the above change in addons.js file for getElement nodeCollector.queryNodes("a[class='button   add installer']"); INSTEAD of nodeCollector.queryNodes(".install-button a"); the test will pass

As a conclusion:
- the following tests can be fixed: testDiscoveryPane_FeaturedModule/test1.js, testDiscoveryPane_installPickOfMonthAddon/test1.js, testDiscoveryPane_UpAndComingModule/test1.js
- the "First time addons" section changed and does not show any addons now
- there aren't any colletion addons in Discovery pane anymore - I think testDiscoveryPane_installCollectionAddon/test1.js should remain skipped or we should remove it
- the page from which the addons with EULA was installed previously does not exist anymore - in test testDiscoveryPane_installAddonWithEULA/test1.js. I think we should find another page from where this add-on should be installed.
(In reply to Daniela Petrovici from comment #56)
> Please tell me if anything else needs to be investigated here.

Thanks for the detailed feedback. Lets see.

> 1) testDiscoveryPane_FeaturedModule/test1.js - am.setCategory({category:
> am.getCategoryById({id: "discover"})}); - returns error since no category
> needs to be selected now before choosing a featured addon. The test will
> pass if we remove this line.

Hm, interesting. I wonder why it has been put in. I can see this too now and I agree with your statement. Does it work across platforms?

> 2) testDiscoveryPane_FirstTimeModule/test1.js - except for the issue that is
> also presented in the above test case (related to category not needing to be
> selected which occurs in every test under remove/restartTests folder), the
> test will fail with error: "randomAddon is undefined". Verified this and the
> issue is:
> - var feature = discovery.getSection("main-feature"); - main-feature does
> not exist anymore. It is now called main

So we simply would have to change the id or class then in the lib.

> - There seems to have been a list of addons shown in main-feature -> First
> time add-ons section. The section now contains only a watch the video button

So looks like we can get rid of the test. But I can see that this video breaks the navigation between the different panes. Once it's shown you can't navigate anymore because the arrows are gone. Do you see the same. I would file a bug on that then.

> 3) testDiscoveryPane_installAddonWithEULA - "message": ": could not find
> element Selector: .install-action" due to the fact that
> https://services.addons.mozilla.org/en-US/firefox/discovery/addon/echofon-
> for-twitter/?src=discovery-featured page does not exist anymore

Looks like we should remove it then and replace it with a test that installs an addon with an Eula directly from AMO. There might already be a bug about it, if not please file one.

> 4) testDiscoveryPane_installCollectionAddon - fails with error "randomAddon
> is undefined" - Collection Addons is not displayed in the Discovery pane
> anymore

Yeah, lets get it removed.

> 5) testDiscoveryPane_installPickOfMonthAddon - "message": "aElement is
> undefined" - in addons.js, line: "var source =
> /.*src=([^&]+)/.exec(aElement.getNode().href);" in getInstallSource
> function. The locator for install button changed to "a[class='button   add
> installer']", so if we change 
> 
> case "addon_installButton":
>         nodeCollector.queryNodes(".install-button a"); 
> TO       
> 
> case "addon_installButton":
>         nodeCollector.queryNodes("a[class='button   add installer']");
> in getElement function in addons.js the install button will be detected. 
> 
> Also, since the Pick of the month is actually the next link after First Time
> Addons now, we also need to remove these lines:
> for (var i = 0; i < CLICK_COUNT; i++) {
>     controller.click(nextLink);
>     controller.sleep(TIMEOUT_SWITCH);
>   }

If it fixes it lets go with it! Yeah.

> 6) testDiscoveryPane_UpAndComingModule - "message": "aElement is undefined".
> This is the same issue as with testDiscoveryPane_installPickOfMonthAddon.
> The locator for install button changed. So performing the above change in
> addons.js file for getElement nodeCollector.queryNodes("a[class='button  
> add installer']"); INSTEAD of nodeCollector.queryNodes(".install-button a");
> the test will pass

Sweet.

> - the following tests can be fixed:
> testDiscoveryPane_FeaturedModule/test1.js,
> testDiscoveryPane_installPickOfMonthAddon/test1.js,
> testDiscoveryPane_UpAndComingModule/test1.js

Lets do it.

> - the "First time addons" section changed and does not show any addons now
> - there aren't any colletion addons in Discovery pane anymore - I think
> testDiscoveryPane_installCollectionAddon/test1.js should remain skipped or
> we should remove it
> - the page from which the addons with EULA was installed previously does not
> exist anymore - in test testDiscoveryPane_installAddonWithEULA/test1.js. I
> think we should find another page from where this add-on should be installed.

Lets get those removed and action on the last one as given in my above comment please. Thanks!
There is one more additional issue here that I have found while running tests runs with the above changes:
- the "Featured" and "Up and coming" sections can contain addons with EULA. When this happens, the "Continue to download" button appears instead of "Add to Firefox". After clicking "Continue to download", the button to install the addon appears.

Can you please tell me which of the following two flows would be ok with you?
1) split both tests so that there would be a test to install the addon without EULA and one to install the addon with EULA
OR
2) create a method in addons.js so that we can handle the case where the "Continue to download" button appears.

I investigated a bit more and I think that the first would be a better solution. This is because we won't pick a random addon we are not sure which type it is, but pick one that we know how to handle. And also I think it would make the tests more stable.
Flags: needinfo?(hskupin)
(In reply to Daniela Petrovici from comment #58)
> 1) split both tests so that there would be a test to install the addon
> without EULA and one to install the addon with EULA

That will not work because it's not in our hands which addons are getting displayed in those panes. Also I don't want that we have to update our tests over and over again whenever such a change happens.

> 2) create a method in addons.js so that we can handle the case where the
> "Continue to download" button appears.

That's better. It's just an if/else case here. We might already have something like that already. Please check the discovery pane class in addons.js. But I might be wrong that it already exists.

> I investigated a bit more and I think that the first would be a better
> solution. This is because we won't pick a random addon we are not sure which
> type it is, but pick one that we know how to handle. And also I think it
> would make the tests more stable.

Testing specifically for an addon with an EULA and without from AMO should be done in another test. Those tests here are for the discovery pane and as said we do not have any influence in which addons are promoted.
Flags: needinfo?(hskupin)
Attached patch patch v1.0 - re-enable remote tests (obsolete) (deleted) — — Splinter Review 
I have added the patch to re-enable the remote tests, but there are still some items I am not sure about. 

Per the discussion on IRC, the tests need to be run on Beta, ESR17, Release and ESR10 branches. I have created this patch for Beta branch.

Regarding some of the changes done:
1) Line 39 of the patch is needed in order to click on the Continue to download button when the Addon is with EULA
2) Line 42 of the patch is needed in order to click on the right Add to Firefox button. It seems that there are two or more addons that have 2 buttons called "Add to Firefox" on the installation page (one is hidden and one is not)
3) handleInstallAddon function is needed to handle all possible cases of the addons installation: Continue to Download for addons with EULA, 2 Add to Firefox buttons and the normal case
4) In the handleInstallAddon, there is this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE); line. This is needed due to bug 832947
5) Lines 646 - 649 are needed due to the fact that the section containing Pick of the month addon loads very slowly, so I added an assert.waitFor to make sure that the section was loaded before trying to click on the element.

The problem with this patch is that even though most of the issues are fixed, there are certain errors that still appear intermittently:
a) "Modal dialog has been found and processed" - that appeared once on Windows out of about 90 runs
b) " Disconnect Error: Application unexpectedly closed " - that appeared on Linux once out of 20 runs.

I am still investigating these errors, but I added the patch to get a feedback on what else should I do here, too.

Also, logged bug 835296 for comment 57 - install addon with EULA.
Attachment #707016 - Flags: feedback?(hskupin)
Comment on attachment 707016 [details] [diff] [review]
patch v1.0 - re-enable remote tests

This does not apply cleanly anymore since it was tested on FF 19. Needs to be re-created for FF 20 and also tested with this. I will start working on this.
Attachment #707016 - Flags: feedback?(hskupin)
I have started working on this and modified the patch to work with Firefox 20 Beta. The problem now is that there still are two or three add-ons with which it fails when test is run. 

One of the issues is that when opening Extensions, a modal dialog appears (e.g. asking the user if he wants to add the new search engine to the list). Due to this modal dialog, the test does not find the add-on in the list of extensions. I am still working on this issue, but since the add-on with which it fails is very difficult to get, it will take some time.
I have fixed an issue with the add-on that open tabs after installation (which was a problem in previous patch). There are two more issues to fix before uploading the patch:
1) Still need confirmation from Kris regarding whether or not we should ask the developer of DuckDuckGo add-on to remove the dialog that appears after installation and is affecting our tests
2) Also found a new add-on with which the install method does not work and I am investigating that - add-on name is Copy Plain Text.
(In reply to Daniela Petrovici from comment #63)
> 1) Still need confirmation from Kris regarding whether or not we should ask
> the developer of DuckDuckGo add-on to remove the dialog that appears after
> installation and is affecting our tests

As Kris has written he will get in contact with the developer. Given that it can take a while until it is fixed I agree that we should blacklist any extension which causes problems, and skip the test if this addon is proposed.
Attached patch patch v2.0 (Beta) (obsolete) (deleted) — — Splinter Review 
This patch is for Beta and I tested with 21.0b1 build - 200 runs and no fail except for bug 793705 which reproduces once in 21 runs now.

Modified getElements method in addons.js thus:
1) made sure that the Addons Manager is opened before searching for the element. This is due to the fact that some addons opened a new tab after the installation and this impeded us to get the category.
2) added a case for Continue to download button in case the addon that we pick randomly is with EULA - per previous discussions we want to make the installation method to work for both with and without EULA addons
3) removed nodeCollector.filterByCSSProperty("display", "inline-block"); due to the fact that some addons like Copy Plain Text had the display property set to inline.

Created the handleInstallAddon method to handle multiple cases:
- addon with EULA installation
- picking the right "Add to Firefox" button when there are multiple buttons displayed in the page source. This uses now the utils.isDisplayed method per discussion in the last Ask an expert meeting. This method uses display, state and offset to see if an element is really visible or not.
- this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE); - was added because of a bug logged on discovery page due to which after clicking on an addon in Discovery Pane, the page loads after 2 seconds. I have used 5 seconds in case the connection is not working properly

Also skipped DuckDuckGo Plus addon until the issue with the dialog is fixed. This is done in the FeaturedModule tests since it is a featured addon. But since we are picking the addon randomly, I have created an if-else case so that in case DDG is selected we do not install it and skip teardown.
Attachment #640164 - Attachment is obsolete: true
Attachment #641403 - Attachment is obsolete: true
Attachment #707016 - Attachment is obsolete: true
Attachment #733237 - Flags: feedback?(hskupin)
Attachment #733237 - Flags: feedback?(dave.hunt)
Comment on attachment 733237 [details] [diff] [review]
patch v2.0 (Beta)

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

::: lib/addons.js
@@ +1405,5 @@
>          break;
>        case "addon_learnMoreButton":
>          nodeCollector.queryNodes("#learn-more");
>          break;
> +      case "continue_toDownload":

I would call this addon_continueToDownload

@@ +1421,5 @@
> +    if (continueToDownload) {
> +      this._controller.click(continueToDownload);
> +    }
> +
> +    this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE);

What is the sleep required for? Can we add a comment? It would be better if we could use an appropriate wait.

@@ +1423,5 @@
> +    }
> +
> +    this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE);
> +    var installButton;
> +    var elements =  this.getElements({type: "addon_installButton"});

Check whitespace

@@ +1425,5 @@
> +    this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE);
> +    var installButton;
> +    var elements =  this.getElements({type: "addon_installButton"});
> +
> +    if(elements.length > 0) {

Check whitespace

@@ +1426,5 @@
> +    var installButton;
> +    var elements =  this.getElements({type: "addon_installButton"});
> +
> +    if(elements.length > 0) {
> +      for (var i = 0; i < elements.length; i++) {

Could this be a .forEach?

@@ +1427,5 @@
> +    var elements =  this.getElements({type: "addon_installButton"});
> +
> +    if(elements.length > 0) {
> +      for (var i = 0; i < elements.length; i++) {
> +        if (utils.isDisplayed(this._controller, elements[i])) {

In this case it should be enough to check that display is not 'none'.

@@ +1433,5 @@
> +          break;
> +        }
> +      }
> +    }
> +    return installButton;

We should assert that we have an installButton before returning.

::: lib/utils.js
@@ +428,5 @@
>        break;
>      default:
>        var style = controller.window.getComputedStyle(element, '');
>        var state = style.getPropertyValue('visibility');
> +      var display = style.getPropertyValue('display');

As mentioned before, I believe that enhancements to this method should be kept separately, and may not be needed at this point.

::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ -29,5 @@
>  function testInstallFeaturedAddon() {
>    am.open();
>  
> -  // Select the Get Add-ons pane
> -  am.setCategory({category: am.getCategoryById({id: "discover"})});

Why do we no longer select the category?

@@ +45,5 @@
>    var addonList = discovery.getElements({type: "upAndComing_addons", parent: featured});
>    var randomIndex = Math.floor(Math.random() * addonList.length);
>    var randomAddon = addonList[randomIndex];
>    var addonId = randomAddon.getNode().getAttribute("data-guid");
> +  var addonName = randomAddon.getNode().childNodes[3].textContent;

This looks fragile to me.

@@ +50,2 @@
>  
> +  if (addonName !== "DuckDuckGo Plus") {

We should maintain a list of blacklisted addons, and we should use the addon ID.

@@ +71,4 @@
>  
> +    assert.ok(am.isAddonInstalled({addon: addon}), "Add-on has been installed");
> +  } else {
> +    teardownModule.__force_skip__ = "Dialog opened by DuckDuckGo addon";

Can't we just pick another random addon, rather than skipping the test?
Attachment #733237 - Flags: feedback?(hskupin)
Attachment #733237 - Flags: feedback?(dave.hunt)
Attachment #733237 - Flags: feedback-
(In reply to Dave Hunt (:davehunt) from comment #66)
> > +    this._controller.sleep(TIMEOUT_LOAD_INSTALL_PAGE);
> 
> What is the sleep required for? Can we add a comment? It would be better if
> we could use an appropriate wait.

This was due to bug 832947, but I will try waiting for the install button.

> In this case it should be enough to check that display is not 'none'.

We cannot check for display != none because in the case of certain addons: https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-text-2/?src=discovery-featured
the display for the three of the four buttons is: inline, inline-block and block. Sometimes we need to click on the button with property inline and sometimes on the one with inline-block.

> > -  // Select the Get Add-ons pane
> > -  am.setCategory({category: am.getCategoryById({id: "discover"})});
> 
> Why do we no longer select the category?

Please see comment #57. We don't have the category anymore.

> Can't we just pick another random addon, rather than skipping the test?
We can pick another addon, but my understanding (based on comment #64) was that we need to skip the test if the addon is proposed.

I will make the changes to the patch, but can you please tell me how should I continue regarding the above items?
(In reply to Daniela Petrovici from comment #67)
> We cannot check for display != none because in the case of certain addons:
> https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-
> text-2/?src=discovery-featured
> the display for the three of the four buttons is: inline, inline-block and
> block. Sometimes we need to click on the button with property inline and
> sometimes on the one with inline-block.

That's no different from your current solution. You are picking the first 'visible' button. If display is none then the button wouldn't be displayed, so you want the first button that *is* displayed. That way it doesn't matter what the value of display is, so long as it's not 'none'. Can you give an example where you have multiple buttons where display is not none?

> > > -  // Select the Get Add-ons pane
> > > -  am.setCategory({category: am.getCategoryById({id: "discover"})});
> > 
> > Why do we no longer select the category?
> 
> Please see comment #57. We don't have the category anymore.

Okay, thanks

> > Can't we just pick another random addon, rather than skipping the test?
> We can pick another addon, but my understanding (based on comment #64) was
> that we need to skip the test if the addon is proposed.
> 
> I will make the changes to the patch, but can you please tell me how should
> I continue regarding the above items?

Regarding which items?
(In reply to Dave Hunt (:davehunt) from comment #68)
> (In reply to Daniela Petrovici from comment #67)
> > We cannot check for display != none because in the case of certain addons:
> > https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-
> > text-2/?src=discovery-featured
> > the display for the three of the four buttons is: inline, inline-block and
> > block. Sometimes we need to click on the button with property inline and
> > sometimes on the one with inline-block.
> 
> That's no different from your current solution. You are picking the first
> 'visible' button. If display is none then the button wouldn't be displayed,
> so you want the first button that *is* displayed. That way it doesn't matter
> what the value of display is, so long as it's not 'none'. Can you give an
> example where you have multiple buttons where display is not none?

The addon: https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-text-2/?src=discovery-featured has multiple buttons where display is not none. But you are right, the first button seems to always be the one on which we need to click, so I will change the method to only use display is not none.

> > > Can't we just pick another random addon, rather than skipping the test?
> > We can pick another addon, but my understanding (based on comment #64) was
> > that we need to skip the test if the addon is proposed.
> > 
> > I will make the changes to the patch, but can you please tell me how should
> > I continue regarding the above items?
> 
> Regarding which items?
Based on comment #64, we wanted to skip the test in case of DDG addon. Doesn't this still apply?

Also, I have found another addon with EULA where the handleInstallAddon method does not seem to work. I will investigate this before attaching a new patch.
(In reply to Daniela Petrovici from comment #69)
> (In reply to Dave Hunt (:davehunt) from comment #68)
> > (In reply to Daniela Petrovici from comment #67)
> > > We cannot check for display != none because in the case of certain addons:
> > > https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-
> > > text-2/?src=discovery-featured
> > > the display for the three of the four buttons is: inline, inline-block and
> > > block. Sometimes we need to click on the button with property inline and
> > > sometimes on the one with inline-block.
> > 
> > That's no different from your current solution. You are picking the first
> > 'visible' button. If display is none then the button wouldn't be displayed,
> > so you want the first button that *is* displayed. That way it doesn't matter
> > what the value of display is, so long as it's not 'none'. Can you give an
> > example where you have multiple buttons where display is not none?
> 
> The addon:
> https://services.addons.mozilla.org/en-US/firefox/discovery/addon/copy-plain-
> text-2/?src=discovery-featured has multiple buttons where display is not
> none. But you are right, the first button seems to always be the one on
> which we need to click, so I will change the method to only use display is
> not none.

I found the elements using web console:

var elements = document.querySelectorAll('.install-button a');
for (var i = 0; i < elements.length; ++i) { console.log(window.getComputedStyle(elements[i]).display) };

inline
none
inline-block
none

It appears that the first item is the install button for Firefox 4+ and the others are in a hidden div for pre Firefox 4. We should update the locator so we only see current versions. The following works for this example, but may not be robust enough for other addons.

document.querySelectorAll('.install-action > .install-shell .install-button a')
> It appears that the first item is the install button for Firefox 4+ and the
> others are in a hidden div for pre Firefox 4. We should update the locator
> so we only see current versions. The following works for this example, but
> may not be robust enough for other addons.
> 
> document.querySelectorAll('.install-action > .install-shell .install-button
> a')

This will not work when we have addons that are dependent on the platform. For instance: https://services.addons.mozilla.org/en-US/firefox/discovery/addon/print-pages-to-pdf/?src=discovery-upandcoming.
The locator will work for each button. I don't think we would like to check the platform inside the getElements method.
That's a different scenario, and one that hasn't been brought up previously (as far as I can tell). The linked addon is not available for Mac, and I can filter out the Windows/Linux buttons using the locator: '.install-action > .install-shell .install-button a:not(.concealed)'

I don't know how we're going to handle the situation in our tests though when an addon is not available for a platform. Henrik, is there a way we can control the content of the discovery pane so we don't have to handle all these edge cases?
Flags: needinfo?(hskupin)
No, there is no way to modify how the discovery pane is working. It receives a list of already installed add-ons and magically proposes a suites list of featured addons. If add-ons don't work we can only blacklist those. But usually we should not see add-ons listed which are not compatible with the current platform. If that's the case here we should get this fixed.
Flags: needinfo?(hskupin)
We don't see this addon listed on MAC. 

I have tried the locator Dave proposed in comment 72, but when trying it on Linux, this does not filter out the buttons for Windows. We will still need to rely on the display property to get to the button we need.
It would be great if there was a simple locator we could use for the install button. For now, let's use the first where display is not none for now and maintain a list of addons that we block whenever this causes an issue.
Depends on: 668976
Attached patch patch v3.0 (Beta) (obsolete) (deleted) — — Splinter Review 
Made changes per feedback. Also, I have kept the code for blacklisting DDG addon until the new version that fixes the dialog issue will be reviewed and added to the Featured addons in Discovery Pane.
Attachment #733237 - Attachment is obsolete: true
Attachment #735766 - Flags: feedback?(hskupin)
Attachment #735766 - Flags: feedback?(dave.hunt)
Comment on attachment 735766 [details] [diff] [review]
patch v3.0 (Beta)

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

::: lib/addons.js
@@ +61,5 @@
>    // Global section
>    ///////////////////////////////
>  
> +  get blacklisted() {
> +    var elements = [

I would call this addons or ids rather than elements.

@@ +62,5 @@
>    ///////////////////////////////
>  
> +  get blacklisted() {
> +    var elements = [
> +      // DuckDuckGo Plus addon

Could you give a brief reason for the blacklist. Just something like 'modal dialog present after restart' or similar.

@@ +702,5 @@
> +    var addonId = randomAddon.getNode().parentNode.getAttribute("data-guid");
> +
> +    this.blacklisted.forEach(function (aAddon) {
> +      if (aAddon === addonId) {
> +        randomIndex = Math.floor(Math.random() * aAddonList.length);

This appears to use the original list to pick a random addon. If the blacklisted addon is picked again then the test would fail.

@@ +1427,5 @@
>        case "addon_learnMoreButton":
>          nodeCollector.queryNodes("#learn-more");
>          break;
> +      case "addon_continueToDownload":
> +        nodeCollector.queryNodes("a[class='button eula go']");

If the order of these classes changed, or any additional classes were added then the test would fail. Please test with 'a.button.eula.go'
Attachment #735766 - Flags: feedback?(hskupin)
Attachment #735766 - Flags: feedback?(dave.hunt)
Attachment #735766 - Flags: feedback-
Attached patch patch v4.0 (Beta) (obsolete) (deleted) — — Splinter Review 
Modified patch based on review
Attachment #735766 - Attachment is obsolete: true
Attachment #750462 - Flags: feedback?(hskupin)
Attachment #750462 - Flags: feedback?(dave.hunt)
Comment on attachment 750462 [details] [diff] [review]
patch v4.0 (Beta)

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

In general I think it looks good. I haven't actually run the code. There are some things I would like to see updated. I gave details inline. Also the patch needs an update given that the strict mode is now enabled for remote tests on default. Can we get this checked-in to default first?

::: lib/addons.js
@@ +70,5 @@
> +    var ids = [
> +               // DuckDuckGo Plus addon - dialog opens after installation
> +               "jid1-ZAdIEUB7XOzOJw@jetpack"];
> +    return ids;
> +  },

I would not bind the list of add-ons which are blacklisted to the AddonsManager. Make this a global const instead. Further I would call it BLACKLISTED_ADDONS and use the id and name as entry, similar to other add-on tests.

@@ -647,1 @@
>      var categories = this.getElements({

nit: Please revert so we have this empty line.

@@ +714,5 @@
> +   *        Information for getting a category
> +   *        Elements: category - Category to get the id from
> +   *
> +   * @returns Category Id
> +   * @type {string}

The whole jsdoc comment does not match the method.

@@ +728,5 @@
> +        randomIndex = Math.floor(Math.random() * aAddonList.length);
> +        randomAddon = aAddonList[randomIndex];
> +        addonId = randomAddon.getNode().parentNode.getAttribute("data-guid");
> +      }
> +    });

Before selecting a random add-on I would filter the add-on list via the blacklist. That would remove the need of having duplicated code and unnecessary loops.

@@ +1099,5 @@
>     */
>    getElements : function AddonsManager_getElements(aSpec) {
> +    // In case the installed addon opens a new tab, make sure we are focused on
> +    // Addons Manager page when we search for elements
> +    var tabBrowser = new tabs.tabBrowser(this._controller);

this._controller might not work if the add-ons manager was opened in another window. You want to call getTabs() first and use the tab's controller. See the close() method.

@@ +1455,4 @@
>        default:
>          throw new Error(arguments.callee.name + ": Unknown element type - " + spec.type);
>      }
> +    return nodeCollector.elements;

nit: Please do not remove the empty lines on various places.

@@ +1460,2 @@
>  
> +  handleInstallAddon : function DiscoveryPane_handleInstallAddon() {

The method name doesn't correlate to the code in this method.

@@ +1469,5 @@
> +
> +    elements.some(function (aElement) {
> +      var style = this._controller.window.getComputedStyle(aElement.getNode(), '');
> +      var display = style.getPropertyValue('display');
> +      if (display !== "none") {

Can't we tweak utils.isDisplayed() for that case? I don't want to re-invent code like that on various places.

::: tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +52,5 @@
> +
> +  if (continueToDownload) {
> +      controller.click(continueToDownload);
> +      discovery.waitForPageLoad();
> +  }

Please add a comment that this is for possible add-ons which have an EULA.

::: tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
@@ +7,1 @@
>  var addons = require("../../../../lib/addons");

Why the change?

@@ +53,5 @@
> +
> +  if (continueToDownload) {
> +      controller.click(continueToDownload);
> +      discovery.waitForPageLoad();
> +  }

Please add a comment that this is for add-ons with an EULA.

::: tests/remote/restartTests/testDummy/manifest.ini
@@ -1,1 @@
> -[test1.js]

I would keep the dummy tests for now until bug 849962 has been fixed.
Attachment #750462 - Flags: feedback?(hskupin)
Attachment #750462 - Flags: feedback?(dave.hunt)
Attachment #750462 - Flags: feedback+
(In reply to Henrik Skupin (:whimboo) from comment #79)
> Comment on attachment 750462 [details] [diff] [review]
> patch v4.0 (Beta)
> 
> Review of attachment 750462 [details] [diff] [review]:
> -----------------------------------------------------------------
> Also the
> patch needs an update given that the strict mode is now enabled for remote
> tests on default. Can we get this checked-in to default first?
I can change the patch to be checked in to default first, but we have discuss some time ago about the fact that some addons do not work on Nightly and Aurora versions, but are compatible by default on Beta. Can you please confirm that we want to run the tests on Nightly and Aurora given the above.

> ::: tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
> @@ +7,1 @@
> >  var addons = require("../../../../lib/addons");
> 
> Why the change?
I have ordered the libraries alphabetically since we are already modifying the test. The change is not really necessary, but seemed the right thing to do since we are already modifying the tests.
(In reply to Daniela Petrovici from comment #80)
> I can change the patch to be checked in to default first, but we have
> discuss some time ago about the fact that some addons do not work on Nightly
> and Aurora versions, but are compatible by default on Beta. Can you please
> confirm that we want to run the tests on Nightly and Aurora given the above.

Update the tests for beta for now for strict mode. Later as follow-up we could add those addons to the blacklist for aurora and beta. Otherwise can we find out if we have a nightly or aurora build? Then we could easily skip those tests on the branches.

> > Why the change?
> I have ordered the libraries alphabetically since we are already modifying
> the test. The change is not really necessary, but seemed the right thing to
> do since we are already modifying the tests.

I don't see it alphabetically ordered. addons still comes before assertions.
(In reply to Henrik Skupin (:whimboo) from comment #81)
> (In reply to Daniela Petrovici from comment #80)
> > I can change the patch to be checked in to default first, but we have
> > discuss some time ago about the fact that some addons do not work on Nightly
> > and Aurora versions, but are compatible by default on Beta. Can you please
> > confirm that we want to run the tests on Nightly and Aurora given the above.
> 
> Update the tests for beta for now for strict mode. 
The strict mode did not land on Beta, only default branch. Do we want to wait with these changes until the strict mode will be backported for Beta, too?

> Otherwise can we find out if we have a nightly or aurora build? Then we could easily skip
> those tests on the branches.
The tests are already skipped on Nightly and Aurora builds.
 
> > > Why the change?
> > I have ordered the libraries alphabetically since we are already modifying
> > the test. The change is not really necessary, but seemed the right thing to
> > do since we are already modifying the tests.
> 
> I don't see it alphabetically ordered. addons still comes before assertions.
My understanding was that we have non-alphanumeric characters before the alphanumeric ones. This is how the majority of our tests are so this is why I changed it.
(In reply to Daniela Petrovici from comment #82)
> > Update the tests for beta for now for strict mode. 
> The strict mode did not land on Beta, only default branch. Do we want to
> wait with these changes until the strict mode will be backported for Beta,
> too?

Have the remote tests been updated for the strict mode in nightly? If yes, we would most likely get merge conflicts once it comes to aurora->beta merge. So I would really see that we also update the default and aurora branches, so that everything is in sync.

> > Otherwise can we find out if we have a nightly or aurora build? Then we could easily skip
> > those tests on the branches.
> The tests are already skipped on Nightly and Aurora builds.

I know but it will always give us problems when we merge from aurora->beta because that would need a merge by hand, which I want to avoid. Can we dynamically skip those tests based on the version of the firefox build which gets run?

> > I don't see it alphabetically ordered. addons still comes before assertions.
> My understanding was that we have non-alphanumeric characters before the
> alphanumeric ones. This is how the majority of our tests are so this is why
> I changed it.

I don't see non-alphanumeric characters in the module name. addons is still before assertions for me in the alphabet.
(In reply to Henrik Skupin (:whimboo) from comment #83)
> Have the remote tests been updated for the strict mode in nightly? If yes,
> we would most likely get merge conflicts once it comes to aurora->beta
> merge. So I would really see that we also update the default and aurora
> branches, so that everything is in sync.
I will update the patch for Nightly and Aurora, too.

> Can we
> dynamically skip those tests based on the version of the firefox build which
> gets run?
I think we can if we make use of Services.jsm - using Services.appinfo.version and checking if it contains the string "0a" that will apply for Nightly and Aurora.

> I don't see non-alphanumeric characters in the module name. addons is still
> before assertions for me in the alphabet.
I was refering to the "{" character before the "assert" string. I have looked at existing tests and bug 669600, bug 600291. They both have "assert" before "addons", but I will revert the change I made.

At the moment, the patch is done, but there is still an issue with the "FT DeepDark" theme which appears under Featured Addons and is installed under "Appearance", not "Extensions" in Addons Manager. 

I am waiting for confirmation if this is the expected behaviour. If it is, I think we should change the "getAddons" method to check the addon is in "Appearance" section. If it is not the expected behavior we will need to blacklist this addon too until the issue is solved.
(In reply to Daniela Petrovici from comment #84)
> (In reply to Henrik Skupin (:whimboo) from comment #83)
> I think we can if we make use of Services.jsm - using
> Services.appinfo.version and checking if it contains the string "0a" that
> will apply for Nightly and Aurora.

That's what I wanted to hear. Great. But lets make use of utils.appInfo instead and use the version comparator for a1 and a2.

> > I don't see non-alphanumeric characters in the module name. addons is still
> > before assertions for me in the alphabet.
> I was refering to the "{" character before the "assert" string. I have
> looked at existing tests and bug 669600, bug 600291. They both have "assert"
> before "addons", but I will revert the change I made.

They shouldn't have, given that we sort by modules and not imported symbols nowadays. Those two referenced bugs are way too old.

> At the moment, the patch is done, but there is still an issue with the "FT
> DeepDark" theme which appears under Featured Addons and is installed under
> "Appearance", not "Extensions" in Addons Manager. 

Well, you can retrieve the type of add-on given that we have the id and select the right category.

> I am waiting for confirmation if this is the expected behaviour. If it is, I
> think we should change the "getAddons" method to check the addon is in
> "Appearance" section. If it is not the expected behavior we will need to
> blacklist this addon too until the issue is solved.

We cannot hard-code that given that add-ons are replaced over time. It has to be dynamic as given above.
(In reply to Henrik Skupin (:whimboo) from comment #85)
> Well, you can retrieve the type of add-on given that we have the id and
> select the right category.
So, in case of a complete theme which appears under Featured Addons in Discovery Pane - like FT DeepDark theme, should we check that the addon is under Extensions, if it is not, check under Appearance category?
Why would you look under extensions first, if it's known to be a theme? That doesn't make that much of sense. Directly check the themes category.
(In reply to Henrik Skupin (:whimboo) from comment #87)
> Why would you look under extensions first, if it's known to be a theme? That
> doesn't make that much of sense. Directly check the themes category.
Because we are randomly picking featured addons in the test, we don't know if they are themes or not. 
When the theme/extension is presented in the Discovery Pane - Featured Addons section, there isn't anything different between them.
Why don't we know about their type? Once the addon is installed all the information is retrievable.
Attached patch patch v1.0 for default (obsolete) (deleted) — — Splinter Review 
This is the patch for default that has the skip based on version. I have also created a new method in addon.js to get all addons that have the given ID. 

It is different from the getInstalledAddons because we need to combine the AddonManager.getAllAddons and AddonManager.getAllInstalls calls. This is because:
1) We have addons that require restart after installation and every test checks the category before restarting Firefox.
2) AddonManager.getAllAddons - returns the list of installed addons. The addons that are installed after restart do not appear in this list
3) AddonManager.getAllInstalls returns the list of addons pending installation
4) Also, we do not restart before checking addon installation.

NOTE: I have requested feedback again because this is a major change in the way we handle finding the type of add-on. Please tell me if this is the correct approach.
Attachment #750462 - Attachment is obsolete: true
Attachment #759116 - Flags: feedback?(hskupin)
Comment on attachment 759116 [details] [diff] [review]
patch v1.0 for default

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

::: lib/addons.js
@@ +43,5 @@
>    {name: "extensions.getMoreThemesURL", old: "addons.mozilla.org", new: AMO_PREVIEW_DOMAIN}
>  ];
>  
> +const BLACKLISTED_ADDONS = [
> +  {name: "DuckDuckGo Plus", id: "jid1-ZAdIEUB7XOzOJw@jetpack"}

Please put the id before the name. Not only because of an alphabetical order but also given that the id is more important as the name and uniquely identifies the addon.

@@ +299,5 @@
> +   *        The addon that needs to be checked against the black list
> +   *
> +   * @returns {Boolean} True if the addon is blacklisted
> +   */
> +  isBlacklisted : function AddonsManager_isBlacklisted(aAddon) {

I don't see why this has to be a method of the AddonsManager. It can be used everywhere. So please make it global.

@@ +309,5 @@
> +        found = true;
> +      }
> +    });
> +
> +    return !found;

Please use Array.some() here, which will simply it a lot.

@@ +320,5 @@
> +   *        List of addons
> +   *
> +   * @returns {Object} A random addon that is not blacklisted
> +   */
> +  getRandomAddon : function AddonsManager_getRandomAddon(aAddonList) {

Same here. Make it a global function.

@@ +1083,5 @@
> +    // In case the installed addon opens a new tab, make sure we are focused on
> +    // Addons Manager page when we search for elements
> +    var amTab = this.getTabs()[0];
> +    var tabBrowser = new tabs.tabBrowser(amTab.controller);
> +    tabBrowser.selectedIndex = amTab.index;

This has absolutely nothing to do with the getElements() method and should be separated out.

@@ +1454,5 @@
> +
> +    assert.waitFor(function () {
> +      elements = this.getElements({type: "addon_installButton"});
> +      return elements.length > 0;
> +    }, "The addon details page has been opened", undefined, undefined, this);

Make use of self instead so that we can get rid of the additional 3 arguments.

@@ +1537,5 @@
> + * @param {String} aAddonId Id of the addon 
> + *
> + * @returns {array of ElemBase} The list of addons with given ID
> + */
> +function getAllAddons(aAddonId) {

Why do you pass in an addon which you directly compare? You want a callback here similar to getInstalledAddons().

Also I think something better is to only have getAllAddons which returns both the installed and to installed addons. The retrieval for installed addons can be done via an additional check in the callback.
Attachment #759116 - Flags: feedback?(hskupin) → feedback-
Attached patch patch v1.1 (obsolete) (deleted) — — Splinter Review 
Made changes based on feedback. 

NOTES:
1) I have also changed the timeout for downloading the addon since there are addons with a larger size (like Print Pages to PDF) that take a long time to download especially when the network is very slow.
2) Due to the fact that some addons open new tabs (and that this sometimes affects finding the correct category), we need to handle the opening of new tabs. This is done inside each test now.
Attachment #759116 - Attachment is obsolete: true
Attachment #774612 - Flags: feedback?(hskupin)
Comment on attachment 774612 [details] [diff] [review]
patch v1.1

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

Before I start to review this patch please incorporate the same changes Andrei did on bug 867217, so we can run those tests also with Mozmill 2.0.
Attachment #774612 - Flags: feedback?(hskupin)
Attached patch patch_v1.2 (obsolete) (deleted) — — Splinter Review 
Added changes from bug  867217.
Assignee: daniela.p98911 → cosmin.malutan
Attachment #774612 - Attachment is obsolete: true
Attachment #777659 - Flags: feedback?(hskupin)
Comment on attachment 777659 [details] [diff] [review]
patch_v1.2

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

Sorry for the long delay here. Now that I had a look at this patch  I recognized that it is not the right one. It's about l10n.
Attachment #777659 - Flags: feedback?(hskupin)
Comment on attachment 825904 [details] [diff] [review]
patch_v2.0

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

The reports you posted are not very helpful.
These DiscoveryPane tests are only run on Beta and Release, they are skipped on Nightly and Aurora.

We will want to land this patch on default and mozilla-aurora as well.
Please provide a backported patch for mozilla-beta (this one doesn't apply cleanly).

I've tried manually running some tests on Beta with this patch, and I've seen some failures like:
> "exception": {
>   "message": "'Get Add-ons' pane has been selected. - '[object XULElement]' should equal '[object XULElement]'", 
>   "lineNumber": 90, 
>   "name": "Error", 
>   "fileName": "file:///Users/andrei.eftimie/work/mozilla/bugs/732353/mozmill-tests/lib/addons.js"
> }

These might be due to running tests from a different branch, but please check that we have no failures.
Attachment #825904 - Flags: review?(andrei.eftimie)
Attachment #825904 - Flags: review?(andreea.matei)
Attachment #825904 - Flags: review-
Attachment #825904 - Flags: review?(andrei.eftimie)
Attachment #825904 - Flags: review?(andreea.matei)
Attachment #825904 - Flags: review-
Attachment #825904 - Attachment is obsolete: true
Attachment #825904 - Flags: review?(andrei.eftimie)
Attachment #825904 - Flags: review?(andreea.matei)
Attachment #832153 - Flags: review?(andrei.eftimie)
Attachment #832153 - Flags: review?(andreea.matei)
Attachment #832153 - Flags: review?
Comment on attachment 832153 [details] [diff] [review]
patch_v3.0 [beta]

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

::: lib/addons.js
@@ +793,5 @@
>     */
>    waitForCategory : function AddonsManager_waitForCategory(aSpec, aActionCallback) {
>      var spec = aSpec || { };
>      var category = spec.category;
>      assert.ok(category, arguments.callee.name + ": Category has been specified.");

Please remove "arguments.callee.name" from here as well.

@@ +800,1 @@
>      // For the panes 'discover' and 'search' we have to increase the timeout

New line above, after return.

@@ +1463,5 @@
>          nodeCollector.queryNodes("#learn-more");
>          break;
> +      case "addon_continueToDownload":
> +        nodeCollector.queryNodes("a.button.eula.go");
> +        break;

This should be a few cases up, alphabetically sorted.

@@ +1571,5 @@
> + *
> + * @param {Boolean} aInstalled
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons
> + *        (install or in progress of being installed)

installed or ..

@@ +1580,2 @@
>   */
> +function getAllAddons(aCallbackFilter, aAllAddons) {

aAllAddons is not listed in the parameters list, but aInstalled is. Also, doesn't that have a default value?

@@ +1627,5 @@
> +/**
> + * Get a random addon that is not blacklisted
> + *
> + * @param {Object} aAddonList
> + *        List of addons

So this is an array? it's missing the [] for the type.

::: lib/utils.js
@@ +457,5 @@
>    switch (element.nodeName) {
>      case 'panel':
>        visible = (element.state === 'open');
>        break;
> +    case 'a':

We should find a more descriptive name to this case.

::: tests/remote/restartTests/manifest.ini
@@ -10,1 @@
>  [include:testDiscoveryPane_installAddonWithEULA/manifest.ini]

I don't see this test anymore.

::: tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
@@ +124,2 @@
>    assert.ok(addonIsInstalled, "Extension '" + addonName + "' has been installed");
>  }

If we don't use a test2 to fully check the installing part, since we have addons that are not restartless, the message is not completely right.
The addons which are not restartless are not being installed at this point, but are in the process of being installed.

So we either have a test2 which checks installed state, no matter the addon type, or we change this messages.

Andrei, what do you think?

::: tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +32,5 @@
>  
>  function teardownModule(aModule) {
>    delete persisted.currentAddon;
>    aModule.am.close();
> +  // Bug 867217

New line above.

@@ +54,5 @@
>    am.open();
>  
>    // Select the Get Add-ons pane
>    am.setCategory({category: am.getCategoryById({id: "discover"})});
> +  // Wait for the Get Add-ons pane to load

New line before comments. Actually, not sure we need this comment, it's pretty obvious from the code that we wait for the discovery pane to load.

@@ +110,4 @@
>  
>    var addon = am.getAddons({attribute: "name", value: persisted.currentAddon})[0];
>  
>    assert.ok(am.isAddonInstalled({addon: addon}), "Add-on has been installed");

Now, the pick of the month is not a restartless addon, so it's fully installing the addon only after restart.
Attachment #832153 - Flags: review?(andreea.matei) → review-
Comment on attachment 8340378 [details] [diff] [review]
patch_v3.1[beta]

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

Cosmin, we still need to firstly apply these changes on default and mozilla-aurora.
I asked for an additional backport to mozilla-beta so we can run these tests as they are not run at all on Nightly and Aurora due to the nature of the tested addons.

::: firefox/lib/addons.js
@@ +1457,5 @@
>        case "addon_backLink":
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> +	      nodeCollector.queryNodes("a.button.eula.go");

nit: indentation

@@ +1567,5 @@
>   *        If not provided the unfiltered add-ons will be returned.
>   *        If provided, the callback filter takes an argument of an Addon object
>   *        as documented at https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon
>   *        and returns a filtered version.
> + *        	 *

I'm guessing this is a typo?

@@ +1580,5 @@
>   */
> +function getAllAddons(aCallbackFilter, aInstalled) {
> +  var addonInfo = [];
> +  var addonInstallsInfo = [];
> +  var installed = aInstalled || false;

This is a little vague.
Lets cast this a boolean.

::: firefox/lib/utils.js
@@ +459,5 @@
> +      var style = controller.window.getComputedStyle(element, '');
> +      var state = style.getPropertyValue('visibility');
> +      var display = style.getPropertyValue('display');
> +      visible = (state === 'visible' || display !== 'none');
> +      break;

I don't like this at all, but Dave requested to make them a separate case in comment 66 so leave them as is.

We should really amend the default case to check for 3 things:
- visiblity
- display
- offset

That should make this method much more reliable.
We might want to open another bug for this enhancement.

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +35,1 @@
>  

You miss a closing bracket here. This fails.
Attachment #8340378 - Flags: review?(andrei.eftimie)
Attachment #8340378 - Flags: review?(andreea.matei)
Attachment #8340378 - Flags: review-
Attached patch patch v3.2[nightly] (obsolete) (deleted) — — Splinter Review 
(In reply to Andrei Eftimie from comment #101)
> Cosmin, we still need to firstly apply these changes on default and
> mozilla-aurora.
> I asked for an additional backport to mozilla-beta so we can run these tests
> as they are not run at all on Nightly and Aurora due to the nature of the
> tested addons.

Andrei here is the patch for Nightly.

Reports:
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d179794d41
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d1797963b8
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d17979620b
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d179796fb5
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d17978fd54
http://mozmill-crowd.blargon7.com/#/remote/report/0bbed480c5898d9d02bca5d179790955
Attachment #8343134 - Flags: review?(andrei.eftimie)
Attachment #8343134 - Flags: review?(andreea.matei)
Attached patch patch v3.2[beta] (obsolete) (deleted) — — Splinter Review 
Hi, here is the patch for beta. 

Reports:
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979c9ea
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979e565
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979c645
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979e377
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979d7e7
mozmill-crowd.blargon7.com/#/functional/report/0bbed480c5898d9d02bca5d17979eda0
Attachment #8340378 - Attachment is obsolete: true
Attachment #8343155 - Flags: review?(andrei.eftimie)
Attachment #8343155 - Flags: review?(andreea.matei)
Comment on attachment 8343134 [details] [diff] [review]
patch v3.2[nightly]

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

This fails for me (both mozmill 1.5 and 2.0):
http://mozmill-crowd.blargon7.com/#/remote/report/40742e0746fd767e6d2fd58659003871

::: firefox/lib/utils.js
@@ +459,5 @@
> +      var style = controller.window.getComputedStyle(element, '');
> +      var state = style.getPropertyValue('visibility');
> +      var display = style.getPropertyValue('display');
> +      visible = (state === 'visible' || display !== 'none');
> +      break;

Please remove this whole block, it is not needed anymore.
We have ameliorated the default case to also take display and dimensions into considerations.
Attachment #8343134 - Flags: review?(andrei.eftimie)
Attachment #8343134 - Flags: review?(andreea.matei)
Attachment #8343134 - Flags: review-
Comment on attachment 8343155 [details] [diff] [review]
patch v3.2[beta]

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

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test2.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var { assert } = require("../../../../../lib/assertions");

In backporting this you missed to include utils.js here, not sure what you have run, but the included reports referenced in comment 103 have all these tests skipped.
Attachment #8343155 - Flags: review?(andrei.eftimie)
Attachment #8343155 - Flags: review?(andreea.matei)
Attachment #8343155 - Flags: review-
Attached patch patch v3.2[beta] (obsolete) (deleted) — — Splinter Review 
(In reply to Andrei Eftimie from comment #104)
> > +      visible = (state === 'visible' || display !== 'none');
> > +      break;
> 
> Please remove this whole block, it is not needed anymore.
> We have ameliorated the default case to also take display and dimensions
> into considerations.
I removed that code but I had to enhance your check because in case you parse an "auto" width you will get an NaN witch compared with 0 will return false, even though the element is visible.

Also this is dependant on bug 951138, so so I had to find and clear some preferences in teardown module and I referenced the bug number so we will remove them after the bug is fixed.

Reports:
http://mozmill-crowd.blargon7.com/#/remote/report/0fc916b47806e5d11cba8af7dc1508d8
http://mozmill-crowd.blargon7.com/#/remote/report/0fc916b47806e5d11cba8af7dc151661
http://mozmill-crowd.blargon7.com/#/remote/report/0fc916b47806e5d11cba8af7dc151f05
Attachment #8343155 - Attachment is obsolete: true
Attachment #8357805 - Flags: review?(andrei.eftimie)
Attachment #8357805 - Flags: review?(andreea.matei)
Comment on attachment 8357807 [details] [diff] [review]
patch v3.3[nightly]

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

::: firefox/lib/addons.js
@@ +1507,5 @@
>        case "mainFeature_ryfLearnMore":
>          nodeCollector.queryNodes(".ryff .more>a");
>          break;
>        case "mainFeature_pickMonthInstall":
> +        nodeCollector.queryNodes("#monthly");

This should have the install button.

@@ +1536,5 @@
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> + 	      nodeCollector.queryNodes("a.button.eula.go");
> +        break;

Indentation.

@@ +1561,5 @@
> +    var elements;
> +
> +    var self = this;
> +    assert.waitFor(function () {
> +      elements = self.getElements({type: "addon_installButton"});

Do we need to get the element in the waitfor?

@@ +1669,2 @@
>        aAddons.forEach(function (addon) {
> +        var result = aCallbackFilter(addon);

aAddon

@@ +1726,5 @@
> + *
> + * @returns {Boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS.some(function (addon) {

You should find a better name and have the 'a' prefix. It can't be aAddon since it's already part of the general method.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test2.js
@@ +12,5 @@
>  
>  const PREF_INSTALL_DIALOG = "security.dialog_enable_delay";
>  const PREF_LAST_CATEGORY = "extensions.ui.lastCategory";
>  
> +// Bug 951138

Please add a TODO item to change these, along with the bug number, in all tests.

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +55,5 @@
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST_180);
> +
> +  if (aModule.locationBar.getNotification()) {
> +    aModule.controller.keypress(null, "VK_ESCAPE", {});
> +  }

So this is for the notification regarding the addon being installed (when it's restartless) right? But what about one that isn't restartless, don't we need to add a second test?
Attachment #8357807 - Flags: review?(andrei.eftimie)
Attachment #8357807 - Flags: review?(andreea.matei)
Attachment #8357807 - Flags: review-
Comment on attachment 8357805 [details] [diff] [review]
patch v3.2[beta]

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

This should be added after default is approved.
Attachment #8357805 - Flags: review?(andrei.eftimie)
Attachment #8357805 - Flags: review?(andreea.matei)
Attachment #8357805 - Flags: review-
Attached patch patch_v3.4[beta] (obsolete) (deleted) — — Splinter Review 
Those tests are ran only on Beta and release, so this patch would be for testing. I will return with the patch for Nightly.
http://mozmill-crowd.blargon7.com/#/remote/report/00bcee9ac333de165bbc47d9e82a5e64
http://mozmill-crowd.blargon7.com/#/remote/report/00bcee9ac333de165bbc47d9e82a0909
http://mozmill-crowd.blargon7.com/#/remote/report/00bcee9ac333de165bbc47d9e82a4e8c
Attachment #8357805 - Attachment is obsolete: true
Attachment #8399402 - Flags: review?(andrei.eftimie)
Attachment #8399402 - Flags: review?(andreea.matei)
Attached patch patch_v3.4[nightly] (obsolete) (deleted) — — Splinter Review 
This is the patch for nightly.
Thanks
Attachment #8357807 - Attachment is obsolete: true
Attachment #8399405 - Flags: review?(andrei.eftimie)
Attachment #8399405 - Flags: review?(andreea.matei)
Comment on attachment 8399405 [details] [diff] [review]
patch_v3.4[nightly]

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

::: firefox/lib/addons.js
@@ +1536,5 @@
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> + 	      nodeCollector.queryNodes("a.button.eula.go");
> +        break;

nit: indentation

@@ +1675,4 @@
>      }
>    });
>  
> +  if (installed) {

This should be at the beginning of the method and completely split this method into 2 parts. As it is now we run the above code even installed addons, and we fail to properly return them. And it's hard to read.

@@ +1676,5 @@
>    });
>  
> +  if (installed) {
> +    AddonManager.getAllInstalls(function (aAddons) {
> +      if (aCallbackFilter == undefined) {

This will fail if you want to get all installed addons without a callback and you call `getAllAddons(null, true)`

@@ +1693,3 @@
>  
> +  assert.waitFor(function () {
> +    return (addonInfo.length !== 0) || (addonInstallsInfo.length !== 0);

We never get to return addonInstallsInfo if there's anything in addonInfo. This is in contradiction with the description of the method.

@@ +1710,5 @@
> + *
> + * @returns {Object} A random addon that is not blacklisted
> + */
> +function getRandomAddon(aAddonList) {
> +  aAddonList = aAddonList.filter(isBlacklisted);

You should name the variable addonList (this is a local variable filtered from the one received as an argument)

::: firefox/lib/utils.js
@@ +466,5 @@
>        var height = parseInt(style.getPropertyValue('height'), 10);
>  
>        visible = (display !== 'none') &&
>                  (visibility === 'visible') &&
> +                ((width === width) ? width > 0 : true) &&

You could use the built-in function isNaN().

If the width and height are NaN can we assert that the element is visible?
Where did you come across this?

@@ +468,5 @@
>        visible = (display !== 'none') &&
>                  (visibility === 'visible') &&
> +                ((width === width) ? width > 0 : true) &&
> +                ((height === height) ? height > 0: true);
> +                // This checks that the dimensions are different from NaN

You can also remove this comment if you use isNaN()

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test2.js
@@ +51,5 @@
> +
> +function testPickOfTheMonthAddonIsInstalled() {
> +  am.open();
> +
> +  var addonType = addons.getAllAddons(function (aAddon) {

Please update anonymous functions to the fat arrow syntax. Especially in new code.
Attachment #8399405 - Flags: review?(andrei.eftimie)
Attachment #8399405 - Flags: review?(andreea.matei)
Attachment #8399405 - Flags: review-
Comment on attachment 8399402 [details] [diff] [review]
patch_v3.4[beta]

Thanks for also providing the backport to Beta so we can test this patch.
It doesn't need review flags up until we land it on default.
Attachment #8399402 - Flags: review?(andrei.eftimie)
Attachment #8399402 - Flags: review?(andreea.matei)
Attached patch patch_v4.0[nightly] (obsolete) (deleted) — — Splinter Review 
Here is the patch for nightly.
Attachment #8399405 - Attachment is obsolete: true
Attachment #8401148 - Flags: review?(andrei.eftimie)
Attachment #8401148 - Flags: review?(andreea.matei)
Comment on attachment 8401148 [details] [diff] [review]
patch_v4.0[nightly]

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

We're getting closer here.

::: firefox/lib/addons.js
@@ +690,5 @@
>        type: "categories",
>        subtype: spec.attribute,
>        value: spec.value
>      });
> +    assert.notEqual(categories.length, 0, "Categories has been found.");

There was a mistake here from the past, it's "Categories have been found".

@@ +1536,5 @@
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> + 	      nodeCollector.queryNodes("a.button.eula.go");
> +        break;

Indentation is off here.

@@ +1704,5 @@
> +  return randomAddon;
> +}
> +
> +/**
> + * Checks addon against the list of blacklisted ones

nit: Check

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +120,5 @@
>  
>    md.waitForDialog(TIMEOUT_DOWNLOAD);
>  
> +  try {
> +    assert.waitFor(function () {

I think try block should also include the action - applies to all files.

@@ +122,5 @@
>  
> +  try {
> +    assert.waitFor(function () {
> +      return tabOpened;
> +    }, "A new tab has been opened");

Please use fat arrows where possible.

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test2.js
@@ +48,5 @@
> +    aModule.controller.stopApplication(true);
> +  }
> +}
> +
> +function testFeaturedModuleIsInstalled() {

Please add js doc.

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test2.js
@@ +48,5 @@
> +    aModule.controller.stopApplication(true);
> +  }
> +}
> +
> +function testUpAndComingAddonIsInstalled() {

We need to add js doc.

@@ +53,5 @@
> +  am.open();
> +
> +  var addonType = addons.getAllAddons((aAddon) => {
> +    let addonId = (aAddon.addon && aAddon.addon.id) ?
> +                  aAddon.addon.id : aAddon.id;

One space shifted to the right.

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +64,5 @@
>    var discovery = am.discoveryPane;
> +
> +  controller.window.addEventListener("TabOpen", checkNewTabOpened, false);
> +
> +  let pickMonthInstal = new elementslib.ID(controller.window.document,

nit: pickMonthInstall

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test2.js
@@ +48,5 @@
> +    aModule.controller.stopApplication(true);
> +  }
> +}
> +
> +function testPickOfTheMonthAddonIsInstalled() {

jsdoc please.

@@ +53,5 @@
> +  am.open();
> +
> +  var addonType = addons.getAllAddons((aAddon) => {
> +    let addonId = (aAddon.addon && aAddon.addon.id) ?
> +                  aAddon.addon.id : aAddon.id;

Shift to the right here too.
Attachment #8401148 - Flags: review?(andrei.eftimie)
Attachment #8401148 - Flags: review?(andreea.matei)
Attachment #8401148 - Flags: review-
Attached patch patch_v4.1[nightly] (obsolete) (deleted) — — Splinter Review 
Here is the updated patch.
Attachment #8401131 - Attachment is obsolete: true
Attachment #8401148 - Attachment is obsolete: true
Attachment #8402656 - Flags: review?(andrei.eftimie)
Attachment #8402656 - Flags: review?(andreea.matei)
Attached patch patch_v4.1 (obsolete) (deleted) — — Splinter Review 
This is patch for beta for testing.
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] s=130114 u=failure c=private_browsing p=1 → [mozmill-test-failure][mozmill-test-skipped]
Comment on attachment 8402656 [details] [diff] [review]
patch_v4.1[nightly]

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

::: firefox/lib/addons.js
@@ +790,5 @@
>    waitForCategory : function AddonsManager_waitForCategory(aSpec, aActionCallback) {
>      var spec = aSpec || { };
>      var category = spec.category;
> +    assert.ok(category, "Category has been specified.");
> +    if (this.selectedCategory.getNode().isEqualNode(category.getNode()))

Please use curly brackets

@@ +1535,5 @@
>        case "addon_backLink":
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> + 	      nodeCollector.queryNodes("a.button.eula.go");

nit: indentation  (you have a tab mingled with spaces)

@@ +1560,5 @@
> +    var installButton = null;
> +    var elements;
> +
> +    var self = this;
> +    assert.waitFor(function () {

Fat arrow syntax please.

@@ +1565,5 @@
> +      elements = self.getElements({type: "addon_installButton"});
> +      return elements.length > 0;
> +    }, "The addon details page has been opened");
> +
> +    elements.some(function (aElement) {

Fat arrow syntax please

@@ +1570,5 @@
> +      if (utils.isDisplayed(this._controller, aElement)) {
> +        installButton = aElement;
> +        return installButton.exists();
> +      }
> +    }, this);

You can also remove this binding with the use of the fat arrow function syntax above.

@@ +1651,5 @@
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons
> + *        (installed or in progress of being installed)
> + *
> + *  @returns {Array of addons}

nit: indentation is off

@@ +1662,4 @@
>  
> +  function filter(aAddons) {
> +    if (typeof aCallbackFilter === "function") {
> +      aAddons.forEach(function (aAddon) {

Fat arrow syntax

@@ +1680,5 @@
> +  else {
> +    AddonManager.getAllAddons(filter);
> +  }
> +
> +  assert.waitFor(function () {

Fat arrow please.

@@ +1681,5 @@
> +    AddonManager.getAllAddons(filter);
> +  }
> +
> +  assert.waitFor(function () {
> +    return addonInfo.length !== 0;

Are we waiting enough here?
I see a possibility where we might return earlier then expected.
We should wait until addonInfo.length equals aAddons.length (from the filter function).

Otherwise we'll return true here after we have our first addon, potentially missing the rest.

@@ +1712,5 @@
> + *
> + * @returns {Boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS.some(function (aBlacklistedAddon) {

fat arrow please

@@ +1758,5 @@
>   * Retrieve information from installed add-ons and send it to Mozmill
>   */
>  function submitInstalledAddons() {
>    frame.events.fireEvent('installedAddons',
> +    getAllAddons(function (aAddon) {

fat arrow please

::: firefox/lib/utils.js
@@ +467,5 @@
>  
>        visible = (display !== 'none') &&
>                  (visibility === 'visible') &&
> +                ((isNaN(width)) ? true : width > 0) &&
> +                ((isNaN(height)) ? true : height > 0);

I don't think this is the right approach. We get NaN from parseInt() when there is no set value.
In this case the computedStyle is 'auto'.

We should switch to use:
> var width = element.offsetWidth;
> var height = element.offsetHeight;
This will return the actual render dimensions.

::: firefox/tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js
@@ +12,5 @@
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>  
>    // Skip test if we don't have enabled plugins
> +  var activePlugins = addons.getAllAddons(function (aAddon) {

fat arrow please

::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +24,5 @@
>  
>    tabs.closeAllTabs(controller);
>  
>    // Skip test if we don't have enabled plugins
> +  var activePlugins = addons.getAllAddons(function (aAddon) {

fat arrow please

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +77,5 @@
>  function testInstallFeaturedAddon() {
> +  var tabOpened = false;
> +
> +  function checkNewTabOpened() {
> +    controller.window.removeEventListener("TabOpen", checkNewTabOpened, false);

Please use our general used pattern with try/finally for adding/removing event listeners.

@@ +102,5 @@
> +  // In case the addon is with EULA
> +  var continueToDownload = discovery.getElement({type: "addon_continueToDownload"});
> +
> +  if (continueToDownload) {
> +    controller.click(continueToDownload);

continueToDownload.click()

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
@@ +62,5 @@
>  function testInstallUpAndComingAddon() {
> +  var tabOpened = false;
> +
> +  function checkNewTabOpened() {
> +    controller.window.removeEventListener("TabOpen", checkNewTabOpened, false);

Pleas follow the try/finally pattern to remove the event listener.

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +52,5 @@
>  function testInstallPickOfTheMonthAddon() {
> +  var tabOpened = false;
> +
> +  function checkNewTabOpened() {
> +    controller.window.removeEventListener("TabOpen", checkNewTabOpened, false);

try/finally pattern to remove the listener

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +24,5 @@
> +  try {
> +    addons.getAllAddons((aAddon) => {
> +      if (aAddon && aAddon.isActive &&
> +          aAddon.type === "plugin" && aAddon.name === "Shockwave Flash")
> +        return true;

You can return the whole check directly, you don't need the if clause at all.
Attachment #8402656 - Flags: review?(andrei.eftimie)
Attachment #8402656 - Flags: review?(andreea.matei)
Attachment #8402656 - Flags: review-
Attached patch patch_v4.2[nightly] (obsolete) (deleted) — — Splinter Review 
Here is the patch for default.
Attachment #8402656 - Attachment is obsolete: true
Attachment #8405271 - Flags: review?(andrei.eftimie)
Attachment #8405271 - Flags: review?(andreea.matei)
Comment on attachment 8405271 [details] [diff] [review]
patch_v4.2[nightly]

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

You might also want to check the recent changes to the notifications. Those are not in beta yet but they might be in time so we'll need to handle them.

::: firefox/lib/addons.js
@@ +1536,5 @@
>        case "addon_backLink":
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> + 	      nodeCollector.queryNodes("a.button.eula.go");

The indentation here is still off.

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
@@ +104,4 @@
>  
>    md.start(addons.handleInstallAddonDialog);
>    controller.click(addToFirefox);
>    md.waitForDialog(TIMEOUT_DOWNLOAD);

In the other tests these lines are in the try block as well.

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +56,5 @@
>    am.setCategory({category: am.getCategoryById({id: "discover"})});
>    var discovery = am.discoveryPane;
> +
> +  let pickMonthInstall = new elementslib.ID(controller.window.document,
> +                                           "monthly");

Indentation a bit off.
Attachment #8405271 - Flags: review?(andrei.eftimie)
Attachment #8405271 - Flags: review?(andreea.matei)
Attachment #8405271 - Flags: review-
Attached patch patch_v4.3[nightly] (obsolete) (deleted) — — Splinter Review 
Fixed the nits, thanks for review!
Attachment #8405271 - Attachment is obsolete: true
Attachment #8413631 - Flags: review?(andrei.eftimie)
Attachment #8413631 - Flags: review?(andreea.matei)
Comment on attachment 8413631 [details] [diff] [review]
patch_v4.3[nightly]

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

::: firefox/lib/addons.js
@@ +1554,5 @@
> +
> +  /**
> +   * Get the "Add to Firefox" button
> +   *
> +   * @returns {Object} Element that is the install button for the addon

nit: type should be lowercase

@@ +1644,2 @@
>   *
> + * @param {Function} [aCallbackFilter]

nit: type should be lowercase, please check all instances

@@ +1652,5 @@
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons
> + *        (installed or in progress of being installed)
> + *
> + * @returns {Array of addons}

{Addon[]}

(https://code.google.com/p/jsdoc-toolkit/wiki/TagParam#Parameter_Type_Information)

@@ +1659,4 @@
>   */
> +function getAllAddons(aCallbackFilter, aInstalled) {
> +  var addonInfo = [];
> +  var installed = !!aInstalled;

You can use default arguments, I think it works well in this case.

(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters)

@@ +1713,5 @@
> + *
> + * @returns {Boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS.some((aBlacklistedAddon) => {

You don't need parentheses around passed-in arguments.

@@ +1759,5 @@
>   * Retrieve information from installed add-ons and send it to Mozmill
>   */
>  function submitInstalledAddons() {
>    frame.events.fireEvent('installedAddons',
> +    getAllAddons(function (aAddon) {

You can also change this function to the fat arrow syntax.

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +57,5 @@
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST);
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST_180);
> +
> +  if (aModule.locationBar.getNotification()) {
> +    aModule.controller.keypress(null, "VK_ESCAPE", {});

elem.sendKeys

@@ +117,3 @@
>  
> +  try {
> +    controller.click(addToFirefox);

addToFirefox.click()

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test2.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var { assert } = require("../../../../../lib/assertions");

You don't need to include the assertion module in tests.

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js
@@ +42,5 @@
>    prefs.preferences.clearUserPref(PREF_INSTALL_DIALOG);
>    prefs.preferences.clearUserPref(PREF_LAST_CATEGORY);
>  
> +  if (aModule.locationBar.getNotification()) {
> +    aModule.controller.keypress(null, "VK_ESCAPE", {});

elem.sendKeys

@@ +80,5 @@
> +  // In case the addon is with EULA
> +  var continueToDownload = discovery.getElement({type: "addon_continueToDownload"});
> +
> +  if (continueToDownload) {
> +    controller.click(continueToDownload);

continueToDownload.click()

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test2.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var { assert } = require("../../../../../lib/assertions");

You can remove this.

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +5,5 @@
>  "use strict";
>  
>  // Include required modules
>  var addons = require("../../../../lib/addons");
> +var { assert, expect } = require("../../../../../lib/assertions");

You can remove this.

@@ +62,5 @@
> +
> +  persisted.addonId = pickMonthInstall.getNode().parentNode.getAttribute("data-addonguid");
> +
> +  var btnGoToAddonInstall = discovery.getElements({type: "mainFeature_pickMonthInstall"})[0];
> +  controller.click(btnGoToAddonInstall);

btnGoToAddonInstall.click()

@@ +85,3 @@
>  
> +  try {
> +    controller.click(addToFirefox);

addToFirefox.click()

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test2.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var { assert } = require("../../../../../lib/assertions");

Remove this.

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +21,5 @@
>    aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
>  
>    // Skip test if we don't have Flash plugin enabled
> +  try {
> +    addons.getAllAddons((aAddon) => {

You can omit parentheses around the arguments.
Attachment #8413631 - Flags: review?(andrei.eftimie)
Attachment #8413631 - Flags: review?(andreea.matei)
Attachment #8413631 - Flags: review-
Attached patch patch_v5.0[nightly] (obsolete) (deleted) — — Splinter Review 
Here is the patch for default.
Attachment #8413631 - Attachment is obsolete: true
Attachment #8417850 - Flags: review?(andrei.eftimie)
Attachment #8417850 - Flags: review?(andreea.matei)
Comment on attachment 8417850 [details] [diff] [review]
patch_v5.0[nightly]

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

Looks good.

Please ask a review from Henrik with the mentioned issues fixed.

::: firefox/lib/addons.js
@@ +690,5 @@
>        type: "categories",
>        subtype: spec.attribute,
>        value: spec.value
>      });
> +    assert.notEqual(categories.length, 0, "Categories has been found.");

nit: `have`

@@ +1763,1 @@
>        return {

nit: You can also omit the return statement, it is implied if there is only 1 expression.

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test1.js
@@ +65,5 @@
> +
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  // Remove condition when transitioned to 2.0
> +  if ("restartApplication" in aModule.controller) {

We don't run mozmill 1.5 anymore, please remove this condition.

@@ +111,4 @@
>  
> +  var tabOpened = false;
> +  function checkNewTabOpened() {
> +    tabOpened = true;

This whole function can live on 1 line.

::: firefox/tests/remote/restartTests/testDiscoveryPane_FeaturedModule/test2.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var addons = require("../../../../lib/addons");
> +var { assert } = require("../../../../../lib/assertions");

You don't need to include assert in test files.

@@ +43,5 @@
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST_180);
> +
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  if ("stopApplication" in aModule.controller) {

This was for mozmill 1.5, you can remove the condition.

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test2.js
@@ +42,5 @@
> +  prefs.preferences.clearUserPref(PREF_XPI_WHITELIST_180);
> +
> +  // Bug 867217
> +  // Mozmill 1.5 does not have the restartApplication method on the controller.
> +  if ("stopApplication" in aModule.controller) {

Same. Remove the condition.

::: firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js
@@ +56,5 @@
> +  pickMonthInstall.waitForElement();
> +
> +  persisted.addonId = pickMonthInstall.getNode().parentNode.getAttribute("data-addonguid");
> +
> +  var btnGoToAddonInstall = discovery.getElements({type: "mainFeature_pickMonthInstall"})[0];

You should use `getElement` here.

@@ +88,2 @@
>  
>    var addon = am.getAddons({attribute: "name", value: persisted.currentAddon})[0];

I think you forgot to remove this line. (This is not present in the Beta patch, nor is it used here)
I've run tests against the Beta patch (so I am able to run them).

Please compare the Nightly vs the Beta patch to ensure the only differences are backport issues.
Attachment #8417850 - Flags: review?(andrei.eftimie)
Attachment #8417850 - Flags: review?(andreea.matei)
Attachment #8417850 - Flags: review-
Attached patch patch_v5.1[nightly] (obsolete) (deleted) — — Splinter Review 
(In reply to Andrei Eftimie from comment #127)
> @@ +1763,1 @@
> >        return {
> 
> nit: You can also omit the return statement, it is implied if there is only
> 1 expression.
It throws an exception because it will evaluate the object as a function block.
I fixed the other nits, I will return with the patch for beta and reports.
Attachment #8417850 - Attachment is obsolete: true
Attachment #8418677 - Flags: review?(hskupin)
No longer blocks: 780556
Attached patch 732353[beta].patch (obsolete) (deleted) — — Splinter Review 
This is beta patch, for testing.
Attachment #8418678 - Attachment is obsolete: true
Attached patch patch_v6.0[nightly] (obsolete) (deleted) — — Splinter Review 
Due to lots of changes I had to update the patch because it didn't applied anymore. 
testrun nightly
http://mozmill-crowd.blargon7.com/#/functional/report/12a8568e34c97b929089c7a61f9e177e
testrun beta
http://mozmill-crowd.blargon7.com/#/remote/report/12a8568e34c97b929089c7a61f9d208a
Attachment #8417435 - Attachment is obsolete: true
Attachment #8418677 - Attachment is obsolete: true
Attachment #8418677 - Flags: review?(hskupin)
Attachment #8483342 - Flags: review?(hskupin)
Attached patch patch v6.1 [default] (obsolete) (deleted) — — Splinter Review 
Patch needs to be updated because it didn't applied anymore

Nightly http://mozmill-crowd.blargon7.com/#/remote/report/3dac09185ffafe61214f5a99be1eb245
Beta http://mozmill-crowd.blargon7.com/#/remote/report/3dac09185ffafe61214f5a99be1b5a3f
Attachment #8483341 - Attachment is obsolete: true
Attachment #8483342 - Attachment is obsolete: true
Attachment #8483342 - Flags: review?(hskupin)
Attachment #8497517 - Flags: review?(andrei.eftimie)
Attachment #8497517 - Flags: review?(andreea.matei)
Attached patch 732353[beta].patch (obsolete) (deleted) — — Splinter Review 
This is patch for testing on beta
Attachment #604332 - Attachment is obsolete: true
Attachment #604335 - Attachment is obsolete: true
Attachment #604339 - Attachment is obsolete: true
Attachment #604340 - Attachment is obsolete: true
Attachment #604341 - Attachment is obsolete: true
Comment on attachment 8497517 [details] [diff] [review]
patch v6.1 [default]

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

Since this re-enables a number of tests, I've also calculated the extra time it will consume:
> new = 4m11s = 251 s
> old = 2m46s = 166 s
> difference = new-old = 85 seconds
> locales = 95 = 95
> parallel = 4 = 4
> difference × locales / parallel in min = 33,6458333333 min
I feel I've missed something, but this _should_ be correct.

::: firefox/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test2.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

It would be great to get some of these tests into a single file. Most of the 2nd test is setup and cleanup code.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +32,5 @@
>  /**
>   * Test the Larry displays as BLUE
>   */
>  function testLarryBlue() {
> +  // Bug 1046645

This has been fixed. Check if we can remove the workaround.

::: lib/addons.js
@@ +769,5 @@
> +      return;
> +    }
> +
> +
> +    assert.ok(category, "Category has been specified.");

The assert should come before the check.

@@ +1584,5 @@
> +    var elements;
> +
> +    var self = this;
> +    assert.waitFor(() => {
> +      elements = self.getElements({type: "addon_installButton"});

You should be fine using this directly with the fat arrow function syntax.

@@ +1588,5 @@
> +      elements = self.getElements({type: "addon_installButton"});
> +      return elements.length > 0;
> +    }, "The addon details page has been opened");
> +
> +    elements.some((aElement) => {

I don't understand what we're doing here.
I see we can get multiple results, but I'm seeing 2 different XPI's for the same addon (which is weird).

If we want the first one, shouldn't it be better to get the first one, and wait for it to be ready?

@@ +1595,5 @@
> +        return installButton.exists();
> +      }
> +    });
> +
> +    assert.ok(installButton.exists(), "Install button has been found");

This assert also doesn't work well with the above code.

@@ +1683,5 @@
> +  var addonInfo = [];
> +
> +  function filter(aAddons) {
> +    if (typeof aCallbackFilter === "function") {
> +      aAddons.forEach((aAddon) => {

nit: you can remove the braces around he argument

@@ +1703,5 @@
> +    AddonManager.getAllAddons(filter);
> +  }
> +
> +  assert.waitFor(() => {
> +    return addonInfo.length !== 0;

nit: can remove the curly braces and the return statement
Attachment #8497517 - Flags: review?(andrei.eftimie)
Attachment #8497517 - Flags: review?(andreea.matei)
Attachment #8497517 - Flags: review-
Attached patch patch v6.2 [beta] (obsolete) (deleted) — — Splinter Review 
I made the tests as a single file and moved the outside of restart directory.

http://mozmill-crowd.blargon7.com/#/remote/report/78c2fb649de7649b6e328c246615920f
Attachment #8497517 - Attachment is obsolete: true
Attachment #8497518 - Attachment is obsolete: true
Attachment #8500378 - Flags: review?(andrei.eftimie)
Attachment #8500378 - Flags: review?(andreea.matei)
Attached patch patch v6.2 [default] (obsolete) (deleted) — — Splinter Review 
This is the patch for default. The tests are ran on Beta and Release only.
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][sprint]
Comment on attachment 8500379 [details] [diff] [review]
patch v6.2 [default]

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

::: firefox/tests/remote/restartTests/testAddons_installFromFTP/test2.js
@@ +16,5 @@
> +// Mozprofile doesn't clear this pref while it is clearing all permissions
> +// TODO: Remove this once bug 951138 gets closed
> +const PREF_LAST_APP_VERSION = "extensions.lastAppVersion";
> +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> +const PREF_XPI_WHITELIST_180 = "xpinstall.whitelist.add.180";

So what's with all these preferences?

The only one we need should be:
> const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
And the only thing we need is to clear it:
> prefs.preferences.clearUserPref(PREF_XPI_WHITELIST);

At least this is what we already do, please check if this isn't enough.

::: firefox/tests/remote/restartTests/testDummy/manifest.ini
@@ +1,2 @@
>  [parent:../manifest.ini]
>  

If you remove this, you can remove the whole Dummy folder.

::: firefox/tests/remote/testDiscoveryPane/testInstallFeaturedAddon.js
@@ +1,1 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Same feedback for this test as the other 2 DiscoveryPane tests.

::: firefox/tests/remote/testDiscoveryPane/testInstallPickOfTheMonthAddon.js
@@ +76,5 @@
>    am.setCategory({category: am.getCategoryById({id: "discover"})});
>    var discovery = am.discoveryPane;
> +
> +  let pickMonthInstall = new elementslib.ID(controller.window.document,
> +                                            "monthly");

This should be in the library, "mainFeature_pickMonthWrapper"
Please also rename the variable, this is not the install button.

@@ +82,5 @@
> +
> +  persisted.addonId = pickMonthInstall.getNode().parentNode.getAttribute("data-addonguid");
> +
> +  var btnGoToAddonInstall = discovery.getElement({type: "mainFeature_pickMonthInstall"});
> +  btnGoToAddonInstall.click();

We need 2 events here
1) bring the "Pick of the Month" carousel entry into view
2) Click on the install button

For 1) issue a click on the "mainFeature_pickMonthWrapper" element.
For 2) issue a click on the "mainFeature_pickMonthInstall" element.

@@ +89,2 @@
>    // Install the addon
> +  var addToFirefox = discovery.getInstallButton();

The above 2) should probably be done here.

@@ +98,5 @@
>    var md = new modalDialog.modalDialog(am.controller.window);
>    md.start(addons.handleInstallAddonDialog);
> +  var tabOpened = false;
> +  function checkNewTabOpened() { tabOpened = true; }
> +  controller.window.addEventListener("TabOpen", checkNewTabOpened, false);

Can we use?
> tabBrowser.openTab({ method: "callback", callback: () => {
>   addToFirefox.click();
> }})

@@ +124,5 @@
> +
> +/**
> + * Check that add-on has been installed
> + */
> +function testPickOfTheMonthAddonIsInstalled() {

We didn't had this previously. We would have failed for addons which required restart. Good idea.

@@ +125,5 @@
> +/**
> + * Check that add-on has been installed
> + */
> +function testPickOfTheMonthAddonIsInstalled() {
> +  var addonType = addons.getAllAddons(aAddon => {

This whole call should be only:
> var addonType = addons.getAllAddons(aAddon => (aAddon.id === persisted.addonId), true)[0].type;

@@ +133,5 @@
> +      return aAddon;
> +    }
> +
> +    return null;
> +  })[0].type;

We should pass in `true` as a second argument to only returned installed addons as noted above.

@@ +140,5 @@
> +
> +  var addon = am.getAddons({attribute: "value", value: persisted.addonId})[0];
> +  var addonIsInstalled = am.isAddonInstalled({addon: addon});
> +  assert.ok(addonIsInstalled, "Extension '" + addon.getNode().getAttribute("name") +
> +                              "' has been installed");

Use `am.isAddonInstalled({addon: addon})` directly in the assert call.

::: firefox/tests/remote/testDiscoveryPane/testInstallUpAndComingAddon.js
@@ +81,2 @@
>    // Wait for the Get Add-ons pane to load
> +  am.setCategory({category: am.getCategoryById({id: "discover"})});

This doesn't do anything...

@@ +104,2 @@
>    // Install the addon
> +  var addToFirefox = discovery.getInstallButton();

As said in addons, revert this change please.

@@ +116,2 @@
>  
> +  try {

Same as the other test, can we use tabBrowser?

Edit. I've thought about this a bit. *Some* addons will open a new tab.
Instead of dealing with it, wouldn't it be easier to switch back to amo before restarting Firefox (or after restart)?
It might be easier to just ignore any potential new opened tab...

@@ +137,5 @@
> +/**
> + * Check that add-on has been installed
> + */
> +function testUpAndComingAddonIsInstalled() {
> +  var addonType = addons.getAllAddons(aAddon => {

Same as the other test please.

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +26,5 @@
> +  try {
> +    addons.getAllAddons(aAddon => {
> +      return aAddon && aAddon.isActive &&
> +             aAddon.type === "plugin" && aAddon.name === "Shockwave Flash"
> +    }, true);

You can simplify this a bit:
>    addons.getAllAddons(aAddon => aAddon.isActive &&
>                                  aAddon.type === "plugin" &&
>                                  aAddon.name === "Shockwave Flash",
>                        true);

::: lib/addons.js
@@ +1528,5 @@
>        case "mainFeature_ryfLearnMore":
>          nodeCollector.queryNodes(".ryff .more>a");
>          break;
>        case "mainFeature_pickMonthInstall":
> +        nodeCollector.queryNodes("#monthly");

This should not be changed.

@@ +1557,5 @@
>          nodeCollector.queryNodes("#back a");
>          break;
> +      case "addon_continueToDownload":
> +        nodeCollector.queryNodes("a.button.eula.go");
> +        break

nit: missing semicolon

@@ +1576,5 @@
> +   * Get the "Add to Firefox" button
> +   *
> +   * @returns {object} Element that is the install button for the addon
> +   */
> +  getInstallButton : function DiscoveryPane_getInstallButton() {

I had to do a bit of digging to figure out what this does.

This method does get the installButton when we are on an addon page.
When we are on the AMO homepage it returns the installButton for the "Pick of the Month" addon // I don't think this works as expected from what I see in the "Pick of the Month" test.

Please just use discovery.getElement('addon_installButton')

@@ +1732,5 @@
> + * @returns {boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS.some(aBlacklistedAddon => {
> +    return aBlacklistedAddon.id === aAddon.getNode().parentNode.getAttribute("data-guid");

You can remove the curly braces and the return statement.

@@ +1785,5 @@
>          name : aAddon.name,
>          version : aAddon.version,
>          isActive : aAddon.isActive,
>          isCompatible : aAddon.isCompatible
>        }

This should have the second argument set to `true`.
Attachment #8500379 - Flags: review-
Attached patch patch v7.0 [default] (obsolete) (deleted) — — Splinter Review 
(In reply to Andrei Eftimie from comment #139)
> ::: firefox/tests/remote/restartTests/testAddons_installFromFTP/test2.js
> @@ +16,5 @@
> > +// Mozprofile doesn't clear this pref while it is clearing all permissions
> > +// TODO: Remove this once bug 951138 gets closed
> > +const PREF_LAST_APP_VERSION = "extensions.lastAppVersion";
> > +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> > +const PREF_XPI_WHITELIST_180 = "xpinstall.whitelist.add.180";
> 
> So what's with all these preferences?
All of them need to be cleared, if we remove them the AMO won't work after an installation of an addon, you can remove them and ran a test to see it.

> ::: firefox/tests/remote/testDiscoveryPane/testInstallUpAndComingAddon.js
> @@ +81,2 @@
> >    // Wait for the Get Add-ons pane to load
> > +  am.setCategory({category: am.getCategoryById({id: "discover"})});
> 
> This doesn't do anything...
It does if the default category is different from "discover", if we were on a different tab and the discovery pane needs to load.

> 
> @@ +116,2 @@
> >  
> > +  try {
> 
> Same as the other test, can we use tabBrowser?
> 
> Edit. I've thought about this a bit. *Some* addons will open a new tab.
> Instead of dealing with it, wouldn't it be easier to switch back to amo
> before restarting Firefox (or after restart)?
> It might be easier to just ignore any potential new opened tab...
I've done that, looks like AddonsManager.open handles the exceptions if is already open so it's safe to just close in teardownTest and open it in setupTest.

> > +   */
> > +  getInstallButton : function DiscoveryPane_getInstallButton() {
> 
> I had to do a bit of digging to figure out what this does.
> 
> This method does get the installButton when we are on an addon page.
> When we are on the AMO homepage it returns the installButton for the "Pick
> of the Month" addon // I don't think this works as expected from what I see
> in the "Pick of the Month" test.
We have to wait for element, as i't appended to the page asynchronously and nodeCollector doesn't search again, so I switched this to findElement, and use waitForElement() method to be sure the node is ready, it's cleaner indeed. 

http://mozmill-crowd.blargon7.com/#/remote/report/e5ac82d610ad4751cf2efea91501bc30
http://mozmill-crowd.blargon7.com/#/remote/report/e5ac82d610ad4751cf2efea915014e35
Attachment #8500378 - Attachment is obsolete: true
Attachment #8500379 - Attachment is obsolete: true
Attachment #8500378 - Flags: review?(andrei.eftimie)
Attachment #8500378 - Flags: review?(andreea.matei)
Attachment #8502353 - Flags: review?(andrei.eftimie)
Attached patch patch v7.0 [beta] (obsolete) (deleted) — — Splinter Review 

  

  



    
    

    
  
Comment on attachment 8502353 [details] [diff] [review]
patch v7.0 [default]

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

::: firefox/tests/remote/manifest.ini
@@ +2,5 @@
>  server-root = ../../../data
>  
>  [include:restartTests/manifest.ini]
>  [include:testAddons/manifest.ini]
> +[include:testDiscoveryPane/manifest.ini]

the DiscoveryPane tests should be part of the testAddons suite.

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test2.js
@@ +33,5 @@
>    delete persisted.addon;
>  
>    addons.resetDiscoveryPaneURL();
>    aModule.addonsManager.close();
> +  aModule.addonsManager.open("about:home");

Why do we need this? We usually call setCategory at the beginning of a test.
Either way, Kind of redundant to call close, and open on the next statement right before we shutDown firefox...

::: firefox/tests/remote/testDiscoveryPane/testInstallFeaturedAddon.js
@@ +79,5 @@
>  /**
>   * Verifies installation of a Featured add-on
>   */
>  function testInstallFeaturedAddon() {
> +  // Wait for the Get Add-ons pane to load

Preserve the same spacings as the other tests. (I'm fine either way, but all 3 should be consistent)

@@ +95,2 @@
>  
>    controller.click(randomAddon);

Please do randomAddon.click()

::: firefox/tests/remote/testDiscoveryPane/testInstallPickOfTheMonthAddon.js
@@ +78,5 @@
>    var discovery = am.discoveryPane;
> +
> +  var pickMonthWrapper = discovery.getElement({type: "mainFeature_pickMonthWrapper"});
> +  pickMonthWrapper.waitForElement();
> +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");

Please move this assignment after the pickMonthWrapper block.

@@ +80,5 @@
> +  var pickMonthWrapper = discovery.getElement({type: "mainFeature_pickMonthWrapper"});
> +  pickMonthWrapper.waitForElement();
> +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");
> +
> +  pickMonthWrapper.click();

All these should come _after_ we wait for the discoveryPane to load.

@@ +87,2 @@
>    // Install the addon
> +  var addToFirefox = discovery.getElement({type: "addon_installButton"});

Please use discovery.getElement({type: "mainFeature_pickMonthInstall"});

@@ +104,5 @@
> +/**
> + * Check that add-on has been installed
> + */
> +function testPickOfTheMonthAddonIsInstalled() {
> +  var addonType = addons.getAllAddons(aAddon => aAddon.id == persisted.addonId)[0].type;

nit: use ===
You could also split this in 2 lines for better readability.

::: firefox/tests/remote/testDiscoveryPane/testInstallUpAndComingAddon.js
@@ +19,5 @@
> +// Mozprofile doesn't clear this pref while it is clearing all permissions
> +// TODO: Remove this once bug 951138 gets closed
> +const PREF_LAST_APP_VERSION = "extensions.lastAppVersion";
> +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> +const PREF_XPI_WHITELIST_180 = "xpinstall.whitelist.add.180";

From my testing it is enough to clear "xpinstall.whitelist.add" after each test so we're not getting blocked.
What does `extensions.lastAppVersion` do?

Here's a report on OSX with only "xpinstall.whitelist.add"
http://mozmill-crowd.blargon7.com/#/remote/report/e5ac82d610ad4751cf2efea91521b326

@@ +93,2 @@
>  
>    controller.click(randomAddon);

Please do randomAddon.click();

@@ +95,5 @@
>    discovery.waitForPageLoad();
>  
> +  // In case the addon is with EULA
> +  var continueToDownload = discovery.getElement({type: "addon_continueToDownload"});
> +

nit: remove this empty line

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +24,5 @@
>  
>    // Skip test if we don't have Flash plugin enabled
> +  try {
> +    addons.getAllAddons(aAddon => {
> +      return aAddon && aAddon.isActive &&

You can remove the first check. This won't be called if there are no addons returned by the API's

::: lib/addons.js
@@ +49,5 @@
>  ];
>  
> +const BLACKLISTED_ADDONS = [
> +  {id: "jid1-ZAdIEUB7XOzOJw@jetpack", name: "DuckDuckGo Plus"}
> +];

We can simplify this quite a bit by making an object with the ID the key, and the name the value.
> const BLACKLISTED_ADDONS = {
>   "jid1-ZAdIEUB7XOzOJw@jetpack": "DuckDuckGo Plus"
> }

@@ +788,5 @@
>    setCategoryById : function AddonsManager_setCategoryById(aSpec) {
>      var spec = aSpec || { };
>      var id = spec.id;
> +    assert.ok(id, "Category ID has been specified.");
> +    if (this.selectedCategory.getNode().isEqualNode(category.getNode())) {

This is not right. You get an id, yet you check the category...

@@ +1553,2 @@
>          break;
>        case "upAndComing_addons":

We should name this better, as the same markup is used in both "Up & Coming" and "Featured Add-ons", and it gets confusing to retireve "upAndComing_addons" when we actually get back the Featured addons...

Something like "addon_list_items"

@@ +1564,2 @@
>        case "addon_installButton":
> +        return [findElement.Selector(root, ".install-button a")];

I've found another expection here.
The Ageless for YouTube addon: https://services.addons.mozilla.org/en-US/firefox/discovery/addon/ageless/?src=discovery-upandcoming
has 3 different versions based on the OS you are on (linux, win, osx), and AMO shows 3 different buttons for each of those.

With the current selector we will always return the first link, so the test will fail with this type of addons on any other platforms. We should make sure to return the first visible item... Not sure yet what the best approach for that would be right now.

@@ -1550,5 @@
> -        // Bug 668976
> -        // No easy way to handle the multiple platform case. We have to filter
> -        // by the final computed CSS style for the element
> -        if (nodeCollector.nodes.length > 1)
> -          nodeCollector.filterByCSSProperty("display", "inline-block");

Ah, so this was the fix for the above mentioned issue...

@@ +1645,5 @@
> + *
> + * @param {boolean} [aInstalled=false]
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons
> + *        (installed or in progress of being installed)

"Return all addons (installed or in progress).
 If true return only installed addons"

@@ +1648,5 @@
> + *        If set to false, the function will return all addons
> + *        (installed or in progress of being installed)
> + *
> + * @returns {addon[]}
> + *          The array of installed addons or in progress of being installed

"Array of..."

@@ +1654,2 @@
>   */
> +function getAllAddons(aCallbackFilter, aInstalled = false) {

nit: remove the spaces around the equal sign

@@ +1654,3 @@
>   */
> +function getAllAddons(aCallbackFilter, aInstalled = false) {
> +  var addonInfo = [];

Can we name this: addonList ?

@@ +1657,5 @@
> +
> +  function filter(aAddons) {
> +    if (typeof aCallbackFilter === "function") {
> +      aAddons.forEach(aAddon => {
> +        var addon = aAddon.addon || aAddon;

Add a short comment above this line explaining the API difference between getAllInstalles vs getAllAddons

@@ +1692,5 @@
> + */
> +function getRandomAddon(aAddonList) {
> +  var addonList = aAddonList.filter(isBlacklisted);
> +  var randomIndex = Math.floor(Math.random() * addonList.length);
> +  var randomAddon = addonList[randomIndex];

We can return directly this result.

@@ +1707,5 @@
> + * @returns {boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS.some(aBlacklistedAddon =>
> +                                  aBlacklistedAddon.id === aAddon.getNode().parentNode.getAttribute("data-guid"));

And instead of iterating through an array we only need to check if the object contains the key:
> function isBlacklisted(aAddon) {
>   var id = aAddon.getNode().parentNode.getAttribute("data-guid");                                  
>   return BLACKLISTED_ADDONS.hasOwnProperty(id);
> }

  

  



        Attachment #8502353 -
        Flags: review?(andrei.eftimie) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v7.5 [default] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I had to add an asertion to check if the addon is compatible, "FT GraphiteGlow" for example is not compatible with version higher than 28 so I had to blacklist it.
http://mozmill-crowd.blargon7.com/#/remote/report/c1ae8473ee5b384b83bbfde3312f82c7
http://mozmill-crowd.blargon7.com/#/remote/report/c1ae8473ee5b384b83bbfde3312176d1

  

  



        Attachment #8502353 -
        Attachment is obsolete: true

        Attachment #8502354 -
        Attachment is obsolete: true

        Attachment #8504043 -
        Flags: review?(andrei.eftimie)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v7.6 [beta] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
(In reply to Cosmin Malutan from comment #143)
> Created attachment 8504043 [details] [diff] [review]
> patch v7.5 [default]
> 
> I had to add an asertion to check if the addon is compatible, "FT
> GraphiteGlow" for example is not compatible with version higher than 28 so I
> had to blacklist it.
> http://mozmill-crowd.blargon7.com/#/remote/report/
> c1ae8473ee5b384b83bbfde3312f82c7
> http://mozmill-crowd.blargon7.com/#/remote/report/
> c1ae8473ee5b384b83bbfde3312176d1

You should have provided more info here.
So the mentioned addon "FT GraphiteGlow" is only compatible with Firefox < 28 and at the same time it is listed in AMO as an "Up & Coming" addon. This looks like an issue for AMO

Please raise a bug against AMO for that.

  

  



    
    

    
  
Comment on attachment 8504044 [details] [diff] [review]
patch v7.6 [beta]

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

With yesterday's merge this doesn't apply on beta cleanly anymore. Sorry for that.
At least the patch for default/aurora still applies, and only has 1 conflict against beta.

  

  



        Attachment #8504044 -
        Flags: review-

    
    

    
  
Comment on attachment 8504043 [details] [diff] [review]
patch v7.5 [default]

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

::: firefox/tests/remote/testAddons/testDiscoveryPane/testInstallPickOfTheMonthAddon.js
@@ +89,3 @@
>  
>    // Install the addon
> +  var addToFirefox = discovery.getElement({type: "addon_installButton"});

If we keep this instead of the custom pickMonthInstall query, please add the relevant parent:
> parent: pickMonthWrapper

::: firefox/tests/remote/testAddons/testDiscoveryPane/testInstallUpAndComingAddon.js
@@ +96,5 @@
> +    continueToDownload.click();
> +    discovery.waitForPageLoad();
> +  }
> +
> +  // Check that addon is compatible

This is a nice catch.

It would be great to have this in the addons library directly, so we won't have to add it in all test.
Could you please check if there's a good place for this check. I assume we'd want to do this right before trying to install the addon, so maybe add a method that makes the check and has the assertion, and invoke it when we retrieve `addon_installButton`

::: firefox/tests/remote/testPrivateBrowsing/testFlashCookie.js
@@ +26,5 @@
> +  try {
> +    addons.getAllAddons(aAddon => {
> +      return aAddon.isActive && aAddon.type === "plugin" &&
> +             aAddon.name === "Shockwave Flash"
> +    }, true);

What about:
> addons.getAllAddons(aAddon => aAddon.isActive &&
>                               aAddon.type === "plugin" &&
>                               aAddon.name === "Shockwave Flash",
>                     true);

::: lib/addons.js
@@ +1696,5 @@
> + * @returns {object} A random addon that is not blacklisted
> + */
> +function getRandomAddon(aAddonList) {
> +  var addonList = aAddonList.filter(isBlacklisted);
> +  var GraphiteGlow  = aAddonList.forEach(function(addon){

Please don't add any specific addon in here.
You added it in the blacklist, it should be filtered by the above filter method.

  

  



        Attachment #8504043 -
        Flags: review?(andrei.eftimie) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8.0 [default] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
(In reply to Andrei Eftimie from comment #147)
> firefox/tests/remote/testAddons/testDiscoveryPane/
> testInstallUpAndComingAddon.js
> @@ +96,5 @@
> > +    continueToDownload.click();
> > +    discovery.waitForPageLoad();
> > +  }
> > +
> > +  // Check that addon is compatible
> 
> This is a nice catch.
> 
> It would be great to have this in the addons library directly, so we won't
> have to add it in all test.
> Could you please check if there's a good place for this check. I assume we'd
> want to do this right before trying to install the addon, so maybe add a
> method that makes the check and has the assertion, and invoke it when we
> retrieve `addon_installButton`
I made this a method, and indeed this brings in a lot of value, because if the add-on won't be compatible we we won't just fail with "Extension has been installed", we will know that's not compatible and has to be black-listed and maybe contact the developer or file an bug to don't be listed as a featured add-on in discovery pane if is not compatible with latest Release.  

Reports
http://mozmill-crowd.blargon7.com/#/remote/report/02345d0791a18ffc72a4b042ab590310
http://mozmill-crowd.blargon7.com/#/remote/report/02345d0791a18ffc72a4b042ab591249

Thanks for review Andrei ;)

  

  



        Attachment #8504043 -
        Attachment is obsolete: true

        Attachment #8504044 -
        Attachment is obsolete: true

        Attachment #8505436 -
        Flags: review?(andrei.eftimie)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8.0 [beta] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
Comment on attachment 8505436 [details] [diff] [review]
patch v8.0 [default]

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

I feel most rough edges have been fixed.
Seems to be working fine from my tests.

Tempted to give this an r+, but with a grain of salt, I feel I'm to close now and might not notice obvious things.
But I'd like some other pair of eyes across this, 

We'll have to come with a good deployment strategy. These tests have had a poor success rate in the past.
I don't want to simply push these. Since these tests only run on Beta & Release combined with the fact that we run all locales on these branches... I'd feel comfortable that we get to test this somehow prior to a full beta run.

  

  



        Attachment #8505436 -
        Flags: review?(hskupin)

        Attachment #8505436 -
        Flags: review?(andrei.eftimie)

        Attachment #8505436 -
        Flags: review?(andreea.matei)

        Attachment #8505436 -
        Flags: review+

    
    

    
  
Comment on attachment 8505436 [details] [diff] [review]
patch v8.0 [default]

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

This slipped through my review because there is still a request open on that attachment for Andreea. Is that still valid?

  

  



    
    

    
  
Comment on attachment 8505436 [details] [diff] [review]
patch v8.0 [default]

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

No, please have a look as Andrei already did several times and tested it.

  

  



        Attachment #8505436 -
        Flags: review?(andreea.matei)

    
    

    
  
Comment on attachment 8505436 [details] [diff] [review]
patch v8.0 [default]

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

::: firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test2.js
@@ +14,5 @@
>  
> +// Bug 951138
> +// Mozprofile doesn't clear this pref while it is clearing all permissions
> +// TODO: Remove this once bug 951138 gets closed
> +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";

I do not see why we explicitly have to call this out. I would just clean it up and all good.

::: firefox/tests/remote/testAddons/testDiscoveryPane/testInstallFeaturedAddon.js
@@ +34,5 @@
> +                                              "Nightly and Aurora versions";
> +    testFeaturedModuleIsInstalled.__force_skip__ = "Discovery tests are not run on " +
> +                                                   "Nightly and Aurora versions";
> +    teardownModule.__force_skip__ = "Discovery tests are not run on Nightly and " +
> +                                    "Aurora versions";

Please declare the message string once and reuse it as a variable.

@@ +46,3 @@
>  
>    tabs.closeAllTabs(aModule.controller);
> +  aModule.am.open();

Please leave this in the test.

@@ +63,5 @@
> +  aModule.controller.stopApplication(true);
> +}
> +
> +function teardownTest(aModule) {
> +  let notification = aModule.locationBar.getNotification();

Please be consistent with var and let but do not mix. Make it a var.

@@ +100,3 @@
>    // Install the addon
>    var addToFirefox = discovery.getElement({type: "addon_installButton"});
> +  discovery.checkAddonIsCompatible(addToFirefox);

I do not see why checkAddonIsCompatible cannot fetch the element itself.

::: firefox/tests/remote/testAddons/testDiscoveryPane/testInstallPickOfTheMonthAddon.js
@@ +75,5 @@
>    discovery.waitForPageLoad();
>  
> +  var pickMonthWrapper = discovery.getElement({type: "mainFeature_pickMonthWrapper"});
> +  pickMonthWrapper.waitForElement();
> +  pickMonthWrapper.click();

Why not .waitThenClick()?

@@ +80,2 @@
>  
> +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");

Cannot we get the target node directly?

@@ +107,1 @@
>  

If the line is too long, use {} and put the body into its own line.

::: lib/addons.js
@@ +50,5 @@
>  
> +const BLACKLISTED_ADDONS = {
> +  "jid1-ZAdIEUB7XOzOJw@jetpack" : "DuckDuckGo Plus",
> +  "{99e34760-2754-11e0-91fa-0800200c9a66}" : "FT GraphiteGlow"
> +};

Why do we hard-code possible blacklisted add-ons here? This can clearly cause problems.

@@ +1664,5 @@
>   *        and returns a filtered version.
> + *
> + * @param {boolean} [aInstalled=false]
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons (installed or in progress)

I feel the first line is enough.

@@ +1667,5 @@
> + *        If set to true, the function will return only the installed addons
> + *        If set to false, the function will return all addons (installed or in progress)
> + *
> + * @returns {addon[]}
> + *          Array of installed addons or in progress of being installed

Array of add-ons

@@ +1697,5 @@
> +    AddonManager.getAllAddons(filter);
> +  }
> +
> +  assert.waitFor(() => addonsList.length !== 0,
> +                 "Addons are installed");

This could wait forever if no add-ons are returned.

@@ +1726,5 @@
> + * @returns {boolean} True if the addon is blacklisted
> + */
> +function isBlacklisted(aAddon) {
> +  return !BLACKLISTED_ADDONS[aAddon.getNode().parentNode
> +                             .getAttribute("data-guid")];

So we do not see the blacklist state on AMO?

  

  



        Attachment #8505436 -
        Flags: review?(hskupin) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8.1 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I moved the AOM opening in tests itself s from setupTest as requested and I made the skip messages constants.
I also fixed the other remaining nits.
http://mozmill-crowd.blargon7.com/#/remote/report/a6906174787fc0d722108aa1faa338c6
http://mozmill-crowd.blargon7.com/#/remote/report/a6906174787fc0d722108aa1faa3b825

(In reply to Henrik Skupin (:whimboo) from comment #153)
> > +// TODO: Remove this once bug 951138 gets closed
> > +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> 
> I do not see why we explicitly have to call this out. I would just clean it
> up and all good.
For consistency, you requested so when we had this issue in a different bug. 
bug 944337 comment 29
 
> @@ +100,3 @@
> >    // Install the addon
> >    var addToFirefox = discovery.getElement({type: "addon_installButton"});
> > +  discovery.checkAddonIsCompatible(addToFirefox);
> 
> I do not see why checkAddonIsCompatible cannot fetch the element itself.
Because sometimes we might have more addons in the same page and we expect to check a specific one. When we get the the element in test we might give the parent node too, this is the case for pickOfTheMonth
http://hg.mozilla.org/qa/mozmill-tests/file/c202ab5d4f74/firefox/tests/remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js#l67
Also fetching it in the module will just require an extra search in DOM, those are expensive.

> >  
> > +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");
> 
> Cannot we get the target node directly?
It has no specific selector, and it's use just here. 

> > +const BLACKLISTED_ADDONS = {
> > +  "jid1-ZAdIEUB7XOzOJw@jetpack" : "DuckDuckGo Plus",
> > +  "{99e34760-2754-11e0-91fa-0800200c9a66}" : "FT GraphiteGlow"
> > +};
> 
> Why do we hard-code possible blacklisted add-ons here? This can clearly
> cause problems.
They are not incompatible addons, are just addons that require specific steps for installations and we decided not to handle them but to avoid them.
See comment 79.  

> > +
> > +  assert.waitFor(() => addonsList.length !== 0,
> > +                 "Addons are installed");
> 
> This could wait forever if no add-ons are returned.
We have a default timeout of 5 seconds, and we expect to fail if there are no addons.

> @@ +1726,5 @@
> > +function isBlacklisted(aAddon) {
> > +  return !BLACKLISTED_ADDONS[aAddon.getNode().parentNode
> > +                             .getAttribute("data-guid")];
> 
> So we do not see the blacklist state on AMO?
I explained this above(comment 79).

  

  



        Attachment #8505436 -
        Attachment is obsolete: true

        Attachment #8523748 -
        Flags: review?(hskupin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8.1 [beta] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

  

  



        Attachment #8505437 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Cosmin Malutan from comment #154)
> (In reply to Henrik Skupin (:whimboo) from comment #153)
> > > +// TODO: Remove this once bug 951138 gets closed
> > > +const PREF_XPI_WHITELIST = "xpinstall.whitelist.add";
> > 
> > I do not see why we explicitly have to call this out. I would just clean it
> > up and all good.
> For consistency, you requested so when we had this issue in a different bug. 
> bug 944337 comment 29

I see. So leave it as it is for now.

> > I do not see why checkAddonIsCompatible cannot fetch the element itself.
> Because sometimes we might have more addons in the same page and we expect
> to check a specific one. When we get the the element in test we might give
> the parent node too, this is the case for pickOfTheMonth
> http://hg.mozilla.org/qa/mozmill-tests/file/c202ab5d4f74/firefox/tests/
> remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js#l67
> Also fetching it in the module will just require an extra search in DOM,
> those are expensive.

I see, but passing in the install button does not really correlate to the method name we have here. It's confusing. Is there no way to pass in the add-on name or id, and find the element by selector? It should not about having to search again, it's about consistency. Otherwise maybe we can rename the helper method like 'isInstallable'?

> > > +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");
> > 
> > Cannot we get the target node directly?
> It has no specific selector, and it's use just here. 

Why can't we use the classes 'install clickHijack' for searching? I checked a couple of different pages in the discovery pane and as it looks like this element always contains the information we want.

> > > +const BLACKLISTED_ADDONS = {
> > > +  "jid1-ZAdIEUB7XOzOJw@jetpack" : "DuckDuckGo Plus",
> > > +  "{99e34760-2754-11e0-91fa-0800200c9a66}" : "FT GraphiteGlow"
> > > +};
> > 
> > Why do we hard-code possible blacklisted add-ons here? This can clearly
> > cause problems.
> They are not incompatible addons, are just addons that require specific
> steps for installations and we decided not to handle them but to avoid them.
> See comment 79.  

So you cannot call them blacklisted then. This causes quite a lot of confusion. Please find a better name.

> > > +  assert.waitFor(() => addonsList.length !== 0,
> > > +                 "Addons are installed");
> > 
> > This could wait forever if no add-ons are returned.
> We have a default timeout of 5 seconds, and we expect to fail if there are
> no addons.

Why do we expect to fail if there are no addons? This is not something this method has to decide. It's part of the test, if the querying options restrict the available add-ons that much, so no add-on is found.

Next time please refrain from putting up another patch for review until an ongoing discussion has been finished. It only delays the landing given that reviews will need more time as a quick answer.

  

  



        Attachment #8523748 -
        Flags: review?(hskupin) → review-

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #156)
> > > I do not see why checkAddonIsCompatible cannot fetch the element itself.
> > Because sometimes we might have more addons in the same page and we expect
> > to check a specific one. When we get the the element in test we might give
> > the parent node too, this is the case for pickOfTheMonth
> > http://hg.mozilla.org/qa/mozmill-tests/file/c202ab5d4f74/firefox/tests/
> > remote/restartTests/testDiscoveryPane_installPickOfMonthAddon/test1.js#l67
> > Also fetching it in the module will just require an extra search in DOM,
> > those are expensive.
> 
> I see, but passing in the install button does not really correlate to the
> method name we have here. It's confusing. Is there no way to pass in the
> add-on name or id, and find the element by selector? It should not about
> having to search again, it's about consistency. Otherwise maybe we can
> rename the helper method like 'isInstallable'?
I would prefer to rename it, I see no reason to reinvent the wheel.

> > > > +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");
> > > 
> > > Cannot we get the target node directly?
> > It has no specific selector, and it's use just here. 
> 
> Why can't we use the classes 'install clickHijack' for searching? I checked
> a couple of different pages in the discovery pane and as it looks like this
> element always contains the information we want.
The element that contains the attribute we need(data-addonguid) it's the parent div, the one you specified contains other data about the addon, but not the one we need, see the screenshot below.
https://drive.google.com/file/d/0B0TFVdllG6sZRF8wRE5uVFhZTkE/view?usp=sharing


> > > > +const BLACKLISTED_ADDONS = {
> > > > +  "jid1-ZAdIEUB7XOzOJw@jetpack" : "DuckDuckGo Plus",
> > > > +  "{99e34760-2754-11e0-91fa-0800200c9a66}" : "FT GraphiteGlow"
> > > > +};
> > > 
> > > Why do we hard-code possible blacklisted add-ons here? This can clearly
> > > cause problems.
> > They are not incompatible addons, are just addons that require specific
> > steps for installations and we decided not to handle them but to avoid them.
> > See comment 79.  
> 
> So you cannot call them blacklisted then. This causes quite a lot of
> confusion. Please find a better name.
Blacklisted stays for something that we don't want to ran, Block-listed is something that it's broken and therefor prevented from being installed.
BLACKLISTED_ADDONS sounds reasonable for me, otherwise I would suggest AVOIDED_ADDONS, PREVENTED_ADDONS or EXCEPTIONAL_ADDONS.

  

  



    
    

    
  
Whimboo should I go with the implementation I suggested?

  

  


Flags: needinfo?(hskupin)

    
    

    
  
(In reply to Cosmin Malutan from comment #157)
> > I see, but passing in the install button does not really correlate to the
> > method name we have here. It's confusing. Is there no way to pass in the
> > add-on name or id, and find the element by selector? It should not about
> > having to search again, it's about consistency. Otherwise maybe we can
> > rename the helper method like 'isInstallable'?
> I would prefer to rename it, I see no reason to reinvent the wheel.

So lets do that.

> > > > > +  persisted.addonId = pickMonthWrapper.getNode().parentNode.getAttribute("data-addonguid");
> > > > 
> > > > Cannot we get the target node directly?
> > > It has no specific selector, and it's use just here. 
> > 
> > Why can't we use the classes 'install clickHijack' for searching? I checked
> > a couple of different pages in the discovery pane and as it looks like this
> > element always contains the information we want.
> The element that contains the attribute we need(data-addonguid) it's the
> parent div, the one you specified contains other data about the addon, but
> not the one we need, see the screenshot below.
> https://drive.google.com/file/d/0B0TFVdllG6sZRF8wRE5uVFhZTkE/view?usp=sharing

Hm, I see. It's kinda bad mark-up of that page. The monthly class should have been put onto the li element. So there is no other way to test this now. We would have to risk that our tests are failing in case they inject other elements/nodes in between those two nodes. A safer approach would be to have a loop which iterates through parentNode until the current node is the panel.  

> Blacklisted stays for something that we don't want to ran, Block-listed is
> something that it's broken and therefor prevented from being installed.
> BLACKLISTED_ADDONS sounds reasonable for me, otherwise I would suggest
> AVOIDED_ADDONS, PREVENTED_ADDONS or EXCEPTIONAL_ADDONS.

So lets keep the name but please add comments why they are blacklisted. I tried to install the DuckDuckGo add-on but haven't seen a problem locally.

  

  


Flags: needinfo?(hskupin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v9.0 [default] (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I made the changes requested:
 * added a comment above the BLACKLISTED_ADDONS constant.
 * made helper method in utils to get an attribute from parent node, which walks up to the tree until it finds it!
 * removed the check for length in getAllAddons and leaved the assertion to check only if the array has been updated by setting it initially to null. 

http://mozmill-crowd.blargon7.com/#/remote/report/635fcffb06b246be47416301bd69c0f9

  

  



        Attachment #8523748 -
        Attachment is obsolete: true

        Attachment #8525998 -
        Flags: review?(hskupin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v10.0 default (deleted)
        — 
          — Splinter Review
      

    


  
I updated the patch, so it can be applied again.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/dd0cbbf05a3aab4a7676502a86503e00

  

  



        Attachment #8525998 -
        Attachment is obsolete: true

        Attachment #8525998 -
        Flags: review?(hskupin)

        Attachment #8551767 -
        Flags: review?(hskupin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v10.0 beta (deleted)
        — 
          — Splinter Review
      

    


  
Report:
http://mozmill-crowd.blargon7.com/#/remote/report/dd0cbbf05a3aab4a7676502a86523a06

  

  



        Attachment #8523762 -
        Attachment is obsolete: true

        Attachment #8551768 -
        Flags: review?(hskupin)

    
    

    
  
This patch had the duty of making the remote discovery pane test reliable again, see comment 7.
Also those tests are ran only agains beta and release and are skipped on nightly and aurora, that's why I've always had to double the patches, one for nightly and one for beta for testing.
The patch is in a final state, but since it didn't got a review in the past 3 months, I doubt it will get reviewed and landed today so I'll unassign.

  

  


Assignee: cosmin.malutan → nobody
Status: ASSIGNED → NEW

    
    

    
  
Comment on attachment 8551768 [details] [diff] [review]
patch v10.0 beta

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

We don't have the capacity to fix those tests. We will check later if it would be helpful to keep them.

  

  



        Attachment #8551768 -
        Flags: review?(hskupin)

        Attachment #8551767 -
        Flags: review?(hskupin)

    
    

    
  
Mozmill is dead, WONTFIX the remaining bugs.

  

  


Status: NEW → RESOLVED
Closed: 13 years ago → 6 years ago
Resolution: --- → WONTFIX

    
  
Product: Mozilla QA → Mozilla QA Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: