775416 - Mozrunner has to use --app-arg options first in the right order before appending default args

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now mozrunner adds the default arguments first when calling the application. That breaks the feature that it can be used for Xulrunner applications because those expect as first argument the location of the application.ini file. If there is another argument like --foreground Xulrunner cannot find details about the application and exits out.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Here the code where all the arguments are getting put into a dict:
https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L345

So not sure if this datatype is the one we really want here. I would say we combine the different kinds of args right before we run the command and stick everything into an array. Jeff, what are your thoughts?

Comment 2

[Jeff Hammel]

So https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L345 are only the arguments to create the runner class.  The actual array is made here:

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L171

with self.cmdargs from 

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L95

So AIUI it is these that need to be fixed.

:whimboo, do you have any examples of command lines passed in to mozrunner where this is being handled wrong?  (Expected vs. actual?)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> :whimboo, do you have any examples of command lines passed in to mozrunner
> where this is being handled wrong?  (Expected vs. actual?)

This affects all XULRunner based applications which expect the "--application %path_to_application.ini%" as the first argument after the application binary. 

That means right now Mozmill cannot be used to test XULRunner based applications, even when we claim that. :/

Comment 4

[moijes]

I apologize for my ignorance, but which are the examples that use XULRunner which can be tested here ?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: example application

Attached you can find the example xulrunner application we could use for getting the behavior right in Mozmill.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I talked with moijes on IRC and he wants to work on this bug.

Comment 7

[moijes]

Taking myself out of this as I've tried but couldn't reach anywhere.

Comment 8

[Johannes Vogel (:nebelhom)]

Attachment: 1st patch attempt

Ok, this is the first attempt.

After speaking with jhammel, I adjusted runner.command to include self.cmdargs

I couldn't find any mozrunner specific tests in the folder (unlike for the other folders), so I just the entire test suite in case I missed it and it came back ok...

Hope it is ok.

Comment 9

[Jeff Hammel]

Sadly, mozrunner does not have tests yet :(

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 752245 [details] [diff] [review]
1st patch attempt

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

Thanks Johannes for this patch. It's great to see that were able to dig into this problem and got it fixed. As you said on IRC you got feedback from Jeff regarding the changes in the command property. So I assume those are fine. From my part you will get r+ with the above mentioned issues fixed. For an updated patch please also request review from Jeff, given that he knows the details of Mozrunner.

::: mozrunner/mozrunner/runner.py
@@ +127,5 @@
>      def command(self):
>          """Returns the command list to run."""
> +        commands = [self.binary, '-profile', self.profile.profile]
> +        # This makes sure that self.cmdargs is usable by the binary
> +        # see https://bugzilla.mozilla.org/show_bug.cgi?id=775416

You can shorten the comment to something like:

Bug 775416 - Ensure that binary options are passed in first

@@ +172,5 @@
>              self.profile.reset()
>              assert self.profile.exists(), "%s : failure to reset profile" % self.__class__.__name__
>  
> +        cmd = self._wrap_command(self.command)
> +        

Please fix the trailing spaces.

Comment 11

[Johannes Vogel (:nebelhom)]

Attachment: 2nd patch attempt

Thanks for the quick review, Henrik.

I fixed the issues that you raised, so it should be fine.

@jhammel: If you still see something wrong with the code, then please let me know. Thanks.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 752380 [details] [diff] [review]
2nd patch attempt

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

::: mozrunner/mozrunner/runner.py
@@ +128,5 @@
>          """Returns the command list to run."""
> +        commands = [self.binary, '-profile', self.profile.profile]
> +        # Bug 775416 - Ensure that binary options are passed in first
> +        commands[1:1] = self.cmdargs
> +        return commands

Two blank lines around the inserted text wouldn't be that bad, IMHO. But lets see what Jeff says.

Comment 13

[Jeff Hammel]

Comment on attachment 752380 [details] [diff] [review]
2nd patch attempt

If this works in practice, then wfm.  I would have just had the one liner

return [self.binary] + self.cmdargs[:] + ['-profile', self.profile.profile]

Comment 14

[Johannes Vogel (:nebelhom)]

:jhammel I have no problems changing it to that line, if you want me to. Just say the words.

I tried to be explicit about this change and... tbh, I didn't think of your solution :P

Comment 15

[Jeff Hammel]

I am fine with either way as long as it works and is tested.  Are there any objections to landing?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tested this and it works fine with different sets of command line options. I would really kinda see this landed. Andrew or William, any feedback as asked by Jeff in comment 15?

Actually, it's sad to see that we do not have tests for mozrunner!

Comment 17

[Andrew Halberstadt [:ahal]]

No objections here.

Comment 18

[William Lachance (:wlach)]

No issues here.

Comment 19

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/9e390f0ec335d0face04ce603df1ab30d74189f8

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jeff, is there a planned release of mozrunner soon? Or shall I file a bug to get the version bumped? We kinda would like to have this feature in mozmill 2.0rc4.

Comment 21

[Jeff Hammel]

There is no release planned at this time, so yes, file a bug if you need this done.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Filed as bug 877306.