Closed Bug 778329 Opened 9 years ago Closed 9 years ago

devicemanagerSUT - invalid argument in unpackFile

Categories

(Testing :: Mozbase, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: mihneadb, Assigned: mihneadb)

References

Details

Attachments

(1 file, 2 obsolete files)

rumCmds takes a list for dicts, here we are only passing a list of strings.
Attached patch fix the bug (obsolete) — Splinter Review
369 is the line in cause.
Rest is just whitespace fixes.
Assignee: nobody → mbalaur
Status: NEW → ASSIGNED
Attachment #646754 - Flags: review?(mcote)
My bad, line 1107 in the diff.
Blocks: 778366
Comment on attachment 646754 [details] [diff] [review]
fix the bug

Giving this a r- because the real problem here is that there are two versions of unpackFile(), for some reason, and the second one is the broken one, which overrides the first (working) one. Please just delete the second one.

However while you are at it, the 'cd' in that command doesn't make any sense. The agent's unzp command takes an optional destination directory, and it makes a lot more sense to use that than to change the working directory.

So please make those changes, and ensure that there are appropriate tests for the command.
Attachment #646754 - Flags: review?(mcote) → review-
Attached patch Add dest_dir param (obsolete) — Splinter Review
Also created a test, adding it to bug #778366
Attachment #646754 - Attachment is obsolete: true
Attachment #647416 - Flags: review?(mcote)
Comment on attachment 647416 [details] [diff] [review]
Add dest_dir param

Looks good, thanks! I think some of the whitespace edits are unnecessary--it's okay to indent blank lines so long as they line up with the next line--but it's okay.

I only have one little nit:

>-    dir = ''
>-    parts = filename.split('/')
>-    if (len(parts) > 1):
>-      if self.fileExists(filename):
>-        dir = '/'.join(parts[:-1])
>-    elif self.fileExists('/' + filename):
>-      dir = '/' + filename
>-    elif self.fileExists(devroot + '/' + filename):
>-      dir = devroot + '/' + filename
>-    else:
>-      return None
>+    # if no dest_dir is passed in just set it to file_path's folder
>+    if not dest_dir:
>+      parts = file_path.split('/')
>+      dest_dir = '/'.join(parts[:-1])
>+
>+    if dest_dir[-1] != '/':
>+      dest_dir += '/'

Firstly it doesn't appear necessary to add a trailing slash to dest_dir. I just tried it without one, and it worked fine.

Secondly, you can simplify the dest_dir assignment to just

  dest_dir = posixpath.dirname(file_path)

Don't forget to import posixpath. We should be using this more in DeviceManager, since it's a lot clearer than splitting paths into parts and recombining them.

And finally, it's probably not a bad idea to submit this to try, just to be absolutely sure it doesn't bust anything.
Attachment #647416 - Flags: review?(mcote) → review+
If you could just fix that one nit and submit another patch, I will push it to try to make sure it doesn't break anything, then we'll commit it. We are trying to be extra careful with things like DeviceManager. :)
I kept the check for the ending '/' because for me it doesn't work. If I don't do that check it will create "infratestpush2" instead of "infratest/push2".
Attachment #647416 - Attachment is obsolete: true
Attachment #647621 - Flags: review?(mcote)
This should be landed in the mozbase repo as well
Yup, figured I'd wait until it landed in m-c first.
(In reply to Mark Côté ( :mcote ) from comment #11)
> Yup, figured I'd wait until it landed in m-c first.

Sure, just figured I'd note for posterity
https://hg.mozilla.org/mozilla-central/rev/4f5a56859501
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment on attachment 647621 [details] [diff] [review]
updated according to feedback

Just realized I never officially reviewed the last patch. r+ :)
Attachment #647621 - Flags: review?(mcote) → review+
You need to log in before you can comment on or make changes to this bug.