| Summary: | test-webkitpy: add a -p flag to pass through captured output to enable debugging | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, eric, ojan, tony, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dirk Pranke
2012-06-14 19:38:55 PDT
Created attachment 147715 [details]
Patch
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. 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? (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? Created attachment 148386 [details]
update w/ review feedback
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? 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? (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 ... Created attachment 148465 [details]
update w/ more review feedback
trying again, I made the stack inspection clearer and more bulletproof, hopefully that helps. 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. (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! Committed r120794: <http://trac.webkit.org/changeset/120794> |