Closed Bug 609333 Opened 12 years ago Closed 12 years ago

mozrunner uses external ilbraries to parse extension information when it should use xml.dom

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: k0scist, Assigned: k0scist)

References

Details

(Whiteboard: [mozmill-1.5.1+])

Attachments

(2 files)

currently, mozrunner uses elementtree to install addons:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5.1/mozrunner/mozrunner/__init__.py#L200

This causes complications when supporting multiple versions:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5.1/mozrunner/mozrunner/__init__.py#L200

Talos uses borrowed elements of mozrunner for its profile code that does not require external dependencies in any python version > 2:

http://hg.mozilla.org/build/talos/file/ce9b0413fbf1/ffsetup.py#l119

http://docs.python.org/library/xml.dom.minidom.html

Since we support 2.4 and up, we should use this solution
Whiteboard: [mozmill-1.5.1?]
Personally I think we should take care of it in Mozmill 1.5.2. We don't have tests for this area and changing this code could be risky. IMO we are too late in the cycle for such a change. The current solution works and is not a regression.
(In reply to comment #1)
> Personally I think we should take care of it in Mozmill 1.5.2. We don't have
> tests for this area and changing this code could be risky. IMO we are too late
> in the cycle for such a change. The current solution works and is not a
> regression.

This is required for buildbot -- see e.g. http://tools-staging-master.mv.mozilla.com:8010/builders/Rev3%20MacOSX%20Leopard%2010.5.8%20mozilla-central%20debug%20test%20mozmill-all/builds/6/steps/install%20mozmill/logs/stdio

I have no idea why this wasn't spotted before.  I've been told that the windows slaves still use python 2.4, so pip was probably illegally hitting net.  Again, no clue how this ever worked on buildbot + python 2.4
This uses xml.minidom ala talos (http://hg.mozilla.org/build/talos/raw-file/ce9b0413fbf1/ffsetup.py#l119) ; i've restructured addon_id to be its own method, so that it is easier to test.  Currently, I've only tested by hand:  both mozmill and jsbridge work and the current addons i have for FF and thunderbird, but this is a set of about 8 out of millions. So I'm not sure how general purpose this is, but it should be as good as what talos does.  We might want to do more rigorous testing down the line

I'm not married to addon_id being a classmethod, though I'd rather see it be a classmethod, staticmethod, or standalone entirely, as it doesn't need any class data
Assignee: nobody → jhammel
Attachment #487994 - Flags: review?(ctalbert)
Comment on attachment 487994 [details] [diff] [review]
use xml.minidom to find addon ids

Looks good.  This is exactly the code we use on Talos which has downloaded and tested all available addons for talos addon testing, so I'm pretty confident it will work.
R+
Clint
Attachment #487994 - Flags: review?(ctalbert) → review+
Whiteboard: [mozmill-1.5.1?] → [mozmill-1.5.1+]
pushed to hotfix-1.5.1: https://github.com/mozautomation/mozmill/commit/e3faa5d70bf85f15c9af78f004d2219dd1cf2458

needs to go to master and m-c; will update here
this ports the bug to an m-c patch;  also, it corrects installmozmill.py to display python version and exit with the correct code on failure
Attachment #488203 - Flags: review?(ctalbert)
Attachment #488203 - Flags: review?(ctalbert) → review+
Landed no m-c http://hg.mozilla.org/mozilla-central/rev/126e28ef841d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Works great. Marking as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.