Bug 38616 – new-run-webkit-tests: clean up newline handling in printing

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

[Dirk Pranke]

new-run-webkit-tests: clean up newline handling in printing

[Dirk Pranke]

Created attachment 55179 [details]
Patch

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Committed r58853: <http://trac.webkit.org/changeset/58853>