779006 - Print process pid in check-interactive

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Paul Dagnelie]

Attachment: proposed patch

I've been running many tests using gdb and check-interactive over the past few weeks, and I got frustrated suspending and foregrounding the process to get the pid so i could attach gdb to it.  I figured other people might like this fix as well.  Sample output is as follows: 

~/ffx/mozilla-central$ make SOLO_FILE="test_resumable_channel.js" -C obj-x86_64-unknown-linux-gnu/netwerk/test check-interactive
TEST-INFO | profile dir is /tmp/firefox/xpcshellprofile
TEST-INFO | /home/pcd/ffx/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_resumable_channel.js | running test ...
TEST-INFO | /home/pcd/ffx/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_resumable_channel.js | Process ID: 17433
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/firefox/xpcshellprofile/runxpcshelltests_leaks.log
To start the test, type |_execute_test();|.
js> 

Thoughts?

Comment 1

[Josh Matthews [:jdm]]

Heh, I just have a number of platform-specific aliases that grep the ps list for the correct pid, cut it from the output and attach gdb. This sounds pleasant.

Comment 2

[Jason Duell]

But the PID gets printed already (at least for MOZ_DEBUG_CHILD_PROCESS) and with a nice message about "I'm asleep: come attach to me".  It's in the output that gets diverted to a file and then shown at program completion.   All you need is a time machine to use it correctly :)

(I've been using a script much like jdm's...)

Comment 3

[(not currently active) Ted Mielczarek]

If you use "check-one" instead of "check-interactive", you can pass EXTRA_TEST_ARGS=--debugger=gdb and everything will work properly.

Like so:
make -C ../opt-mozilla-central/testing/xpcshell/example/ check-one SOLO_FILE=test_execute_soon.js EXTRA_TEST_ARGS=--debugger=gdb

Comment 4

[Paul Dagnelie]

(In reply to Ted Mielczarek [:ted] from comment #3)
> If you use "check-one" instead of "check-interactive", you can pass
> EXTRA_TEST_ARGS=--debugger=gdb and everything will work properly.
> 
> Like so:
> make -C ../opt-mozilla-central/testing/xpcshell/example/ check-one
> SOLO_FILE=test_execute_soon.js EXTRA_TEST_ARGS=--debugger=gdb

Ah, I see!  That is good to know, but I still think this is useful.  After all, the stated purpose of check-interactive to is to attach a debugger to the programs, and that's what most of the documentation scattered around points to.  Given that it looks like i'm not the only one who had to find a workaround for this, and that check-interactive is what most new people are going to find, i think this could be a useful change.

Comment 5

[cmtalbert]

Comment on attachment 647371 [details] [diff] [review]
proposed patch

This looks good to me.

Comment 6

[Mozilla RelEng Bot]

Try run for 100064210a1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=100064210a1a
Results (out of 292 total builds):
    exception: 1
    success: 238
    warnings: 39
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-100064210a1a

Comment 7

[Mozilla RelEng Bot]

Try run for 100064210a1a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=100064210a1a
Results (out of 293 total builds):
    exception: 1
    success: 239
    warnings: 39
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-100064210a1a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This can't be checked in without a review+. Also, please give your patches a commit message in the future.

Comment 9

[cmtalbert]

Paul, 

Reflag your patch with the review: ? flag, you can have either myself or Ted review it.  And craft the patch like this: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F (that's what comment 8 is referring to).

Thanks!

Comment 10

[Paul Dagnelie]

Attachment: Updated patch

Updated to have a proper commit message, and flagging for review.

Comment 11

[cmtalbert]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6eaf90901568

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6eaf90901568

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 649318 [details] [diff] [review]
Updated patch

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

::: testing/xpcshell/runxpcshelltests.py
@@ +738,5 @@
>          proc = self.launchProcess(completeCmd,
>                      stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir)
>  
> +        if interactive:
> +          self.log.info("TEST-INFO | %s | Process ID: %d" % (name, proc.pid))

Should have used 4 spaces...

Comment 14

[cmtalbert]

doh. I missed that.(In reply to :Ms2ger from comment #13)
> Comment on attachment 649318 [details] [diff] [review]
> Updated patch
> 
> Review of attachment 649318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +738,5 @@
> >          proc = self.launchProcess(completeCmd,
> >                      stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir)
> >  
> > +        if interactive:
> > +          self.log.info("TEST-INFO | %s | Process ID: %d" % (name, proc.pid))
> 
> Should have used 4 spaces...
The overwhelming style is 2 space in this file (it's mixed, actually). I'd rather add something that meshes with the rest of the file and then do a white space fix to pep 8 it later.