Bug 38616 - new-run-webkit-tests: clean up newline handling in printing
Summary: new-run-webkit-tests: clean up newline handling in printing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-05 17:35 PDT by Dirk Pranke
Modified: 2010-05-07 19:38 PDT (History)
1 user (show)

See Also:


Attachments
Patch (28.54 KB, patch)
2010-05-05 17:36 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-05-05 17:35:20 PDT
new-run-webkit-tests: clean up newline handling in printing
Comment 1 Dirk Pranke 2010-05-05 17:36:36 PDT
Created attachment 55179 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-05-05 17:58:48 PDT
Comment on attachment 55179 [details]
Patch

WebKitTools/Scripts/webkitpy/common/array_stream.py:1
 +  #!/usr/bin/python
No need.

WebKitTools/Scripts/webkitpy/common/array_stream.py:36
 +      This is used primarily by unit test classes to mock output streams.
We should document when clients should use this instead of StringIO.  including how this differs from StringIO.

WebKitTools/Scripts/webkitpy/common/array_stream.py:55
 +          """Return whether the stream is empty."""
Although I think pep8 suggests these, they're often completely useless. :)

I'm not sure if is_empty() or empty() is preferred.  I know C++ webkit prefers isEmpty().

WebKitTools/Scripts/webkitpy/common/array_stream.py:46
 +      def get(self):
StringIO calls their equivalent getvalue() which is not pep8 compliant.  But just thought you might want to know.  If they had followed pep8, I guess I would have suggested we change to match.

WebKitTools/Scripts/webkitpy/common/array_stream.py:63
 +          return '<ArrayStream: ' + str(self._contents) + '>'
It's sad that every class has to implement this template.  I'm surprised object doesn't provide some sort of hook to allow you to fill in the middle.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:41
 +          self.assertTrue(a.empty())
You repeat this empty check enough times, I would have considered adding a def _assert_empty(stream) which did those two asserts.  It would probably only net a couple lines in the end but might make the main test method more clear.  Unsure.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:55
 +          self.assertEquals(a.get(), ["foo", "bar"])
Actually, you could easily add an assert_value() check which takes the value to assert and could itself figure out whether to assert empty true or false.  That would clean up several checks and make all of these places consistent.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:68
 +  if __name__ == '__main__':
Won't work anyway given how we import, so please remove.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/metered_stream.py:141
 +          if len(str):
I'm not sure why len() is clearer than just if str:  but I think you and I have had that discussion before. :)

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:486
 +          # from the logger :(.
I think cjerdonek knows some magic.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:674
 +          self._printer.print_update("All threads started ...")
Sigh.  You'll conflict with changes I have locally to this method.  We hang at this message for several seconds for reasons I don't understand.


If you're not particularly partial to the log change, you should consider reverting it to not conflict with bug 38561 which moves code around in that file.

Looks OK.  Please consider updating the unit tests before landing.  I don't need to see this again.
Comment 3 Dirk Pranke 2010-05-05 18:20:08 PDT
(In reply to comment #2)
> (From update of attachment 55179 [details])
> WebKitTools/Scripts/webkitpy/common/array_stream.py:1
>  +  #!/usr/bin/python
> No need.

Sure, but I think it's generally good style to include a shebang line. 

> 
> WebKitTools/Scripts/webkitpy/common/array_stream.py:36
>  +      This is used primarily by unit test classes to mock output streams.
> We should document when clients should use this instead of StringIO.  including
> how this differs from StringIO.
> 

Will do.

> WebKitTools/Scripts/webkitpy/common/array_stream.py:55
>  +          """Return whether the stream is empty."""
> Although I think pep8 suggests these, they're often completely useless. :)
>

Yeah.
 
> I'm not sure if is_empty() or empty() is preferred.  I know C++ webkit prefers
> isEmpty().
> 

In the absence of a standard, I will leave it as empty().

> WebKitTools/Scripts/webkitpy/common/array_stream.py:46
>  +      def get(self):
> StringIO calls their equivalent getvalue() which is not pep8 compliant.  But
> just thought you might want to know.  If they had followed pep8, I guess I
> would have suggested we change to match.

If I was feeling verbose, I probably would've called it get_contents(). But I'm not.

> 
> WebKitTools/Scripts/webkitpy/common/array_stream.py:63
>  +          return '<ArrayStream: ' + str(self._contents) + '>'
> It's sad that every class has to implement this template.  I'm surprised object
> doesn't provide some sort of hook to allow you to fill in the middle.
>

Agreed.
 
> WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:41
>  +          self.assertTrue(a.empty())
> You repeat this empty check enough times, I would have considered adding a def
> _assert_empty(stream) which did those two asserts.  It would probably only net
> a couple lines in the end but might make the main test method more clear. 
> Unsure.
> 
> WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:55
>  +          self.assertEquals(a.get(), ["foo", "bar"])
> Actually, you could easily add an assert_value() check which takes the value to
> assert and could itself figure out whether to assert empty true or false.  That
> would clean up several checks and make all of these places consistent.
>

Okay, I can add this.
 
> WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:68
>  +  if __name__ == '__main__':
> Won't work anyway given how we import, so please remove.

It works fine for me, and I use this mechanism all the time. You just need WebKitTools/Scripts in your PYTHONPATH.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/metered_stream.py:141
>  +          if len(str):
> I'm not sure why len() is clearer than just if str:  but I think you and I have
> had that discussion before. :)

if str is false if str is either None or ""; len(str) will bail out on None, which is probably a good thing.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:486
>  +          # from the logger :(.
> I think cjerdonek knows some magic.
> 
> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:674
>  +          self._printer.print_update("All threads started ...")
> Sigh.  You'll conflict with changes I have locally to this method.  We hang at
> this message for several seconds for reasons I don't understand.
>
> 
> If you're not particularly partial to the log change, you should consider
> reverting it to not conflict with bug 38561 which moves code around in that
> file.
> 
> Looks OK.  Please consider updating the unit tests before landing.  I don't
> need to see this again.

I will undo this change; see the comment I made while reviewing your bug, though.
Comment 4 Dirk Pranke 2010-05-05 18:33:43 PDT
Committed r58853: <http://trac.webkit.org/changeset/58853>