run_webkit_tests.py is large, unwieldy, and hard to understand. We need to split a lot of the logic out into multiple files and clean up our modularity and information hiding. It's perhaps easiest to start with all of the logging/printing code, as it's scattered throughout everything and all of the --log/--print options have become hard to follow. This bug will track that progress.
Created attachment 54117 [details] patch
Attachment 54117 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:45: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:73: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:84: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:93: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:119: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:120: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:129: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:355: too many blank lines (2) [pep8/E303] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:376: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:167: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:900: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1024: missing whitespace after ',' [pep8/E231] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1607: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1611: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/port/test.py:74: trailing whitespace [pep8/W291] [5] Total errors found: 15 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 54118 [details] clean up style nits
Created attachment 54183 [details] merge in --no-retry-failures patch committed recently
Comment on attachment 54183 [details] merge in --no-retry-failures patch committed recently It's great to move this code out of run-webkit-tests. I didn't look over the unittest code in detail. Also, I assume with the larger print functions, you're just copy-pasting the code, right? I didn't look at those too closely. WebKitTools/ChangeLog:85 + This change also renames --log to --print (to be slightly more This seems gratuitous to me. WebKitTools/ChangeLog:114 + :e ummm... WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:79 + - if --trace 'everything' is specified, it will override the '-progress' Why have --trace in addition to print? Why not have print just accept trace-unexepected and trace-everything as options? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:109 + help="(DEPRECATED. same as --print)"), As I said, I don't see the benefit of renaming log. If you insist on it, it should use Eric's new deprecated functionality so it doesn't show up by default when you --help. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:112 + help=("controls test tracing output of test run. " More reason I don't think this should be a separate flage. "test tracing output" can mean anything. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:120 + optparse.make_option("--sources", action="store_true", I thought we were getting rid of this. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:125 + optparse.make_option("--num-slow-tests-to-log", default=50, Can we add a "slowest" value to --print and get rid of this flag? The functionality of being able to specify how many slow tests to log is obviated by having the test results dashboard. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:145 + """Class handling all non-debug output done by run-webkit-tests.""" Can you document these arguments? For example, I don't know what "regular_output" means. Perhaps a more precise word than "regular"? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:147 + child_processes, is_fully_parallel): Aren't child_processes and is_fully_parallel already a part of options? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:173 + def _exit_early(self, method): exit_early is really generic. How about something about printing/logging? _avoids_printing? _should_print? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:421 + self._meter.update("%s" % msg) Can't you just use msg directly here? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:144 + class Controller(object): Generally, filenames should match the primary class in the file, no? Also, Controller is super generic. Output actually seems like a better name. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:84 + return tr why create the local variable? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:97 + class TestFunctions(unittest.TestCase): Maybe call this TestUtilityFunctions?
(In reply to comment #5) > (From update of attachment 54183 [details]) > It's great to move this code out of run-webkit-tests. I didn't look over the > unittest code in detail. Also, I assume with the larger print functions, you're > just copy-pasting the code, right? I didn't look at those too closely. > Yes, pretty much. adjusted the local variables where necessary. > WebKitTools/ChangeLog:85 > + This change also renames --log to --print (to be slightly more > This seems gratuitous to me. I think --print is more intuitive and helps to separate the concept from debug logging, but I don't fell strongly about this. > > WebKitTools/ChangeLog:114 > + :e > ummm... whoops :) > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:79 > + - if --trace 'everything' is specified, it will override the > '-progress' > Why have --trace in addition to print? Why not have print just accept > trace-unexepected and trace-everything as options? Yeah, I thought about combining the two flags and wasn't sure if it was a good idea or not. I can add it back in. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:109 > + help="(DEPRECATED. same as --print)"), > As I said, I don't see the benefit of renaming log. If you insist on it, it > should use Eric's new deprecated functionality so it doesn't show up by default > when you --help. That functionality doesn't actually exist yet ;) > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:112 > + help=("controls test tracing output of test run. " > More reason I don't think this should be a separate flage. "test tracing > output" can mean anything. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:120 > + optparse.make_option("--sources", action="store_true", > I thought we were getting rid of this. > We will, but I didn't want to do it as part of this patch, just to avoid confusing things further. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:125 > + optparse.make_option("--num-slow-tests-to-log", default=50, > Can we add a "slowest" value to --print and get rid of this flag? The > functionality of being able to specify how many slow tests to log is obviated > by having the test results dashboard. > Sure. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:145 > + """Class handling all non-debug output done by run-webkit-tests.""" > Can you document these arguments? For example, I don't know what > "regular_output" means. Perhaps a more precise word than "regular"? Oops, yeah, I meant to. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:147 > + child_processes, is_fully_parallel): > Aren't child_processes and is_fully_parallel already a part of options? They are, normally, but I made them arguments here so that output.py didn't assume that options had been set that it hadn't actually asked to be set. It seemed like a layering violation. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:173 > + def _exit_early(self, method): > exit_early is really generic. How about something about printing/logging? > _avoids_printing? _should_print? Okay. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:421 > + self._meter.update("%s" % msg) > Can't you just use msg directly here? Yeah, probably. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:144 > + class Controller(object): > Generally, filenames should match the primary class in the file, no? Also, > Controller is super generic. Output actually seems like a better name. Is that regular python practice? I'm not generally sure (and this is one of the things I find most awkward about Python). I agree that Controller is a better name; I couldn't think of anything better. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:84 > + return tr > why create the local variable? > As opposed to returning the function call result directly()? This made it easier to debug, but there's no good reason. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:97 > + class TestFunctions(unittest.TestCase): > Maybe call this TestUtilityFunctions? Okay.
(In reply to comment #6) > (In reply to comment #5) > > WebKitTools/ChangeLog:85 > > + This change also renames --log to --print (to be slightly more > > This seems gratuitous to me. > > I think --print is more intuitive and helps to separate the concept from debug > logging, but I don't fell strongly about this. Fair enough. As long as we only end up with one option visible to users of the script. Do any bots currently use --log? If not, then we can just rename it. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:79 > > + - if --trace 'everything' is specified, it will override the > > '-progress' > > Why have --trace in addition to print? Why not have print just accept > > trace-unexepected and trace-everything as options? > > Yeah, I thought about combining the two flags and wasn't sure if it was a good > idea or not. I can add it back in. FWIW, I don't see a benefit to it being a separate flag. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:147 > > + child_processes, is_fully_parallel): > > Aren't child_processes and is_fully_parallel already a part of options? > > They are, normally, but I made them arguments here so that output.py didn't > assume that options had been set that it hadn't actually asked to be set. It > seemed like a layering violation. I could see the layering violation argument if we weren't already passing in options. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:144 > > + class Controller(object): > > Generally, filenames should match the primary class in the file, no? Also, > > Controller is super generic. Output actually seems like a better name. > > Is that regular python practice? I'm not generally sure (and this is one of the > things I find most awkward about Python). > > I agree that Controller is a better name; I couldn't think of anything better. Lols. I was saying the opposite. Output seems like a better name to me than Controller. A controller can do anything really. May as well call it Manager or ThingDoer or Thingy. :) At least with Output, it's clear that it's output related. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output_unittest.py:84 > > + return tr > > why create the local variable? > > > > As opposed to returning the function call result directly()? This made it > easier to debug, but there's no good reason. Not a big deal. I was cringing at the two letter variable name really and saw that it could just be removed. :)
(In reply to comment #6) > > Generally, filenames should match the primary class in the file, no? Also, > > Controller is super generic. Output actually seems like a better name. > > Is that regular python practice? I'm not generally sure (and this is one of the > things I find most awkward about Python). > > I agree that Controller is a better name; I couldn't think of anything better. > Er, I meant "bad name". Per IM discussion w/ Ojan I'll change the module to "printing" and the class to "Printer".
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > WebKitTools/ChangeLog:85 > > > + This change also renames --log to --print (to be slightly more > > > This seems gratuitous to me. > > > > I think --print is more intuitive and helps to separate the concept from debug > > logging, but I don't fell strongly about this. > > Fair enough. As long as we only end up with one option visible to users of the > script. Do any bots currently use --log? If not, then we can just rename it. I don't think so; the bots use --verbose. I still have some muscle memory for --log, so I'm inclined to smooth the transition over for a while. > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:79 > > > + - if --trace 'everything' is specified, it will override the > > > '-progress' > > > Why have --trace in addition to print? Why not have print just accept > > > trace-unexepected and trace-everything as options? > > > > Yeah, I thought about combining the two flags and wasn't sure if it was a good > > idea or not. I can add it back in. > > FWIW, I don't see a benefit to it being a separate flag. Yeah, I've changed it. As you say, it does clean up some things. > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/output.py:147 > > > + child_processes, is_fully_parallel): > > > Aren't child_processes and is_fully_parallel already a part of options? > > > > They are, normally, but I made them arguments here so that output.py didn't > > assume that options had been set that it hadn't actually asked to be set. It > > seemed like a layering violation. > > I could see the layering violation argument if we weren't already passing in > options. > I clarified this in the docstring to the function, but I don't want to assume that options have been set that didn't come from the print_options() function. Note that we don't need these variables at all if we get rid of detailed-progress.
Created attachment 54196 [details] revised patch w/ Ojan's feedback
Comment on attachment 54196 [details] revised patch w/ Ojan's feedback WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:44 + NUM_SLOW_TESTS_TO_LOG = 10 Nice that you changed the default to match ORWT. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:76 + slowest print %d slowest tests and the time they took to run Yay for printing defaults! WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:101 + # NOTE: we keep this around because "print" is a reserved word and This doesn't seem like the real reason. we'll eventually kill --log, no? But obviously we still can't name the option "print" then. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:188 + def _exit_early(self, method): Seems we should process converting everything -> a set of switches up front instead of here. You mentioned that would have the advantage of being able to print what "everything" actually means. We could also then print errors when people specify conflicting options. Also we should consider renaming this. If everything was processed up front, then we could use this check everywhere intead of manual flag in self._switches. self.enabled("switch name") WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:199 + if self._exit_early('actual'): This then becomes: if not self.enabled("actual") return or similar. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:253 + self._write('trace: %s\n' % test_name) I wonder if this bit would be clearer as a multi-line string literal with names. Not sure. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:291 + percent_complete = 100 * (result_summary.expected + I'm tempted to say this should be a method on result_summary. :) WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:300 + def _print_detailed_progress(self, result_summary, test_list): This is a mind-nummingly complicated hack. I'm glad we have it, but i'm also glad it will die soon. It's nice to know its there in case there is a rebellion when we switch to NRWT. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:327 + self._current_progress_str = "%s: " % self._current_dir _current_progress_string seems like a long name from a man who likes short names. :) Is there a non-current progress string? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:353 + for test, results in unexpected_results['tests'].iteritems(): This would be more clear to me if factored out into a function which returned (passes, flaky, regressions), I think... I was initially confused as to why there was processing going on inside a print method. Now I see you're just trying to convert the data into something easier to print. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:69 + def get_options(args): Does PEP8 do get_? WebKit c++ style avoids get_ in accessors. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:70 + lo = printing.print_options() and behold! :p option_list would have been clearer. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:86 + def get_result_summary(port_obj, test_files, expectations_str): Never understood why we call it _obj. Is there a non-obj port? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:92 + rs = run_webkit_tests.ResultSummary(expectations, test_files) A where 80c hurts readability. :( WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:123 + class Testprinter(unittest.TestCase): TestPrinter? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:106 + # self.assertFalse(stream.empty()) Why the commented out code? I'm only 30% of the way through after thus far a long review. :( I need to finish my patch for bug 38027 and eat some dinner. I'll look at this more this weekend.
Generallly speaking I am attempting to avoid changing anything except where necessary to clean up the logging behavior and to do what is strictly necessary to move this code into a new file. There are definitely other things to be cleaned up in this code but I don't wish to cram too many substantive changes in with the refactoring. (In reply to comment #11) > (From update of attachment 54196 [details]) > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:44 > + NUM_SLOW_TESTS_TO_LOG = 10 > Nice that you changed the default to match ORWT. > It seemed like a decent compromise between having a specifiable number and getting rid of the flag completely. I definitely think 50 is too many. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:76 > + slowest print %d slowest tests and the time they took to > run > Yay for printing defaults! > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:101 > + # NOTE: we keep this around because "print" is a reserved word and > This doesn't seem like the real reason. we'll eventually kill --log, no? But > obviously we still can't name the option "print" then. > Well, yeah. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:188 > + def _exit_early(self, method): > Seems we should process converting everything -> a set of switches up front > instead of here. You mentioned that would have the advantage of being able to > print what "everything" actually means. We could also then print errors when > people specify conflicting options. > > Also we should consider renaming this. If everything was processed up front, > then we could use this check everywhere intead of manual flag in > self._switches. > > self.enabled("switch name") > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:199 > + if self._exit_early('actual'): > This then becomes: > if not self.enabled("actual") return or similar. > Can do. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:253 > + self._write('trace: %s\n' % test_name) > I wonder if this bit would be clearer as a multi-line string literal with > names. Not sure. > If we do multi-line output it gets screwed up when using the --verbose switch (we lose the stuff that gets prepended to the line by the logger), so multi-line is bad in most cases. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:291 > + percent_complete = 100 * (result_summary.expected + > I'm tempted to say this should be a method on result_summary. :) > Yeah, but I'm not moving it in this change. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:300 > + def _print_detailed_progress(self, result_summary, test_list): > This is a mind-nummingly complicated hack. I'm glad we have it, but i'm also > glad it will die soon. It's nice to know its there in case there is a > rebellion when we switch to NRWT. Agreed. I've actually changed the default to one-line-progress across the board (it used to be detailed-progress that would automatically fall back to one-line-progress if detailed-progress couldn't be done). > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:327 > + self._current_progress_str = "%s: " % self._current_dir > _current_progress_string seems like a long name from a man who likes short > names. :) Is there a non-current progress string? All of the text we've already printed and flushed; this string is the one that can be overwritten. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:353 > + for test, results in unexpected_results['tests'].iteritems(): > This would be more clear to me if factored out into a function which returned > (passes, flaky, regressions), I think... I was initially confused as to why > there was processing going on inside a print method. Now I see you're just > trying to convert the data into something easier to print. > Yup, this code can definitely be cleaned up, but I don't want to do it here. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:69 > + def get_options(args): > Does PEP8 do get_? WebKit c++ style avoids get_ in accessors. > Python style is to refer to member variables directly and not use accessors at all. This isn't an access, it's a standalone, perhaps confusingly named, function. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:70 > + lo = printing.print_options() > and behold! :p option_list would have been clearer. perhaps. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:86 > + def get_result_summary(port_obj, test_files, expectations_str): > Never understood why we call it _obj. Is there a non-obj port? Yes, the name of the package (as in "import port"). I have to call it "port_obj" to avoid a name conflict. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:92 > + rs = run_webkit_tests.ResultSummary(expectations, test_files) > A where 80c hurts readability. :( > They do crop up from time to time. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:123 > + class Testprinter(unittest.TestCase): > TestPrinter? > That looks like a search&replace goof. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:106 > + # self.assertFalse(stream.empty()) > Why the commented out code? > Because of the FIXME's above; these tests don't actually work correctly. > I'm only 30% of the way through after thus far a long review. :( > > I need to finish my patch for bug 38027 and eat some dinner. I'll look at this > more this weekend. Okay. I will wait until you are done with the review before posting a patch with any changes.
Comment on attachment 54196 [details] revised patch w/ Ojan's feedback WebKitTools/ChangeLog:12 + The new code has unit tests! Yay! I haven't looked at the actual change, but the pattern of green and red and tests makes me very happy. :)
Couple more nits. After this and addressing Eric's feedback, LGTM. WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:88 + options These last two could be included in the description of trace-*. E.g. for trace-unexpected you could just add: (overrides 'unexpected') WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:109 + optparse.make_option("--sources", action="store_true", Maybe include a FIXME to get rid of this? WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:433 + self._meter.update("%s" % msg) Forgot to fix this? > > Fair enough. As long as we only end up with one option visible to users of the > > script. Do any bots currently use --log? If not, then we can just rename it. > > I don't think so; the bots use --verbose. I still have some muscle memory for > --log, so I'm inclined to smooth the transition over for a while. I'd be surprised if anyone but you depended on --log since it's so new. So, I'd rather just do the rename now, but it's not a big deal. I trust you to remove it in an upcoming cleanup. :)
(In reply to comment #14) > Couple more nits. After this and addressing Eric's feedback, LGTM. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:88 > + options > These last two could be included in the description of trace-*. E.g. for > trace-unexpected you could just add: > (overrides 'unexpected') > Good suggestion. Done. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:109 > + optparse.make_option("--sources", action="store_true", > Maybe include a FIXME to get rid of this? > Done. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:433 > + self._meter.update("%s" % msg) > Forgot to fix this? > Guess so. Done. > > > Fair enough. As long as we only end up with one option visible to users of the > > > script. Do any bots currently use --log? If not, then we can just rename it. > > > > I don't think so; the bots use --verbose. I still have some muscle memory for > > --log, so I'm inclined to smooth the transition over for a while. > > I'd be surprised if anyone but you depended on --log since it's so new. So, I'd > rather just do the rename now, but it's not a big deal. I trust you to remove > it in an upcoming cleanup. :) You're probably right. I've removed it.
Created attachment 54343 [details] update w/ ojan and eseidel's feedback
Attachment 54343 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:233: too many blank lines (2) [pep8/E303] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 54343 [details] update w/ ojan and eseidel's feedback 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:153 + 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:346 + 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.
For future reference, at least with our limited review tools, 100k is simply too large to really review in bugzilla. Ojan and I have been through this one enough now though that this is probably reviewable as is, but we should avoid changes this large in the future if possible.
+--print 'everything' is equivalent to --print '%s'. + +The default is to --print '%s'. +""" % (NUM_SLOW_TESTS_TO_LOG, PRINT_EVERYTHING, PRINT_DEFAULT) + >>> "%(greeting)s, %(name)s!" % {'greeting':'hi', 'name':'Dirk'} 'hi, Dirk!'
(In reply to comment #19) > For future reference, at least with our limited review tools, 100k is simply > too large to really review in bugzilla. Ojan and I have been through this one > enough now though that this is probably reviewable as is, but we should avoid > changes this large in the future if possible. For changes like this, where you're basically moving code from one file to another (and adding unit tests), what would you suggest as an alternative? Simply smaller, more incremental changes?
(In reply to comment #18) > (From update of attachment 54343 [details]) > 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. Added. > > 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. I'm trying to contain the scope of this change. > > 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. True. > > 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. > Done. > 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) > Yeah. That's by design. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:352 > + if self._current_progress_str == "": > if not self. _current_progress_str? > I'm not a big fan of using 'not' for testing if strings are empty. There's too many ways to be false, and only one way to be empty (well, two if I was to use len()). > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:153 > + 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:346 > + printer, err, out = self.get_printer( > Toooo much copy paste. This could all be abstracted. > These tests tend to be 4- or 5-line paragraphs where you initialize with something that changes in each paragraph, then run a test where one parameter varies, and then test that the output is slightly different. You can probably figure out how to implement these with somewhat less cut-and-paste effort, but it would involve writing helper functions with callbacks and the structure gets (IMO) a lot less clear. In my experience, unit testing is just repetitive :( I am open to suggestions if you have some in mind. > 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). > Sure. This wasn't part of the diff, but I've changed it. > 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. > Perhaps, but this code didn't change in this patch and I'd prefer not to rewrite it now. > 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. > It's not an accessor function. Do you have a better suggestion for a name? > 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? > One, I need to preserve the line-at-a-time invocation to get the timestamps right with --verbose. Two, I didn't want to change everything at once so I left this alone. > 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.
> > > > 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. > > I'm trying to contain the scope of this change. > > > > > 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. > > True. > To be more explicit here, removing this switch touches ~5 more files, which I'd just as soon not do at the same time as this. The scope and intent of the files that are changed in this patch is at least pretty clear.
Created attachment 54366 [details] more feedback from eseidel
Attachment 54366 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:101: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:102: whitespace before '}' [pep8/E202] [5] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 54366 [details] more feedback from eseidel The unit tests make me think this API is wrong, if we have to copy/paste so much code to test so little. Why not a method: self.test_printer(["args"], expected_err, expected_out) Wouldn't that get rid of a lot of the testing boilerplate? That still leaves the various print_* boilerplate of course.
I'm excited about improving NRWT. I'm tempted to just rs=me this, except the unit testing code as written is unmaintainable. Any time we copy/paste we make unmaintainable code. I think a large part of what has stalled this review so much is the size of the patch. Everyone is scared to r+ it. Even me. The size leaves a lot of space for gray area in the patch. It's not a black or white "yes this is all awesome" or "no this is all bad".
(In reply to comment #26) > (From update of attachment 54366 [details]) > The unit tests make me think this API is wrong, if we have to copy/paste so > much code to test so little. > This is the downside of switching behavior based on method name. > Why not a method: > self.test_printer(["args"], expected_err, expected_out) > > Wouldn't that get rid of a lot of the testing boilerplate? I'll spend some time see if I can come up with a variant that reduces the duplication and post it for comparison.
(In reply to comment #27) > I'm excited about improving NRWT. I'm tempted to just rs=me this, except the > unit testing code as written is unmaintainable. Any time we copy/paste we make > unmaintainable code. > In my experience, maintaining test code is almost always tedious and hard to maintain. Once you actually understand the patterns of the tests, it's actually pretty easy to maintain the duplicated code, but like I said in the previous comment, I'll see if I can make this clearer. > I think a large part of what has stalled this review so much is the size of the > patch. Everyone is scared to r+ it. Even me. The size leaves a lot of space > for gray area in the patch. It's not a black or white "yes this is all > awesome" or "no this is all bad". I'm not sure what to do with this comment. Are you suggesting I break this patch up into chunks and re-submit them?
Oh, in my example I meant to pass the method pointer in my example. printer.print_config or whatever it would be. (In reply to comment #29) > In my experience, maintaining test code is almost always tedious and hard to > maintain. Once you actually understand the patterns of the tests, it's actually > pretty easy to maintain the duplicated code, but like I said in the previous > comment, I'll see if I can make this clearer. Thanks. > > I think a large part of what has stalled this review so much is the size of the > > patch. Everyone is scared to r+ it. Even me. The size leaves a lot of space > > for gray area in the patch. It's not a black or white "yes this is all > > awesome" or "no this is all bad". > > I'm not sure what to do with this comment. Are you suggesting I break this > patch up into chunks and re-submit them? That is always an option. But I don't know if that's the best use of your time (up to you). I don't see super-easy divisions in this patch to cut on. I think it's more a comment towards the future.
Created attachment 54480 [details] try to reduce cut&paste duplication in test methods Okay, I've attached a patch that only changes printing_unittest.py, in an attempt to reduce the amount of copied code. Reducing duplication further gets tricky because the method invocation you're actually testing has varying arguments, and the output starts to vary. This can probably be done using an apply() call, but I'm not sure if this is worth it. Thoughts?
Comment on attachment 54480 [details] try to reduce cut&paste duplication in test methods I think we've said more than needs to be said about this patch. We should land it and move on. :) This all needs more iteration, holding up this patch only slows that down.
Created attachment 54495 [details] merge to HEAD
Attachment 54495 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:101: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:102: whitespace before '}' [pep8/E202] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:166: whitespace after '[' [pep8/E201] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:169: whitespace after '[' [pep8/E201] [5] WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing_unittest.py:178: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:41: expected 2 blank lines, found 1 [pep8/E302] [5] Total errors found: 6 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 54496 [details] lint
Comment on attachment 54480 [details] try to reduce cut&paste duplication in test methods Cleared Eric Seidel's review+ from obsolete attachment 54480 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Committed r58728: <http://trac.webkit.org/changeset/58728>