705864 - [mozprocess] mozprocess tests should use mozprocess.pid

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/mozprocess1.py
and
https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/mozprocess2.py
grep through processes to find the executable, e.g.:

https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/mozprocess1.py

Instead, it should use
https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py
for this lookup.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=705220

Comment 1

[Jarek Śmiejczak [:jotes]]

Looks like in mean time mozprocess had a little test refacorings and mozprocess1/2 where splitted into many files.
From what my little investigation, we need to modify only one function here:
https://github.com/mozilla/mozbase/blob/master/mozprocess/tests/proctest.py#L9

Comment 2

[Jarek Śmiejczak [:jotes]]

:jhammel
I would like to take this bug but i have two question:
- check_for_process returns output of process but it's documented in strange way. It should return output for processName (from the way i understand description) but instead of this it returns output of grep/tasklist command.
- I would like to use get_pids from pid.py but it doesn't provide output. Should I remove all connections to output from check_for_process or extends get_pids/pid.py in some way?

Thanks in advance for the guidance!

Comment 3

[Jeff Hammel]

(In reply to Jarek Śmiejczak from comment #2)
> :jhammel
> I would like to take this bug but i have two question:
> - check_for_process returns output of process but it's documented in strange
> way. It should return output for processName (from the way i understand
> description) but instead of this it returns output of grep/tasklist command.

See https://bugzilla.mozilla.org/show_bug.cgi?id=923770#c10 or more specifically:

"""
::: talos/utils.py
@@ +123,3 @@
>  def is_running(pid, psarg='axwww'):
>    """returns if a pid is running"""
> +  return bool([i for i in mozpid.ps(psarg) if pid == int(i['PID'])])

So we should be careful that 1. the resulting consolidated code is best and more generic than the two different parts and; 2.(IMHO) that the resultant code should live in mozbase (specifically, e.g. mozbase.pid).  The code is close but doesn't entirely match:

``talos.utils``

http://hg.mozilla.org/build/talos/file/ed35a6799e25/talos/utils.py#l121
def _parse_ps():
    """parse the output of the ps command"""
    # -> where are the tests?

def ps(arg='axwww'):
   """
   python front-end to `ps`
   http://en.wikipedia.org/wiki/Ps_%28Unix%29
   """

def is_running(pid, psarg='axwww'):
  """returns if a pid is running"""

def running_processes(name, psarg='axwww', defunct=False):
  """
  returns a list of 2-tuples of running processes:
  (pid, ['path/to/executable', 'args', '...'])
  with the executable named `name`.
  - defunct: whether to return defunct processes
  """
----

``mozprocess.pid``

# https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py#L21
def ps(arg=psarg):
    """
    python front-end to `ps`
    http://en.wikipedia.org/wiki/Ps_%28Unix%29
    returns a list of process dicts based on the `ps` header
    """

# https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py#L42
def running_processes(name, psarg=psarg, defunct=True):
    """
    returns a list of
    {'PID': PID of process (int)
     'command': command line of process (list)}
     with the executable named `name`.
     - defunct: whether to return defunct processes
    """

# https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/pid.py#L73
def get_pids(name):
    """Get all the pids matching name"""

This doesn't have to be done here, but should be done as a follow up.
"""

We should work on consolidating towards a common end.

> - I would like to use get_pids from pid.py but it doesn't provide output.
> Should I remove all connections to output from check_for_process or extends
> get_pids/pid.py in some way?

Yes likely; as guided by above, whichever way works the most smoothly.

> Thanks in advance for the guidance!

Comment 4

[David Burns :automatedtester]

ahal added you as mentor, if you can't please update to someone better suited

Comment 5

[Daniel Y Lee]

Longer overdue? I would like to give this a try

Comment 6

[Andrew Halberstadt [:ahal]]

Hi Daniel, thanks for the interest. In case you hadn't noticed, mozbase has moved repositories since this was last updated. Mozprocess now lives in mozilla-central here:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial in case you need help setting that up. Note that you don't need to build anything for this bug.

Once you have the source checked out, take a look at this function:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/tests/proctest.py#9

This bug is about getting rid of that tasklist/ps/grep hack and instead using the process' pid directly. At this point, if you need further guidance feel free to ask me either here, or on irc.mozilla.org in #ateam.

Thanks!

Comment 7

[Daniel Y Lee]

Hi Andrew,

Thanks for your help!

I've read through Jeff's comment, and it is interestingly confusing! Jarek's questions are the same questions that I have, and I'm having difficulty understanding Jeff's response.. comment #3

> (In reply to Jarek Śmiejczak from comment #2)
> > :jhammel
> > I would like to take this bug but i have two question:
> > - check_for_process returns output of process but it's documented in strange
> > way. It should return output for processName (from the way i understand
> > description) but instead of this it returns output of grep/tasklist command.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=923770#c10 or more
> specifically:
> ...
> 
> We should work on consolidating towards a common end.

Hmm. So what about the output? Do we want the stdout of a process we are interested in? If so, the current implementation doesn't do anything to accomplish this feature, and nothing in pid.py is designed to do this. I've googled around and seems like this is not trivial task (http://stackoverflow.com/questions/249703/how-can-a-process-intercept-stdout-and-stderr-of-another-process-on-linux). 

> > - I would like to use get_pids from pid.py but it doesn't provide output.
> > Should I remove all connections to output from check_for_process or extends
> > get_pids/pid.py in some way?
> 
> Yes likely; as guided by above, whichever way works the most smoothly.
> 
> > Thanks in advance for the guidance!

I'm wondering what "Yes likely" is referring to - (1) remove all connection to output from check_for_process OR (2)extend pid.py in some way. Again, (2) seems rather complicated.

Finally, does this ticket include a task to "consolidating towards a common end" between pid.py and talos/utils.py? I can definitely try to do this, but I'm not really clear whether this ticket is asking for that.

Sorry about the long line of questions :(.

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to Daniel Y Lee from comment #7)
> I've read through Jeff's comment, and it is interestingly confusing! Jarek's
> questions are the same questions that I have, and I'm having difficulty
> understanding Jeff's response.. comment #3

I think a fair amount has changed since that conversation, I wouldn't worry too much about understanding everything there :).


> Hmm. So what about the output? Do we want the stdout of a process we are
> interested in? If so, the current implementation doesn't do anything to
> accomplish this feature, and nothing in pid.py is designed to do this. I've
> googled around and seems like this is not trivial task
> (http://stackoverflow.com/questions/249703/how-can-a-process-intercept-
> stdout-and-stderr-of-another-process-on-linux).

It seems the comments are very misleading, the output returned from 'check_for_process()' is actually the output of 'grep' and not the process' output itself. As far as I can tell, it is only used for printing in the case an assertion fails in 'determine_status()'. So whatever we do can just return a list of the result instead of the grep call.


> I'm wondering what "Yes likely" is referring to - (1) remove all connection
> to output from check_for_process OR (2)extend pid.py in some way. Again, (2)
> seems rather complicated.

I'd say just return the list returned from get_pids. In fact, there isn't really much point in the 'check_for_process' method at all anymore.. it might be best to just refactor the tests to use 'get_pids' instead of 'check_for_process'. If we do it this way, an empty list would indicate detected=False.


> Finally, does this ticket include a task to "consolidating towards a common
> end" between pid.py and talos/utils.py? I can definitely try to do this, but
> I'm not really clear whether this ticket is asking for that.

No let's not worry about Talos :).

Finally, you may notice that a lot of this code is duplicated in test_mozprocess.py.. I think that accidentally got left behind and needs to be deleted, so feel free to ignore it.

Comment 9

[Daniel Y Lee]

Thank you for the clarifications!

I'll work on a patch for it.

Comment 10

[Daniel Y Lee]

Hi,

I'm slightly stuck at a problem and seeking help! 

While I was trying a very simple fix of using pid.py's running_processes(...), I stumbled upon an issue with running_process's logic.

Here is a basic breakdown:

1) running_processes calls ps. ps returns a list of summary of process running on the system
2) running_processes extracts string associated with 'COMMAND' key from each of the processes. 
3) running_processes splits the 'COMMAND' string by whitespace
4) running_processes ASSUMES THAT THE FIRST WORD of the splitted string is the name of the process.

(4) is flawed. For instance, let's say we are running a python process. Then 'COMMAND' looks something like this.

>> python proclaunch.py

Then, it assumes 'python' is the name of the process, while our proctests.py is designed to look for a process <proclaunch.py>.

Is this clear?

I can modify the code in running_processes to fix this behavior, but I wonder if this is desired. If so, should we open another ticket for that? (and assign it to me :) )

Comment 11

[Andrew Halberstadt [:ahal]]

Hmm, good catch, that will be a problem!

It looks like that's currently the desired behaviour (see the comment):
http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprocess/mozprocess/pid.py#47

But this will make it very difficult to use with the unit tests. Maybe we could change the behaviour so it returns a list of processes that have 'name' as a substring of the command (instead of the executable) then that would be ok.

Feel free to do that as part of this bug. If you feel like filing a new bug for it, that's ok too :). Thanks!

Comment 12

[Daniel Y Lee]

Hi Andrew,

Thanks for the suggestion - I will go ahead and implement the "substring" method in the pid. But I do think this should be filed as a separate bug, if anything to leave a more tangible concrete of the change in pid.py.

I filed a bug and would like to assign it to myself. Could you help?


https://bugzilla.mozilla.org/show_bug.cgi?id=1067008

Thanks!

Comment 13

[Daniel Y Lee]

Attachment: 0001-Bug-705864-mozprocess-mozprocess-tests-should-use-mo.patch

Trying out submitting a git patch!

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8491231 [details] [diff] [review]
0001-Bug-705864-mozprocess-mozprocess-tests-should-use-mo.patch

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

Thanks again! This looks good, but there are a couple of things I think we should change before landing it.

First for future reference, try to generate patches with 8 lines of context. You can do this by passing in '-U 8' in git. Further comments inline.

::: mozprocess/__init__.py
@@ +3,4 @@
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from processhandler import *
> +from pid import running_processes

I don't think running_processes is used enough to expose it at the module level. We can just do:

from mozprocess.pid import running_processes

::: tests/proctest.py
@@ +33,2 @@
>          detected = False
> +        processes = mozprocess.running_processes(os.path.basename(processName))

If you use pid.get_pids() instead, then you should be able to replace the windows implementation as well! Don't worry about the output, I think if it prints a list of pids, that is good enough. Note we'd have to test out this change though, I guess there's a chance it could break some tests.

Comment 15

[Daniel Y Lee]

Hi Andrew,

Thanks for the review - I will make changes as you suggest.

On testing - what can I do to test the changes I made on a Windows/Mac? Is there some sort of Mozilla tool that can help me test on platforms I have no easy access to? (I use linux).

Comment 16

[Andrew Halberstadt [:ahal]]

There is, it's called Try server [1]. Unfortunately it requires level 1 commit access. For now let me know when the patch is ready and I can push it to try for you.

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 17

[Daniel Y Lee]

Attachment: 0001-Bug-705864-mozprocess-mozprocess-tests-should-use-mo.patch

Thanks for your advices Andrew - I removed the changes from __init__.py and made use of get_pids(). Thanks for your pointers - the code is much simpler yet does more than my previous patch.

Passed all the test on my local setup.

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8491790 [details] [diff] [review]
0001-Bug-705864-mozprocess-mozprocess-tests-should-use-mo.patch

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

Nice, thanks! r+ with two minor comments. Please upload one last patch with these comments fixed, but no need to flag me for review again. Once the latest patch is uploaded, I'll push it to try (so we can test it on windows) and if everything looks good, then I'll land it for you.

::: tests/proctest.py
@@ +2,5 @@
>  import os
>  import subprocess
>  import sys
>  import unittest
> +import mozprocess.pid

nit: I would do from mozprocess.pid import get_pids

@@ +20,5 @@
>  
> +    if processes:
> +        return True, processes
> +    else:
> +        return False, ''

get_pids returns a list, so instead of '', let's return [].

Comment 19

[Daniel Y Lee]

Attachment: 0001-Bug-705864-mozprocess-mozprocess-tests-should-use-mo.patch

Thanks Andrew for your review. Made appropriate changes.

Comment 20

[Andrew Halberstadt [:ahal]]

Thanks! I'll push this to try next week.

Comment 21

[Andrew Halberstadt [:ahal]]

Attachment: mozprocess_get_pids

Works for me locally, here's a try run to test it out on Windows and Mac:
https://tbpl.mozilla.org/?tree=Try&rev=262827f81f30

If all goes well I'll land the patch on inbound.

One note, the patch seems to be generated based from the "mozprocess" directory instead of from the root "mozilla-central" directory. This makes it so other people cannot apply the patch. I fixed it up for you, but for future reference you can generate patches with "git format-patch". E.g git format-patch HEAD~1 -o patches

Comment 22

[Daniel Y Lee]

Hi Andrew,

Thank you for spending the time to put in the patch for me.

I've looked at the output on tbpl - seems like the test failed for osx :(.

Wondering what would be a good practice to employ at this stage. I do not have system to replicate the issue that is being observed to help me hone in down the issue! Advices will be greatly appreciated :).

Comment 23

[Andrew Halberstadt [:ahal]]

Hmm, usually when this happens we try to guess what the error is and push our attempted fix to try again. Would you be interested in applying for level 1 commit access? This would let you push to try on your own, but there is some process to go through and you'd need to learn how to use it.

Alternatively, you can try to guess what the problem is and I can continue to push to try on your behalf.

The last option is for you to stop working on this bug and find something else to do instead.

Comment 24

[Daniel Y Lee]

Hi Andrew,

Definitely want to try the first process - i think this is a fantastic time to learn more about how mozilla teams work :).

I've started the commit level application process, and would like to ask you if you can vouch for me? Here is the ticket:

https://bugzilla.mozilla.org/show_bug.cgi?id=1071243

Thanks,
Daniel

Comment 25

[Daniel Y Lee]

https://tbpl.mozilla.org/?tree=Try&rev=322bd89fe495

Comment 26

[Daniel Y Lee]

Hi Andrew,

An update - Consider the following code on mozprocess.pid.py

    1.10  # determine the platform-specific invocation of `ps`
    1.11  if mozinfo.isMac:
    1.12      psarg = '-Acj'
    1.14  elif mozinfo.isLinux:
    1.15      psarg = 'axwww'
    1.16  else:
    1.17      psarg = 'ax'

For whatever reason, we pass "-Acj" args to ps in Mac. Because my test case was only failing in OSX, I tried changing to psarg for Max to 'axwww' and pushed the result to Try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb2492b952ea

That seems to accept my changes for this ticket. Do you consider this another ticket item that I should open up and quickly patch it or part of this ticket?

Comment 27

[Andrew Halberstadt [:ahal]]

That looks reasonable. Though I wonder why 'w' is specified three times.. Might as well leave it I guess.

I think you can fix this as part of this patch since it is minor and very related. Thanks!

Comment 28

[Daniel Y Lee]

Attachment: ticket-705864.patch

Comment 29

[Daniel Y Lee]

Attachment: ticket-705864.patch

Comment 30

[Andrew Halberstadt [:ahal]]

Comment on attachment 8500771 [details] [diff] [review]
ticket-705864.patch

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

Woohoo, thanks for sticking with it! I just have two minor style comments. Please upload a new patch (and don't forget to format it with the proper commit message). If you've already pushed this to try and everything passed, feel free to add the "checkin-needed" keyword.

Thanks again!

::: testing/mozbase/mozprocess/mozprocess/pid.py
@@ +16,4 @@
>  elif mozinfo.isLinux:
>      psarg = 'axwww'
>  else:
>      psarg = 'ax'

I think you should change this whole block to:
if mozinfo.isWin:
    psarg = 'ax'
else:
    psarg = 'axwww'

::: testing/mozbase/mozprocess/tests/proctest.py
@@ +23,1 @@
>      else:

nit: no need for the else here.. see General practices in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 31

[Daniel Y Lee]

Attachment: ticket-705864.patch

Thank you very much for the review :)

Comment 32

[Andrew Halberstadt [:ahal]]

Oh sorry, one last thing. Please include "r=ahal" at the end of your commit messages. For more info, see:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

Thanks!

Comment 33

[Daniel Y Lee]

Attachment: ticket-705864.patch

Got it! thank you :)

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5d23ce1ce9

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac5d23ce1ce9