Closed Bug 688300 Opened 13 years ago Closed 12 years ago

mochitest runtests.py --install-extension is totally broken

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
mochitest runtests.py --install-extension is totally broken. Apparently when we removed the ability for xpis to be auto-installed by living in the profile directory no one updated automation.py to fix it. Patch attached fixes xpi files and directories.
Attachment #561589 - Flags: review?(ted.mielczarek)
Comment on attachment 561589 [details] [diff] [review]
Patch, v1

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

This looks mostly correct, just a few things need to be changed.

::: build/automation.py.in
@@ +950,5 @@
> +            self.log.info("INFO | automation.py | Cannot install extension, bad files in xpi")
> +            return
> +
> +          # We may need to dig the extensionID out of the zip file...
> +          if extensionID is None and filename == "install.rdf":

Shouldn't you put the "extensionID is None" check outside of the whole block that reads the zip file? No reason to grovel through the zip file if you don't need the extension ID, right? Or do you really want to run that absolute path check?

@@ +954,5 @@
> +          if extensionID is None and filename == "install.rdf":
> +            # Need a temp path to extract install.rdf to.
> +            installRDFDir = tempfile.mkdtemp()
> +            try:
> +              extensionZipFile.extract(zipInfo, installRDFDir)

ZipFile.extract was added in Python 2.6, which we don't yet require:
http://docs.python.org/library/zipfile.html#zipfile.ZipFile.extract

Besides that, I think you should just use ZipFile.read instead, and pass the result to xml.dom.minidom.parseString.
http://docs.python.org/library/zipfile.html#zipfile.ZipFile.read

@@ +964,5 @@
> +
> +                # Find the <em:id> element.
> +                rdfElement = installRDFDoc.getElementsByTagName("RDF")[0]
> +                descriptionElement = rdfElement.getElementsByTagName("Description")[0]
> +                idElement = descriptionElement.getElementsByTagName("em:id")[0]

Presumably you could be less pedantic here, and just 'installRDFDoc.getElementsByTagName("em:id")[0]' ?

@@ +985,5 @@
> +          shutil.rmtree(thisExtensionDir, True)
> +        os.mkdir(thisExtensionDir)
> +
> +        # Extract all files.
> +        extensionZipFile.extractall(thisExtensionDir)

extractall is also 2.6-only. :-/
http://docs.python.org/library/zipfile.html#zipfile.ZipFile.extractall

I guess you'll have to roll your own here, or just subprocess.check_call(["unzip", extensionSource], cwd=thisExtensionDir)

::: testing/mochitest/runtests.py
@@ +852,5 @@
> +
> +      if os.path.isdir(extensionPath):
> +        extensionID = os.path.split(extensionPath)[1]
> +      else:
> +        extensionID = None

This is a little ugly. Can we just push this down into installExtension instead? Also, maybe your code should just parse the install.rdf file (since you already have that code) instead of expecting that the directory name is correct?
Attachment #561589 - Flags: review?(ted.mielczarek) → review-
Assignee: nobody → bent.mozilla
Attached patch Patch, v2 (obsolete) — Splinter Review
Ok, this fixes your comments and works with only the methods available in python 2.5 (though it uses newer methods on later versions).
Attachment #561589 - Attachment is obsolete: true
Attachment #563076 - Flags: review?(ted.mielczarek)
(In reply to Ted Mielczarek [:ted, :luser] from comment #1)
> Comment on attachment 561589 [details] [diff] [review] [diff] [details] [review]
> ...
> Or do you really want to run that absolute path check?

Yeah... I just don't like the possibility that we could stick files outside the extension directory if the build system makes a bad xpi or something.

> ZipFile.extract was added in Python 2.6, which we don't yet require:

Fixed.

> Besides that, I think you should just use ZipFile.read instead, and pass the
> result to xml.dom.minidom.parseString.
Done.

> Presumably you could be less pedantic here, and just
> 'installRDFDoc.getElementsByTagName("em:id")[0]' ?

No... That does a deep search. I fixed this to be more correct since my version had a bug too.

> extractall is also 2.6-only. :-/

Fixed.

> This is a little ugly. Can we just push this down into installExtension
> instead? Also, maybe your code should just parse the install.rdf file (since
> you already have that code) instead of expecting that the directory name is
> correct?

Yeah, done.
Comment on attachment 563076 [details] [diff] [review]
Patch, v2

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

Better, but still a few things I'm not quite happy with.

::: build/automation.py.in
@@ +99,5 @@
> +class ZipFileReader(object):
> +  "Class to read zip files in Python 2.5 and later. Limited to only what we actually use."
> +
> +  def __init__(self, filename):
> +    from zipfile import ZipFile

Just leave this import up at the top level. I'm not a fan of hiding imports unless they're platform-specific.

@@ +100,5 @@
> +  "Class to read zip files in Python 2.5 and later. Limited to only what we actually use."
> +
> +  def __init__(self, filename):
> +    from zipfile import ZipFile
> +    self.__zipfile = ZipFile(filename, "r")

I never personally use the double underscore prefix for private variables. Doesn't really seem worth it for something that's local to this file. (I don't think I've ever seen it used in any Python library, either.)

@@ +130,5 @@
> +  def read(self, name):
> +    return self.__zipfile.read(name)
> +
> +  def extract(self, name, path = None):
> +    major, minor = sys.version_info[:2]

Yeah, don't do this. Just do:
if hasattr(self.__zipfile, "extract"):
    return self.__zipfile.extract(name, path)

@@ +143,5 @@
> +    self.__extractname(name, self.__getnormalizedpath(path))
> +
> +  def extractall(self, path = None):
> +    major, minor = sys.version_info[:2]
> +    assert major == 2

Same thing here as above.

@@ +150,5 @@
> +      return self.__zipfile.extractall(path)
> +
> +    path = self.__getnormalizedpath(path)
> +
> +    for i, name in enumerate(self.__zipfile.namelist()):

Doesn't look like you're using i here, so you can drop the enumerate.

@@ +1021,5 @@
>      if os.path.isfile(extensionSource):
> +      reader = ZipFileReader(extensionSource)
> +
> +      filenames = reader.namelist()
> +      for filename in filenames:

Skip the temporary:
for filename in reader.namelist():
Attachment #563076 - Flags: review?(ted.mielczarek) → review-
Attached patch Patch, v2.1Splinter Review
Review comments addressed.
Attachment #563076 - Attachment is obsolete: true
Attachment #563504 - Flags: review?(ted.mielczarek)
Comment on attachment 563504 [details] [diff] [review]
Patch, v2.1

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

::: build/automation.py.in
@@ +112,5 @@
> +    path = os.path.normpath(os.path.expanduser(path))
> +    assert os.path.isdir(path)
> +    return path
> +
> +  def _extractname(self, name, path):

It would be nice to have short docstring comments on some of these methods that aren't just forwarding to ZipFile.

@@ +976,5 @@
>      return status
>  
>    """
> +   Retrieves the extension id from an install.rdf file (or string).
> +  """

These docstrings got goofed up at some point. Can you move this (and the one for installExtension) so that the string is after the def line? That's the Python convention.
Attachment #563504 - Flags: review?(ted.mielczarek) → review+
This landed on mozilla-central long ago.
http://hg.mozilla.org/mozilla-central/rev/870bd2683c5e
I assume closing this bug was just forgotten?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.