Closed Bug 718660 Opened 12 years ago Closed 12 years ago

Create script for add-ons compatible by default tests

Categories

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Automation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(3 files, 14 obsolete files)

List of top add-ons with explicit platform restrictions
451 bytes, text/plain
Details
List of addons with yahoo! toolbar removed. ftp.mozilla.org -> stage.mozilla.org
13.74 KB, application/json
Details
Patch v3.3
11.58 KB, patch
cmtalbert
: review+
gmealer
: feedback+
Details | Diff | Splinter Review 
For bug 718403 we need a script which will run a several of our test-runs in a specified order. Here the requirements:

* For each test a given number of fixed top add-ons have to be installed
* For each test a variable number of add-ons have to be installed
* Order of tasks to execute
** Install build
** Run endurance tests
** Run update tests (no fallback)
** Run endurance tests
** Eventually run functional tests
** Uninstall build

Tests will be executed for Firefox 9.0.1 -> Firefox 10.0 (Beta) on all platforms, and for the following locales in sequence: en-US, de, zh-CN 

We have two list of top addons. So we have to ensure which one we have to use. William can you please check this and reply here?

Top 100 add-ons:
https://docs.google.com/document/d/1prS9gvDUF15M7ASlRcZcK2zNBROTPuJgakhInr2Vnk4/edit?hl=en_US

Add-ons compatible with Firefox 9.0:
https://addons.mozilla.org/en-US/firefox/compatibility/9.0
(In reply to Henrik Skupin (:whimboo) from comment #0)
> For bug 718403 we need a script which will run a several of our test-runs in
> a specified order. Here the requirements:
> 
> * For each test a given number of fixed top add-ons have to be installed
> * For each test a variable number of add-ons have to be installed
> * Order of tasks to execute
> ** Install build
> ** Run endurance tests
> ** Run update tests (no fallback)
> ** Run endurance tests
> ** Eventually run functional tests
> ** Uninstall build
> 
> Tests will be executed for Firefox 9.0.1 -> Firefox 10.0 (Beta) on all
> platforms, and for the following locales in sequence: en-US, de, zh-CN 
> 
> We have two list of top addons. So we have to ensure which one we have to
> use. William can you please check this and reply here?

I got my first list of 10 from "Jorge Villalobos" and the original meeting notes suggest that fligtar (Justin Scott) would be the person to ask for the list of top 100 addons. 

Unfortunately the original meeting notes don't specify which of the top 100 addons would be used (i.e. are we restricting ourselves to the top 100 addons just on AMO? or the top 100 period?)

I'll CC both of them on this bug. Hopefully they'll be able to provide a master list of 100 addons that are suitable for this project.
Depends on: 718964
(In reply to William Lachance (:wlach) from comment #1)
> I got my first list of 10 from "Jorge Villalobos" and the original meeting
> notes suggest that fligtar (Justin Scott) would be the person to ask for the
> list of top 100 addons. 

William, I do not see a link to those add-ons. Can you please make sure to supply that list ASAP?

> Unfortunately the original meeting notes don't specify which of the top 100
> addons would be used (i.e. are we restricting ourselves to the top 100
> addons just on AMO? or the top 100 period?)
> 
> I'll CC both of them on this bug. Hopefully they'll be able to provide a
> master list of 100 addons that are suitable for this project.

Justin or Jorge, can you please hand us over the list of add-ons we have to test? It's really important for us to get those soonish, so we can at least have a test-run over the upcoming weekend. Thanks!
When asking for the list of extensions I will need them listed line by line with the download URL. Can we get the list in that format please?
(In reply to Henrik Skupin (:whimboo) from comment #2)
> (In reply to William Lachance (:wlach) from comment #1)
> > I got my first list of 10 from "Jorge Villalobos" and the original meeting
> > notes suggest that fligtar (Justin Scott) would be the person to ask for the
> > list of top 100 addons. 
> 
> William, I do not see a link to those add-ons. Can you please make sure to
> supply that list ASAP?

Sorry, I quoted it in a bunch of places but apparently not this bug. Here it is:

https://addons.mozilla.org/en-US/firefox/compatibility/4.0?type=all
Attached file Top Add-ons -WIP (obsolete) — 
Here's a list I've been working on. For DTC it matters if the add-on is binary or not, so I've been sorting them out accordingly. I have a little over 40 top non-binary add-ons on that list, some of which need some research in order to be found.
(In reply to Jorge Villalobos [:jorgev] from comment #5)
> Created attachment 589876 [details]
> Top Add-ons -WIP
> 
> Here's a list I've been working on. For DTC it matters if the add-on is
> binary or not, so I've been sorting them out accordingly. I have a little
> over 40 top non-binary add-ons on that list, some of which need some
> research in order to be found.

Would it be possible for you to create a text file with this information that could be read programatically? E.g.

"Adblock plus" http://addons.mozilla.org/addon/adblockplus.xpi
"Firebug" http://firebug.mozilla.org/addon/firebug.xpi
Ok, as discussed offline, here's a file with a list of addons in <name>: <url> format based on the original set provided by Jorge. I could not find direct links to the XPIs for all addons, these are noted in commented out sections.
I dumped the script I used to generate the original list here:

https://gist.github.com/1648112
(In reply to William Lachance (:wlach) from comment #7)
> <url> format based on the original set provided by Jorge. I could not find
> direct links to the XPIs for all addons, these are noted in commented out
> sections.

Lets see...

# * Facemoods seems to be distributed as an .exe. It also looks really scary.

This should be tested in any way.

# * Not sure where to get a copy of google toolbar

Yay, for our former addons tests for that addon:
http://hg.mozilla.org/qa/mozmill-tests/file/d1b7ff83104e/tests/addons/toolbar%40google.com/addon.ini

Seems like we would also have to mark specific add-ons platform specific because different XPI's could be distributed.

# * No direct link to Web.de toolbar (but you can get it by going to: http://web-de-toolbar.softonic.de/download)

http://sd-cf.softonic.de/316000/316307/891c5/webde_toolbar1.5.1.xpi?Policy=eyJTdGF0ZW1lbnQiOlt7IlJlc291cmNlIjoiaHR0cDpcL1wvc2QtY2Yuc29mdG9uaWMuZGVcLzMxNjAwMFwvMzE2MzA3XC84OTFjNVwvd2ViZGVfdG9vbGJhcjEuNS4xLnhwaSIsIkNvbmRpdGlvbiI6eyJEYXRlTGVzc1RoYW4iOnsiQVdTOkVwb2NoVGltZSI6MTMyNzExMjcyMn19fV19&Signature=gljGwQpv0fJ~zSGXm~D-Gppgmmd~fOSmT94CDTpdr6rIpSnCfV6NB4NA2t5vUi7ugkfTME4dbNecriMUjHl2t6otBPwo9OxZ8xz6a0k3~UWs19khegz4DaisZ3vRWsozcPhjNsddo9Smf~tm~9tdVqQJZ-Ucd1giSwMteNon-A4_&Key-Pair-Id=APKAJUA62FNWTI37JTGQ

# * No direct link to Somoto toolbar (but you can get it by going to: http://toolbar.somotoinc.com/)

This is also an exe installer and should be tested manually.

# * No direct link to cacaoweb (only see a binary download). Website is: http://www.cacaoweb.org/#

Is that a plugin?

# * No direct link to Ask.com Toolbar (website: http://sp.ask.com/toolbar/install/web/ask/download.php)

It's getting installed with various other tools. I would prefer testing manually. See a former project I was helping with:

https://wiki.mozilla.org/QA/Firefox3.6/TestPlan:DLL_Blocklisting:3rd-party

# * FT Deepdark is a JAR? (ftp://ftp.mozilla.org/pub/mozilla.org/addons/295337/ft_deepdark-2.9.7.3-fx-windows.jar)

So it's a theme and needs to be tested manually.

# * No direct link to AutoComplete Pro xpi (can download .exe at http://www.autocompletepro.com/welcome/)

If it's an exe please test it manually.

Otherwise I will take the current list for testing on my local box. Hopefully I can attach the first working patch later today. At latest it will be uploaded Monday morning.
Attached file List of addons in JSON format (obsolete) — 
Same list in json format, as requested
Attached file Addons in actually valid JSON (obsolete) — 
Oops, the previous attachment wasn't actually valid json
Attachment #590221 - Attachment is obsolete: true
Attachment #590222 - Attachment mime type: application/json → text/plain
William, that's way better! Thanks. I can start to work with this list. Beside that can we please make sure to also include if a specific entry is only available on specific platforms? If we don't have the information we will get failures for those addons. At least those will be the Google Toolbar and the .net Framework add-on. We could add an optional "platforms" property to an add-on with the platform names Mozmill supports.
Attached patch WIP v1 (obsolete) — — Splinter Review 
WIP which partly works. I can't really test it right now due to a bad internet connection. I will continue to work on it tomorrow so that we will still be able to execute a test run over the weekend.
This would be easy (In reply to Henrik Skupin (:whimboo) from comment #12)
> William, that's way better! Thanks. I can start to work with this list.
> Beside that can we please make sure to also include if a specific entry is
> only available on specific platforms? If we don't have the information we
> will get failures for those addons. At least those will be the Google
> Toolbar and the .net Framework add-on. We could add an optional "platforms"
> property to an add-on with the platform names Mozmill supports.

Yes, this would be easy enough to add. I guess the trick will be getting the information. 

Jorge: Does the AMO database have information on what platforms are supported by different addons?
Depends on: 668063
(In reply to William Lachance (:wlach) from comment #14)
> Jorge: Does the AMO database have information on what platforms are
> supported by different addons?

Yes. Every version of every add-on on AMO can either support All platforms, or support a subset of Windows, Linux and Mac.
Ok, we got a problem. Being compatible by default in Firefox 10 doesn't mean that the software update module in Firefox <10 takes care of it too. Whenever add-ons are not compatible with the target version of Firefox the updater will display a separate page of the *old* wizard implementation.

That means I will have to fix this before we can run our tests for all the given add-ons. Right now only a small subset would be possible to test. I will make it my priority for today.
Depends on: 599290
(In reply to Jorge Villalobos [:jorgev] from comment #15)
> (In reply to William Lachance (:wlach) from comment #14)
> > Jorge: Does the AMO database have information on what platforms are
> > supported by different addons?
> 
> Yes. Every version of every add-on on AMO can either support All platforms,
> or support a subset of Windows, Linux and Mac.

Could you send out a report on the platform information for the list you sent out last week?
(In reply to Henrik Skupin (:whimboo) from comment #16)
> Ok, we got a problem. Being compatible by default in Firefox 10 doesn't mean
> that the software update module in Firefox <10 takes care of it too.
> Whenever add-ons are not compatible with the target version of Firefox the
> updater will display a separate page of the *old* wizard implementation.
> 
> That means I will have to fix this before we can run our tests for all the
> given add-ons. Right now only a small subset would be possible to test. I
> will make it my priority for today.

My understanding from the meeting was that we were just going to attempt to install the addons directly in the 10 betas? In that case, the problem should not be triggered, should it?

https://wiki.mozilla.org/Auto-tools/Projects/Compatibile_By_Default_Automation#Deliverable
No, we have to run update tests. Otherwise we do not fully test this new feature. Probably >95% of our upcoming users for Firefox 10 will upgrade from former releases, which include 9.0.x down to 3.6.x.
We should not depend on bug 599290 which would need much efforts to be fixed at this stage. Instead we should simply implement the correct handling of the update wizard dialog and add support for the missing wizard page. I will file a separate bug for it.
No longer depends on: 599290
Depends on: 720546
Depends on: 630388
Attached patch WIP v2 (obsolete) — — Splinter Review 
Updated WIP which for now performs the following:

* Stage all the listed add-ons on the local disk so we do not have to download them over and over again
* Checks the given Firefox build by running the tests with the given amount of add-ons installed
* For now performs functional tests, update tests, and functional tests again
* Sends the report to a dashboard - I have now created http://mozmill-addons.blargon7.com/

We should check in which amount it is useful to run the endurance tests. I will do some test-runs tomorrow.

William, do we have made any progress on the final list of add-ons including the platforms which are supported? My script will be ready by tomorrow.
Attachment #590311 - Attachment is obsolete: true
Here's the list of platforms that are specified for the top AMO add-ons on the previous list. A few notes about this list:

* I only looked at the add-ons hosted on AMO.
* All AMO add-ons from the previous list that are not on this new list have been declared to support all platforms on AMO.
* A couple of add-ons on the list support the 3 major platforms, but are not marked as supporting all possible platforms on AMO. This generally means that the developer is either being very cautious about support, or the add-on has binaries that wouldn't work on other platforms where Firefox runs (BSD, OS/2, etc.).
(In reply to Jorge Villalobos [:jorgev] from comment #22)
> Created attachment 591323 [details]
> List of top add-ons with explicit platform restrictions
> 
> Here's the list of platforms that are specified for the top AMO add-ons on
> the previous list. A few notes about this list:
> 
> * I only looked at the add-ons hosted on AMO.
> * All AMO add-ons from the previous list that are not on this new list have
> been declared to support all platforms on AMO.
> * A couple of add-ons on the list support the 3 major platforms, but are not
> marked as supporting all possible platforms on AMO. This generally means
> that the developer is either being very cautious about support, or the
> add-on has binaries that wouldn't work on other platforms where Firefox runs
> (BSD, OS/2, etc.).

Note that as Jorge and I discussed on irc, we don't have compatibility information for non-AMO addons, and I don't know of any good programmatic way to get them (there is an installPlatform field in the install manifest, but nobody uses that). It is normally a safe assumption to assume that these addons use windows, so let's just do that.

I'll try to update the JSON data with this platform info in a few hours (have an appt I have to go to now).
Depends on: 721055
This adds a bit of info re: platform and amo id information to the addons json, should be useful for running this automation on platforms other than windows.
Attachment #590222 - Attachment is obsolete: true
So while executing an example test-run with the list of add-ons I currently have, I have seen that we shouldn't run our functional tests when add-ons are installed. Some of those modify basic behavior which are causing test failures. But if we run the functional tests we would get at least feedback about total breakage. 

Clint, what would you say? Shall we execute the functional tests?
Attachment #590206 - Attachment is obsolete: true
Attachment #589876 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — — Splinter Review 
New version of the patch which works now as specified. It has not been updated yet to support the exclusion of some platforms as given in the new JSON file William has uploaded. Therefore we should translate the given platform names to the ones Mozmill supports. William, can you please add this to your script?

Otherwise when you want to execute a test-run you can do it that way:

./testrun_compat_addons.py --addons-staging-path=staging --report=http://mozmill-addons.blargon7.com/db/ --tag=addon_compat_fx9.0b6 _addons.json firefox-9.0b6.en-US.mac.dmg

Keep in mind that it only works from beta->beta build right now. There is a problem with the update snippet when we want to test release->beta. I have made a comment in the patch and we have to sort this out.

Also the test will not work yet for 3.6.x->10.0, because of bug 630388. I'm working on it right now and hope to have something ready before I leave today.
Attachment #591236 - Attachment is obsolete: true
Attachment #591783 - Flags: feedback?(wlachance)
Comment on attachment 591783 [details] [diff] [review]
Patch v1

I have almost zero experience with mozmill or this type of integration testing, so in that sense I'm probably not the best person to review this. That said, the logic here looks good. My only comment is that the requirements call for us doing one initial test with only the default set of addons installed -- it seems that currently we're always doing tests with some addons from the extended set.
Attachment #591783 - Flags: feedback?(wlachance) → feedback+
(In reply to William Lachance (:wlach) from comment #27)
> That said, the logic here looks good. My only comment is that the
> requirements call for us doing one initial test with only the default set of
> addons installed -- it seems that currently we're always doing tests with
> some addons from the extended set.

Where has that been mentioned? I can't find the reference, also not on the wiki page.
No longer depends on: 630388
Depends on: 630388
Example test-run with Firefox 3.6.25 as source:

endurance: http://mozmill-addons.blargon7.com/#/endurance/report/cdc03c5373eebee872390db386042ebb
update: http://mozmill-addons.blargon7.com/#/update/report/cdc03c5373eebee872390db386043c1f
endurance: http://mozmill-addons.blargon7.com/#/endurance/report/cdc03c5373eebee872390db386045e79

Waiting for the clarification how to run the first test. Then I will update the script so we will have the final version tomorrow.
(In reply to Henrik Skupin (:whimboo) from comment #29)
> (In reply to William Lachance (:wlach) from comment #27)
> > That said, the logic here looks good. My only comment is that the
> > requirements call for us doing one initial test with only the default set of
> > addons installed -- it seems that currently we're always doing tests with
> > some addons from the extended set.
> 
> Where has that been mentioned? I can't find the reference, also not on the
> wiki page.

You're right, it's not on the wiki page, but here's what I wrote in an e-mail a while ago (which both you and ctalbert agreed to at the time):

* The test will run a basic set of endurance and update tests for the top 10 addons initially.
* Then, for groups of 5 in the list of top 100, install this group of 5 addons in the browser PLUS the top 10, then run the endurance and update tests again.

So I think the wiki page was slightly wrong here. I've updated it here:

https://wiki.mozilla.org/Auto-tools/Projects/Compatibile_By_Default_Automation#Deliverable

Really, my understanding is that initially we really just want to see if making addons compatible by default will break anything, so I think the distinction is probably not that important except from the point of view of trying to narrow down breakage (which presumably we'd do anyway if we noticed something was wrong). Still, I hope this clarifies things.
Attached patch Patch v2 (obsolete) — — Splinter Review 
Updated patch with the following improvements:

* Added check for platform and skip those add-ons with no support for it
* Improvements for add-on staging process
* First iteration only uses specified default add-ons
* Fixed a bug with installing/uninstalling the build which happened outside the loop

William, it would be great if you could check that the patch works fine on Linux and/or Windows. If it works I will ask for review later.
Attachment #591783 - Attachment is obsolete: true
Attachment #592732 - Flags: feedback?(wlachance)
(In reply to Henrik Skupin (:whimboo) from comment #32)
> Created attachment 592732 [details] [diff] [review]
> Patch v2
> 
> Updated patch with the following improvements:
> 
> * Added check for platform and skip those add-ons with no support for it
> * Improvements for add-on staging process
> * First iteration only uses specified default add-ons
> * Fixed a bug with installing/uninstalling the build which happened outside
> the loop
> 
> William, it would be great if you could check that the patch works fine on
> Linux and/or Windows. If it works I will ask for review later.

Tried running it on Windows. Didn't know how to get results, but it seemed to start a bunch of instances of Firefox beta with various addons installed:

*** Updating to branch 'mozilla-beta'
pulling from http://hg.mozilla.org/qa/mozmill-tests
searching for changes
no changes found
29 files updated, 0 files merged, 2 files removed, 0 files unresolved
could not delete 'c:\users\wlach\appdata\local\temp\tmpmrfj6zprofile\extensions\
{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}\chrome\adblockplus.jar': Access is denied

Exception ValueError: "'#MozRunner Prefs Start' is not in list" in <bound method
 FirefoxProfile.cleanup of <mozrunner.FirefoxProfile object at 0x032AEAF0>> igno
red
*** Removing repository 'c:\users\wlach\appdata\local\temp\tmpm1xrot.mozmill-tes
ts'
*** Uninstalling c:\users\wlach\appdata\local\temp\tmpskvenc.binary\

After all the tests were done, all the instances of Firefox were still running, presumably because of the above errors. I'm also not sure how to get the results of the tests if I'm not uploading to a central dashboard.
I also attempted to run this on Linux via xvfb-run. Got a bunch of errors while doing so:

Timeout: bridge.set("2011f388-4b7e-11e1-87e3-782bcbaf0991", Components.utils.import('resource://mozmill/modules/mozmill.js'))

'MozmillRestartWrapper' object has no attribute 'current_test'

And also:

firefox: Fatal IO error 11 (Resource temporarily unavailable) on X server :99.

(I'll attach a larger, though not entirely complete, log to this message)

I guess the bottom line is that things seem to sort of work, though there seem to be either some small problems in the script or my environment which prevent me from saying that for certain.
(In reply to William Lachance (:wlach) from comment #33)
> After all the tests were done, all the instances of Firefox were still
> running, presumably because of the above errors. I'm also not sure how to

I have absolutely no idea why you are getting those errors. Can you please tell me how you have executed those tests?

> get the results of the tests if I'm not uploading to a central dashboard.

You have seen my comment 26 which has an example of the calling convention for this script?

(In reply to William Lachance (:wlach) from comment #34)
> I also attempted to run this on Linux via xvfb-run. Got a bunch of errors
> while doing so:

It's not something we have ever tested or officially support with Mozmill 1.5.x. So please use a vanilla run.
 
> I guess the bottom line is that things seem to sort of work, though there
> seem to be either some small problems in the script or my environment which
> prevent me from saying that for certain.

If you want to attach a log please use the --logfile option to save off any log message to a file. The example attached isn't that helpful because I can't see the earliest issue?

In anyway I will now test the script myself on Windows and Linux.
William, we should probably move the Yahoo Toolbar out of the default addons because it opens a dialog each time Firefox gets restarted. This could cause problems with the focus.
Attachment #591792 - Attachment mime type: application/json → text/plain
Comment on attachment 591792 [details]
List of addons in JSON format with mozmill names for platform info

>  { "name": "Microsoft .NET Framework Assistant", "url": "ftp://ftp.mozilla.org/pub/mozilla.org/addons/9449/microsoft_net_framework_assistant-1.3.1-fx-windows.xpi", "amo_id": 9449, "platforms": [ "winnt" ] },

Today I hit a lot of problems when connecting to the FTP server due to no more users were allowed. Downloads will fail in those cases. So we have to not download the XPI's from the FTP but from 'http://stage.mozilla.org/' which mirrors the FTP via the HTTP protocol.
(In reply to Henrik Skupin (:whimboo) from comment #36)
> (In reply to William Lachance (:wlach) from comment #33)
> > After all the tests were done, all the instances of Firefox were still
> > running, presumably because of the above errors. I'm also not sure how to
> 
> I have absolutely no idea why you are getting those errors. Can you please
> tell me how you have executed those tests?

Using the command line you mentioned above and the mozmill-env python libraries that you linked to (the python inside there didn't work for me, so I fell back to using the python included with the mozilla-build package).

I guess we could troubleshoot here, but I wonder if it would really be worthwhile if you've got an environment to test on yourself.
(In reply to William Lachance (:wlach) from comment #33)
> Tried running it on Windows. Didn't know how to get results, but it seemed
> to start a bunch of instances of Firefox beta with various addons installed:

This is a Mozmill framework issue. Nothing in our scripts. I haven't seen such a behavior so far. Clint can you please take care of bug 722707?
Attached patch Patch v2.1 (obsolete) — — Splinter Review 
This patch makes the staging of add-ons safer by bailing out early enough if a failure happens.

I kinda would like to get feedback and a review on it. Given the aforementioned issues you will only be able to run it on OS X for now.
Attachment #592732 - Attachment is obsolete: true
Attachment #592732 - Flags: feedback?(wlachance)
Attachment #593109 - Flags: review?(gmealer)
Attachment #593109 - Flags: feedback?(wlachance)
Comment on attachment 593109 [details] [diff] [review]
Patch v2.1

Code looks fine. You have some stuff I'd like to have seen commented further for maintenance. 

One is adding docstring comments to all the functions, particularly explaining return types. That's especially true with the ones that return tuples. It's not totally obvious reading the main() function what's going on with the index.

The other is in some of the more complex text or numeric calculations like:

download_addon():

+        filename = url.split('?')[0].rstrip('/').rsplit('/', 1)[-1]
+        target_path = os.path.join(target_path, filename)
+        if not os.path.exists(target_path):

build_addon_list():

+    # Take the next max_count add-ons from the list
+    count = 0
+    while count < max_count and (count + index) < len(addons['extended']):

In the former, the url building is a little complex (as is the later logic that tries to grab it as an OS file then does URL only if that fails). In the latter, you don't document that you're limiting by length of the list.

All minor stuff though. Like I said, code looks great. I just prefer a slightly higher level of detail in comments when I maintain the code so I don't have to read the code as closely.
Comment on attachment 593109 [details] [diff] [review]
Patch v2.1

Oops, I meant to r+ this but forgot to actually set it.
Attachment #593109 - Flags: review?(gmealer) → review+
Comment on attachment 593109 [details] [diff] [review]
Patch v2.1

Somehow missed this, sorry.

This looks fine. There were some minor errors I saw on my console when running on Windows about not being able to delete directories, might be good to look into those (maybe they were related to the bug that was fixed, I dunno). Aside from that, I only have some very minor nits, which are more a matter of personal preference than anything else (changing them is optional):

+        filename = url.split('?')[0].rstrip('/').rsplit('/', 1)[-1]

I'd personally use urlparse here, seems a little less magical:

filename = os.path.basename(urlparse.urlsplit(url).path)

+        # Remove not available add-ons
+        for addon in removable_addons:
+            addons[type].remove(addon)

This is a bit more concise:

addons[type] = [addon in addons[type] if addon not in removable_addons]
Attachment #593109 - Flags: feedback?(wlachance) → feedback+
Ok, we have all four OS's running this code now. mac os x, winxp, win7, and linux. Results should be arriving here: http://mozmill-release.blargon7.com/#/addons/reports
(In reply to Clint Talbert ( :ctalbert ) from comment #46)
> Ok, we have all four OS's running this code now. mac os x, winxp, win7, and
> linux. Results should be arriving here:
> http://mozmill-release.blargon7.com/#/addons/reports

You are probably confused by the 'addons' part. It's not mozmill-release we are using for the DTC tests but mozmill-addons. Means our results can be found here:

Endurance tests: http://mozmill-addons.blargon7.com/
Update tests: http://mozmill-addons.blargon7.com/#/update/reports

Beside that I will work on updating the patch to cover the above mentioned feedback. Thanks for that!
(In reply to Clint Talbert ( :ctalbert ) from comment #46)
> Ok, we have all four OS's running this code now. mac os x, winxp, win7, and
> linux. Results should be arriving here:
> http://mozmill-release.blargon7.com/#/addons/reports

Clint, I can't find any results for Linux. Have you missed to specify report option?
(In reply to Henrik Skupin (:whimboo) from comment #48)
> (In reply to Clint Talbert ( :ctalbert ) from comment #46)
> > Ok, we have all four OS's running this code now. mac os x, winxp, win7, and
> > linux. Results should be arriving here:
> > http://mozmill-release.blargon7.com/#/addons/reports
> 
> Clint, I can't find any results for Linux. Have you missed to specify report
> option?

I see linux tests: http://mozmill-release.blargon7.com/#/functional/top?branch=All&platform=Linux&from=2012-01-30&to=2012-02-06
(In reply to Clint Talbert ( :ctalbert ) from comment #49)
> I see linux tests:
> http://mozmill-release.blargon7.com/#/functional/
> top?branch=All&platform=Linux&from=2012-01-30&to=2012-02-06

This is not the right dashboard. You want to look at mozmill-addons:

http://mozmill-addons.blargon7.com/#/endurance/reports?branch=All&platform=Linux

If mozmill-report is used by your command line please change it ASAP. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #50)
> (In reply to Clint Talbert ( :ctalbert ) from comment #49)
> > I see linux tests:
> > http://mozmill-release.blargon7.com/#/functional/
> > top?branch=All&platform=Linux&from=2012-01-30&to=2012-02-06
> 
> This is not the right dashboard. You want to look at mozmill-addons:
> 
> http://mozmill-addons.blargon7.com/#/endurance/
> reports?branch=All&platform=Linux
> 
> If mozmill-report is used by your command line please change it ASAP. Thanks.
Oops sorry. The systems are using mozmill-addons db, I just mistyped here. It looked like the system blew up while downloading addons, and didn't finish.  I restarted it and now it's running.
(In reply to Geo Mealer [:geo] from comment #43)
> One is adding docstring comments to all the functions, particularly
> explaining return types. That's especially true with the ones that return
> tuples. It's not totally obvious reading the main() function what's going on
> with the index.

Added full doc strings across the file. Left it out for the hopefully final version.

> download_addon():
> 
> +        filename = url.split('?')[0].rstrip('/').rsplit('/', 1)[-1]
> +        target_path = os.path.join(target_path, filename)
> +        if not os.path.exists(target_path):
> 
> build_addon_list():
> 
> +    # Take the next max_count add-ons from the list
> +    count = 0
> +    while count < max_count and (count + index) < len(addons['extended']):

Both don't exist anymore and have been replaced with simpler code.

(In reply to William Lachance (:wlach) from comment #45)
> filename = os.path.basename(urlparse.urlsplit(url).path)

urlsplit looks fine. So I'm using that one now. Other then that os.path.basename will not work because it probably will fail on Windows with backslashes as separator. URLs always have slashes.

> +        # Remove not available add-ons
> +        for addon in removable_addons:
> +            addons[type].remove(addon)
> 
> This is a bit more concise:
> 
> addons[type] = [addon in addons[type] if addon not in removable_addons]

This is way harder to read and also causes a lot more entries to be shuffled around. I don't intend to create a new list but simply want to remove entries we don't need. That's much faster.
Attached patch Patch v3.1 (obsolete) — — Splinter Review 
Updated version of the patch. It will not be the final one because I still want to add some more bits so we can run the tests without having to setup something on our side.

Changes made:
* Complete refactor and splitting code into classes/modules
* Removed some of the strange looking methods
* Added docstring comments for the methods (do we need even more?)
* Removed nearly all CLI options and made those part of the config file
* Bit more of clean-up

Todo:
* Add a setting to the config file which specifies the source builds to test. With it we can automatically download the build and run the tests.
Attachment #593109 - Attachment is obsolete: true
Actually to make it fully automated without the hassle to specify the current platform bug 736098 has to be implemented first.
Depends on: 736098
Attached patch Patch v3.2 (obsolete) — — Splinter Review 
Finalized patch for DTC testing. It now contains:

* Added an example config file
* Source builds to test are now specified in the config file
* Given builds will be downloaded automatically based on the platform under test

Not sure yet if we should always put the staging folder under tmp or not. Any recommendations from your side?
Attachment #606174 - Attachment is obsolete: true
Attachment #606273 - Flags: review?(ctalbert)
Attachment #606273 - Flags: feedback?(gmealer)
Attached patch Patch v3.3 — — Splinter Review 
I made some more tweaks:

* Better handling of testrun options through the config
* If a testrun reports failure those are now collected and a final exception is thrown to get an exit code of 1
Attachment #592858 - Attachment is obsolete: true
Attachment #606273 - Attachment is obsolete: true
Attachment #606273 - Flags: review?(ctalbert)
Attachment #606273 - Flags: feedback?(gmealer)
Attachment #606497 - Flags: review?(ctalbert)
Attachment #606497 - Flags: feedback?(gmealer)
Comment on attachment 606497 [details] [diff] [review]
Patch v3.3

Clean -- I like the refactoring. Thanks for the extra care given to docstrings/comments. Looks great to me.
Attachment #606497 - Flags: feedback?(gmealer) → feedback+
Comment on attachment 606497 [details] [diff] [review]
Patch v3.3

looks good. r+
Attachment #606497 - Flags: review?(ctalbert) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/5124a6be40a6

Any other improvements should be filed as follow-up bugs. I close this one now. Yay!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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: