[webkit-reviews] review denied: [Bug 38018] new-run-webkit-tests: refactor much of the printing code out of run_webkit_tests.py : [Attachment 54343] update w/ ojan and eseidel's feedback

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 38018: new-run-webkit-tests: refactor much of the printing code out of
run_webkit_tests.py
https://bugs.webkit.org/show_bug.cgi?id=38018

Attachment 54343: update w/ ojan and eseidel's feedback
https://bugs.webkit.org/attachment.cgi?id=54343&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:98
 +  --print 'everything' is equivalent to --print '%s'.
There is some way in python to have named string replacements.	That would
probably be clearer here, but is also not a big deal.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:119
 +	    optparse.make_option("--sources", action="store_true",
Any reason why we shouldn't just remove it now?  rs=me to remove it now or
later.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:162
 +	    switches = switches.union(set(PRINT_EVERYTHING.split(',')))
I believe switches.update() is what you're looking for.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:196
 +	      options		 OptionParser object w/ command line settings
I'd probably write "with" instead of w/ since you have space.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:248
 +	    self.write(msg, 'config')
Wow, now these start to look like we should just be doing self.print(CONFIG,
msg)

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:352
 +	    if self._current_progress_str == "":
if not self. _current_progress_str?

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:1
53
 +	def test_print_actual(self):
OMG these tests are a lot of copy/paste code.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:3
46
 +	    printer, err, out = self.get_printer(
Toooo much copy paste.	This could all be abstracted.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:172
 +	TODO(dpranke): split this data structure into a separate class?
For better or worse we use FIXME in WebKit instead of TODO(name).

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:193
 +	tbe = result_summary.tests_by_expectation
I would rather have written a helper function to which you pass result_summary
and NOW, PASS, etc.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:813
 +		exp_str = self._expectations.get_expectations_string(
I don't think we use get_ in WebKit.  Certainly not in c++ code.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1097
 +	    self._printer.print_timing("  Median:	   %6.3f" % median)
I would think that this would be cleaner if you built the string first, and
then called print_timing.  Certainly less copied code between lines?

Overall, I like the idea of this change.  I need to look at it again.  I had a
hard time seeing through the wall of copy/paste that was the unittests. :( 
(Maybe they weren't copy/paste and I just read it wrong).

Will look again in a bit.