Closed Bug 779006 Opened 8 years ago Closed 8 years ago

Print process pid in check-interactive

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: pdagnelie, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch proposed patch (obsolete) — Splinter Review
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?
Attachment #647371 - Flags: feedback?(ctalbert)
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.
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...)
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
(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 on attachment 647371 [details] [diff] [review]
proposed patch

This looks good to me.
Attachment #647371 - Flags: feedback?(ctalbert) → feedback+
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/pdagnelie@mozilla.com-100064210a1a
Keywords: checkin-needed
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/pdagnelie@mozilla.com-100064210a1a
This can't be checked in without a review+. Also, please give your patches a commit message in the future.
Keywords: checkin-needed
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!
Attached patch Updated patchSplinter Review
Updated to have a proper commit message, and flagging for review.
Attachment #647371 - Attachment is obsolete: true
Attachment #649318 - Flags: review?(ctalbert)
Attachment #649318 - Flags: review?(ctalbert) → review+
https://hg.mozilla.org/mozilla-central/rev/6eaf90901568
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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...
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.
You need to log in before you can comment on or make changes to this bug.