Bug 89158 – test-webkitpy: add a -p flag to pass through captured output to enable debugging

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

test-webkitpy: add a -p flag to pass through captured output to enable debugging

[Dirk Pranke]

Created attachment 147715 [details]
Patch

[Dirk Pranke]

I've tested this manually, but I'm not sure how to test this in an automated way, since it would require test-webkitpy to test itself in a particularly insidious manner (I'm not actually sure it's even possible).

I'm not sure it's worth adding tests, but if anyone has any bright ideas I'm open to them.

[Eric Seidel (no email)]

Comment on attachment 147715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147715&action=review

> Tools/Scripts/webkitpy/common/system/outputcapture.py:41
> +# By default we capture the output to a stream. Other modules may override
> +# this function in order to do things like pass through the output. See
> +# webkitpy.test.main for an example.
> +def stream_wrapper(stream):
> +    return StringIO()

Why not have this as a class method/object on outputcaputure.OutputCapture?  Couldn't you then just do outputcapture.OutputCapture.stream_wrapper = Tee to override it?

> Tools/Scripts/webkitpy/test/main.py:198
> +class Tee(object):

We have a Tee object in deprecatedlogging.py  Maybe we should split out a tee.py file?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 147715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147715&action=review
> 
> > Tools/Scripts/webkitpy/common/system/outputcapture.py:41
> > +# By default we capture the output to a stream. Other modules may override
> > +# this function in order to do things like pass through the output. See
> > +# webkitpy.test.main for an example.
> > +def stream_wrapper(stream):
> > +    return StringIO()
> 
> Why not have this as a class method/object on outputcaputure.OutputCapture?  Couldn't you then just do outputcapture.OutputCapture.stream_wrapper = Tee to override it?
> 

Sure, I can do that. Seems like the same either way to me, but I know you don't seem to like free variables and functions.

> > Tools/Scripts/webkitpy/test/main.py:198
> > +class Tee(object):
> 
> We have a Tee object in deprecatedlogging.py  Maybe we should split out a tee.py file?

I didn't know about that class. However, having now looked at it, it seems hardly worth it for a class that is fewer lines of code than the license text. Can we leave things like this and then just focus on getting rid of deprecatedlogging?

[Dirk Pranke]

Created attachment 148386 [details]
update w/ review feedback

[Dirk Pranke]

Okay, having gone through this again, there's three changes.

1) I moved the wrapper to a static method as per your suggestion

2) I realized that your Tee class was more like a proper tee, and mine wasn't, so I renamed mine to avoid confusion. We should still get rid of deprecated_logging :)

3) I added in some code to make sure that output from the debugger itself is not included in the captured stream. I had originally had this in my patch, but for some reason when I was testing the version I initially uploaded, I didn't seem to need it. Testing again now, I can't reproduce that and it seems clear that I do need to filter out the debugger output (which makes more sense).

WDYT?

[Eric Seidel (no email)]

Comment on attachment 148386 [details]
update w/ review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=148386&action=review

Looks OK.  the --pass-through doesn't seem like quite the right name... since this seems to be very specific to the python debugger.

> Tools/Scripts/webkitpy/common/system/outputcapture.py:43
> +    # By default we capture the output to a stream. Other modules may override
> +    # this function in order to do things like pass through the output. See
> +    # webkitpy.test.main for an example.
> +    @staticmethod
> +    def stream_wrapper(stream):
> +        return StringIO()

Thank you.  This is much more appealing to my eyes. :)

> Tools/Scripts/webkitpy/test/main.py:65
> +        parser.add_option('-p', '--pass-through', action='store_true', default=False,
> +                          help='enable debugging by passing captured output through to the system')

You should probably correct this to indicate that this is specific to the python debugger, sicne it seems to do some sort fo stack check.

> Tools/Scripts/webkitpy/test/main.py:210
> +        if not stack[1][1].endswith('cmd.py'):

Why cmd.py?  Where does that come from?  Maybe this should be a self._in_debugger() check instead?

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 148386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148386&action=review
> 
> Looks OK.  the --pass-through doesn't seem like quite the right name... since this seems to be very specific to the python debugger.

Technically it's useful even if you aren't running under the debugger, if you actually want to see the output of stdout/stderr when running the tests (e.g., if you were just adding printfs).

That said, I'm certainly open to other names.

> 
> You should probably correct this to indicate that this is specific to the python debugger, sicne it seems to do some sort fo stack check.
>

Note above, but I will try to clarify.
 
> > Tools/Scripts/webkitpy/test/main.py:210
> > +        if not stack[1][1].endswith('cmd.py'):
> 
> Why cmd.py?  Where does that come from?
>  Maybe this should be a self._in_debugger() check instead?

That's just the basename of the first stack frame above in the debugger. I will try to clarify the comment. I'm not sure that adding a separate method would help beyond the comment that's already there ...

[Dirk Pranke]

Created attachment 148465 [details]
update w/ more review feedback

[Dirk Pranke]

trying again, I made the stack inspection clearer and more bulletproof, hopefully that helps.

[Eric Seidel (no email)]

Comment on attachment 148465 [details]
update w/ more review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=148465&action=review

Looks great.  I don't need to see this again, but you might find my nits below helpful (or not). :)  Thanks again.

> Tools/Scripts/webkitpy/test/main.py:65
> +        parser.add_option('-p', '--pass-through', action='store_true', default=False,
> +                          help='enable the python debugger by passing captured output through to the system')

"enable the python debugger" sounds like it's turning on the debugger.  Maybe you want it do do that too?

> Tools/Scripts/webkitpy/test/main.py:199
> +class _CaptureAndPassThroughStream(object):

This name is no longer probably specific enough, now that I understand what this code does. :)  It probably should mention the debugger or pdb.

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 148465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148465&action=review
> 
> Looks great.  I don't need to see this again, but you might find my nits below helpful (or not). :)  Thanks again.
> 
> > Tools/Scripts/webkitpy/test/main.py:65
> > +        parser.add_option('-p', '--pass-through', action='store_true', default=False,
> > +                          help='enable the python debugger by passing captured output through to the system')
> 
> "enable the python debugger" sounds like it's turning on the debugger.  Maybe you want it do do that too?
> 

No, I don't want to actually start things in the debugger with this flag; usually I put specific breakpoints in the routines I'm testing, and starting in the debugger would just be annoying. If others end up using this I'd be happy to add a --start-in-debugger flag, though.

> > Tools/Scripts/webkitpy/test/main.py:199
> > +class _CaptureAndPassThroughStream(object):
> 
> This name is no longer probably specific enough, now that I understand what this code does. :)  It probably should mention the debugger or pdb.

I'll see what I can come up with. Thanks!

[Dirk Pranke]

Committed r120794: <http://trac.webkit.org/changeset/120794>