| Summary: | new-run-webkit-tests: implement a --log trace message to be able to display detailed output of an individual test run | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||||||||||
| Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, cjerdonek, eric, levin, ojan, tony, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Dirk Pranke
2010-04-16 13:35:12 PDT
Created attachment 53558 [details]
patch
Created attachment 53561 [details]
re-sync to tip of tree to merge some things
Attachment 53561 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:364: trailing whitespace [pep8/W291] [5]
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:429: trailing whitespace [pep8/W291] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Nifty. I could see this information being always printed into results.html as being useful (in some slightly more condense form). Drive by comment: This output seems really useful for tracking down unexpected failures. Would be nice if this output were the default for any unexpected failures (but not for passes or expected failures). Created attachment 53578 [details]
fix style nits
(In reply to comment #5) > Drive by comment: This output seems really useful for tracking down unexpected > failures. Would be nice if this output were the default for any unexpected > failures (but not for passes or expected failures). Ojan, yeah, you're probably right. Let me land this first series of changes and then I can look into changing that. (In reply to comment #6) > Created an attachment (id=53578) [details] I have several comments on this patch if it can wait until tonight. Thanks. (In reply to comment #8) > (In reply to comment #6) > > Created an attachment (id=53578) [details] [details] > > I have several comments on this patch if it can wait until tonight. Thanks. Sure, I can wait. I'll probably go ahead and post the other patches so you can get a sense of where I'm going with this. Hopefully that won't result in too much re-merging down the road. (In reply to comment #6) > Created an attachment (id=53578) [details] Here are several comments to consider that are mostly stylistic. If possible, I'd like to provide feedback on the substance of the patch after you give consideration to these comments. --- a/WebKitTools/ChangeLog + https://bugs.webkit.org/show_bug.cgi?id=37726 + + * Scripts/webkitpy/layout_tests/run_webkit_tests.py: + - use the newly exposed TestResult class and implement + --log trace + * Scripts/webkitpy/layout_tests/layout_package/dump_render_thread.py: + - rename TestStats to TestResult and make it more public, resulting + in cleaner code It looks like the ChangeLog is missing an entry for-- WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py @@ -54,8 +54,8 @@ _log = logging.getLogger("webkitpy.layout_tests.layout_package." def process_output(port, test_info, test_types, test_args, configuration, output_dir, crash, timeout, test_run_time, actual_checksum, output, error): - """Receives the output from a DumpRenderTree process, subjects it to a number - of tests, and returns a list of failure types the test produced. + """Receives the output from a DumpRenderTree process, subjects it to a + number of tests, and returns a list of failure types the test produced. Per PEP8, the first line of a docstring should fit on one line and be followed by a blank line. Also per PEP8, the first line should be phrased as a command, for example "Process DumpRenderTree output, and return...." (By the way, that parameter list is a bit long. It should probably be refactored to have a much shorter parameter list.) @@ -66,7 +66,7 @@ def process_output(port, test_info, test_types, test_args, configuration, configuration: Debug or Release output_dir: directory to put crash stack traces into - Returns: a list of failure objects and times for the test being processed + Returns: a TestResult object """ Per PEP8, a blank line should separate the final line of a multi-line docstring and the triple quotes. @@ -111,16 +111,17 @@ def process_output(port, test_info, test_types, test_args, configuration, time.time() - start_diff_time) total_time_for_all_diffs = time.time() - start_diff_time - return TestStats(test_info.filename, failures, test_run_time, + return TestResult(test_info.filename, failures, test_run_time, total_time_for_all_diffs, time_for_diffs) The second line should line up with the character following the left parenthesis. Also, when calling a function with many parameters, it's good to use named parameters. This is a good safety check (especially when used in conjunction with unit tests) since it adds an additional level of checking. -class TestStats: +class TestResult: Classes at the bottom of your class hierarchy should really inherit from "object". See, for example-- http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Classes def __init__(self, filename, failures, test_run_time, total_time_for_all_diffs, time_for_diffs): self.filename = filename self.failures = failures + self.type = test_failures.determine_result_type(failures) self.test_run_time = test_run_time self.total_time_for_all_diffs = total_time_for_all_diffs self.time_for_diffs = time_for_diffs In constructors, it is good to alphabetize the lines that set data attributes by data attribute name. @@ -157,14 +158,14 @@ class SingleTestThread(threading.Thread): driver.run_test(test_info.uri.strip(), test_info.timeout, test_info.image_hash) end = time.time() - self._test_stats = process_output(self._port, + self._test_result = process_output(self._port, test_info, self._test_types, self._test_args, self._configuration, self._output_dir, crash, timeout, end - start, actual_checksum, output, error) driver.stop() Again, align with the left parenthesis if possible and use named parameters for long parameter lists. - def get_test_stats(self): - return self._test_stats + def get_test_result(self): + return self._test_result class TestShellThread(threading.Thread): @@ -201,7 +202,7 @@ class TestShellThread(threading.Thread): self._canceled = False self._exception_info = None self._directory_timing_stats = {} - self._test_stats = [] + self._test_results = [] self._num_tests = 0 self._start_time = 0 self._stop_time = 0 Alphabetize by data attribute name. @@ -218,10 +219,10 @@ class TestShellThread(threading.Thread): (number of tests in that directory, time to run the tests)""" return self._directory_timing_stats - def get_individual_test_stats(self): - """Returns a list of (test_filename, time_to_run_test, - total_time_for_all_diffs, time_for_diffs) tuples.""" - return self._test_stats + def get_test_results(self): + """Returns the list of all tests run on this thread. Used to calculate + per-thread statistics.""" + return self._test_results Again, phrase the first line of a doc string as a command (e.g. "Return the list..." rather than "Returns....") and restrict the first line to one line followed by a blank line. def cancel(self): """Set a flag telling this thread to quit.""" @@ -317,27 +318,29 @@ class TestShellThread(threading.Thread): batch_count += 1 self._num_tests += 1 if self._options.run_singly: - failures = self._run_test_singly(test_info) + result = self._run_test_singly(test_info) else: - failures = self._run_test(test_info) + result = self._run_test(test_info) You may wish to use the ternary operator here. filename = test_info.filename tests_run_file.write(filename + "\n") - if failures: + if result.failures: # Check and kill DumpRenderTree if we need too. too -> to - if len([1 for f in failures if f.should_kill_dump_render_tree()]): + if len([1 for f in result.failures + if f.should_kill_dump_render_tree()]): It seems this construct can be simplified. For example, can you use the filter() construct instead? The 1's don't really do anything. Also fix the alignment. # Print the error message(s). - error_str = '\n'.join([' ' + f.message() for f in failures]) + error_str = '\n'.join([' ' + f.message() for + f in result.failures]) Alignment. _log.debug("%s %s failed:\n%s" % (self.getName(), self._port.relative_test_filename(filename), error_str)) else: _log.debug("%s %s passed" % (self.getName(), self._port.relative_test_filename(filename))) - self._result_queue.put((filename, failures)) + self._result_queue.put(result) if batch_size > 0 and batch_count > batch_size: # Bounce the shell and reset count. @@ -358,7 +361,7 @@ class TestShellThread(threading.Thread): test_info: Object containing the test filename, uri and timeout Return: - A list of TestFailure objects describing the error. + A TestResult """ Blank line before """. Also, you're using a style here that's different from what you're using above "Returns: blah" versus "Return:\n blah". @@ -370,9 +373,9 @@ class TestShellThread(threading.Thread): worker.start() - # When we're running one test per DumpRenderTree process, we can enforce - # a hard timeout. the DumpRenderTree watchdog uses 2.5x the timeout - # We want to be larger than that. + # When we're running one test per DumpRenderTree process, we can + # enforce a hard timeout. the DumpRenderTree watchdog uses 2.5x Per PEP8, two spaces after each period. Also capitalize the first word of a sentence. + # the timeout We want to be larger than that. Does this sentence make sense? worker.join(int(test_info.timeout) * 3.0 / 1000.0) if worker.isAlive(): # If join() returned with the thread still running, the @@ -380,22 +383,21 @@ class TestShellThread(threading.Thread): # more we can do with it. We have to kill all the # DumpRenderTrees to free it up. If we're running more than # one DumpRenderTree thread, we'll end up killing the other - # DumpRenderTrees too, introducing spurious crashes. We accept that - # tradeoff in order to avoid losing the rest of this thread's - # results. + # DumpRenderTrees too, introducing spurious crashes. We accept This first sentence doesn't make sense and is not a complete sentence. + # that tradeoff in order to avoid losing the rest of this + # thread's results. _log.error('Test thread hung: killing all DumpRenderTrees') worker._driver.stop() try: - stats = worker.get_test_stats() - self._test_stats.append(stats) - failures = stats.failures + result = worker.get_test_result() except AttributeError, e: failures = [] _log.error('Cannot get results of test: %s' % test_info.filename) + result = TestResult(test_info.filename, [], 0, 0, 0) I would use a variable more descriptive than "e" -- for example "err". Also, is that variable even being used here? If not, just use "except AttributeError:". Finally, I would use named parameters so the reader knows what the zeros signify. - return failures + return result def _run_test(self, test_info): """Run a single test file using a shared DumpRenderTree process. @@ -419,19 +421,18 @@ class TestShellThread(threading.Thread): self._driver.run_test(test_info.uri, test_info.timeout, image_hash) end = time.time() - stats = process_output(self._port, test_info, self._test_types, + result = process_output(self._port, test_info, self._test_types, self._test_args, self._options.configuration, self._options.results_directory, crash, timeout, end - start, actual_checksum, output, error) Align correctly and use named parameters. - - self._test_stats.append(stats) - return stats.failures + self._test_results.append(result) + return result def _ensure_dump_render_tree_is_running(self): - """Start the shared DumpRenderTree, if it's not running. Not for use when - running tests singly, since those each start a separate DumpRenderTree in - their own thread. + """Start the shared DumpRenderTree, if it's not running. Not for use + when running tests singly, since those each start a separate + DumpRenderTree in their own thread. """ Blank line after first line. The second sentence is not a complete sentence. if (not self._driver or self._driver.poll() is not None): self._driver = self._port.start_driver( --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py @@ -48,6 +48,7 @@ _log = logging.getLogger("webkitpy.layout_tests.layout_package." # Test expectation file update action constants (NO_CHANGE, REMOVE_TEST, REMOVE_PLATFORM, ADD_PLATFORMS_EXCEPT_THIS) = range(4) + def result_was_expected(result, expected_results, test_needs_rebaselining, test_is_skipped): """Returns whether we got a result we were expecting. Use "command" form -- e.g. "Return whether...." @@ -61,11 +62,12 @@ def result_was_expected(result, expected_results, test_needs_rebaselining, if result in (IMAGE, TEXT, IMAGE_PLUS_TEXT) and FAIL in expected_results: return True if result == MISSING and test_needs_rebaselining: - return True + return True if result == SKIP and test_is_skipped: - return True + return True return False + def remove_pixel_failures(expected_results): """Returns a copy of the expected results for a test, except that we drop any pixel failures and return the remaining expectations. For example, @@ -141,12 +143,16 @@ class TestExpectations: retval = [] for expectation in expectations: - for item in TestExpectationsFile.EXPECTATIONS.items(): - if item[1] == expectation: - retval.append(item[0]) - break + retval.append(self.expectation_to_string(expectation)) + + return " ".join(retval) - return " ".join(retval).upper() + def expectation_to_string(self, expectation): + """Returns the uppercased string equivalent of a given expectation.""" Use the command form of sentence (I'm going to stop mentioning that one). + for item in TestExpectationsFile.EXPECTATIONS.items(): + if item[1] == expectation: + return item[0].upper() + return "" def get_timeline_for_test(self, test): return self._expected_failures.get_timeline_for_test(test) @@ -834,7 +840,8 @@ class TestExpectationsFile: if test in set_of_tests: set_of_tests.remove(test) - def _already_seen_test(self, test, test_list_path, lineno, allow_overrides): + def _already_seen_test(self, test, test_list_path, lineno, + allow_overrides): """Returns true if we've already seen a more precise path for this test than the test_list_path. """ First line on one line followed by a blank line (I'm going to stop mentioning this one as well). diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py index 059fd09..2aca3a7 100755 --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py @@ -81,8 +81,15 @@ _log = logging.getLogger("webkitpy.layout_tests.run_webkit_tests") # directory-by-directory feedback). LOG_DETAILED_PROGRESS = 'detailed-progress' +LOG_SUMMARY = 'summary' + +# "Trace" the test - log the expected result, the actual result, and the +# baselines used +LOG_TRACE = 'trace' + # Log any unexpected results while running (instead of just at the end). LOG_UNEXPECTED = 'unexpected' +LOG_UNEXPECTED_RESULTS = 'unexpected-results' I would recommend using the simple "enum idiom" in Python. It goes something like-- class LoggingMode(object): UNEXPECTED = 'unexpected' UNEXPECTED_RESULTS = 'unexpected-results" etc. Then you can do-- LoggingMode.UNEXPECTED, etc. It also eliminates the need to have a prefix as part of the constant name. # Builder base URL where we have the archived test results. BUILDER_BASE_URL = "http://build.chromium.org/buildbot/layout_test_results/" @@ -135,25 +142,23 @@ class ResultSummary(object): self.tests_by_timeline[timeline] = ( expectations.get_tests_with_timeline(timeline)) - def add(self, test, failures, result, expected): - """Add a result into the appropriate bin. + def add(self, result, expected): + """Add a TestResult into the appropriate bin. Args: - test: test file name - failures: list of failure objects from test execution - result: result of test (PASS, IMAGE, etc.). + result: TestResult from dump_render_tree_thread Be consistent with whether you use a period to end incomplete sentences in comments. expected: whether the result was what we expected it to be. """ @@ -374,8 +379,10 @@ class TestRunner: # subtracted out of self._test_files, above), but we stub out the # results here so the statistics can remain accurate. for test in skip_chunk: - result_summary.add(test, [], test_expectations.SKIP, - expected=True) + result = dump_render_tree_thread.TestResult(test, [], + 0, 0, 0) Use named parameters, as suggested above. def update_summary(self, result_summary): - """Update the summary while running tests.""" + """Update the summary and print results with any completed tests.""" while True: try: - (test, fail_list) = self._result_queue.get_nowait() - result = test_failures.determine_result_type(fail_list) - expected = self._expectations.matches_an_expected_result(test, - result, self._options.pixel_tests) - result_summary.add(test, fail_list, result, expected) - if (LOG_DETAILED_PROGRESS in self._options.log and - (self._options.experimental_fully_parallel or - self._is_single_threaded())): - self._display_detailed_progress(result_summary) - else: - if not expected and LOG_UNEXPECTED in self._options.log: - self._print_unexpected_test_result(test, result) - self._display_one_line_progress(result_summary) + result = self._result_queue.get_nowait() + expected = self._expectations.matches_an_expected_result( + result.filename, result.type, self._options.pixel_tests) + result_summary.add(result, expected) + self._print_test_results(result, expected, result_summary) except Queue.Empty: return Does the try block need to enclose that many lines? I would enclose the smallest number of lines possible. - def _display_one_line_progress(self, result_summary): + def _print_test_results(self, result, expected, result_summary): + "Print the result of the test as determined by the --log switches." + if LOG_TRACE in self._options.log: + self._print_test_trace(result) + elif (LOG_DETAILED_PROGRESS in self._options.log and + (self._options.experimental_fully_parallel or + self._is_single_threaded())): + self._print_detailed_progress(result_summary) + else: + if (not expected and LOG_UNEXPECTED in self._options.log): + self._print_unexpected_test_result(result) + self._print_one_line_progress(result_summary) + + def _print_test_trace(self, result): + """Print detailed results of a test (triggered by --log trace). + For each test, print: + - location of the expected baselines + - expected results + - actual result + - timing info + """ + filename = result.filename + test_name = self._port.relative_test_filename(filename) + _log.info('trace: %s' % test_name) + _log.info(' txt: %s' % + self._port.relative_test_filename( + self._port.expected_filename(filename, '.txt'))) I would break this up so you're not nesting so many function calls in one statement. @@ -1560,7 +1600,7 @@ def parse_args(args=None): help="log various types of data. The param should be a " + "comma-separated list of values from: " + "actual,config," + LOG_DETAILED_PROGRESS + - ",expected,timing," + LOG_UNEXPECTED + " " + + ",expected,timing,trace" + LOG_UNEXPECTED + " " + It might be clearer to use a format string here instead. Also consider using ",".join(). Also, it would be better to use a full word instead of a (non-standard) abbreviated form like "param". "(defaults to " + "--log detailed-progress,unexpected)"), Do you need to be concatenating here? (In reply to comment #10) > @@ -1560,7 +1600,7 @@ def parse_args(args=None): > help="log various types of data. The param should be a " + > "comma-separated list of values from: " + > "actual,config," + LOG_DETAILED_PROGRESS + > - ",expected,timing," + LOG_UNEXPECTED + " " + > + ",expected,timing,trace" + LOG_UNEXPECTED + " " + A couple more things. Per PEP8, the preferred way of wrapping long lines is by using Python's implied line continuation (e.g. by enclosing the string in parentheses). Also, for the future, you may want to take a look at the optparse documentation for supporting multiple-valued options (i.e. the "extend" action, which "will take multiple values in a single comma-delimited string"): http://docs.python.org/library/optparse.html#adding-new-actions This may also let you use the "choices" parameter when making an option, which automatically generates a nice error message if the user includes an unrecognized value: http://docs.python.org/library/optparse.html#optparse.Option.choices Created attachment 53638 [details]
update w/ feedback from cjerdonek
(In reply to comment #10) > (In reply to comment #6) > > Created an attachment (id=53578) [details] [details] > > Here are several comments to consider that are mostly stylistic. If possible, > I'd like to provide feedback on the substance of the patch after you give > consideration to these comments. > Thanks for the detailed feedback! I appreciate you spending the time. I find it's actually more useful to give substantial feedback first, that way you don't spend time reformatting code that ends up changing. Otherwise generally I agree with your comments. I'm less concerned with fanatical adherence to the commenting style of PEP-8, but I'll comply where I notice it. That said, it seemed like a number of your comments apply to changes not introduced by this file (e.g., the number of parameters to process_output). That feedback is entirely valid, but I'm not going to change it as part of this patch to avoid feature creep. > --- a/WebKitTools/ChangeLog > > + https://bugs.webkit.org/show_bug.cgi?id=37726 > + > + * Scripts/webkitpy/layout_tests/run_webkit_tests.py: > + - use the newly exposed TestResult class and implement > + --log trace > + * Scripts/webkitpy/layout_tests/layout_package/dump_render_thread.py: > + - rename TestStats to TestResult and make it more public, resulting > + in cleaner code > > It looks like the ChangeLog is missing an entry for-- > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py > True. > --- > a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py > > @@ -54,8 +54,8 @@ _log = > logging.getLogger("webkitpy.layout_tests.layout_package." > def process_output(port, test_info, test_types, test_args, configuration, > output_dir, crash, timeout, test_run_time, actual_checksum, > output, error): > - """Receives the output from a DumpRenderTree process, subjects it to a > number > - of tests, and returns a list of failure types the test produced. > + """Receives the output from a DumpRenderTree process, subjects it to a > + number of tests, and returns a list of failure types the test produced. > > Per PEP8, the first line of a docstring should fit on one line and be > followed by a blank line. Also per PEP8, the first line should be phrased > as a command, for example "Process DumpRenderTree output, and return...." > > (By the way, that parameter list is a bit long. It should probably be > refactored to have a much shorter parameter list.) > Agreed. I only reformatted the comment to fit within the 80-col limit. I will save more substantial changes for a different CL. > @@ -66,7 +66,7 @@ def process_output(port, test_info, test_types, test_args, > configuration, > configuration: Debug or Release > output_dir: directory to put crash stack traces into > > - Returns: a list of failure objects and times for the test being processed > + Returns: a TestResult object > """ > > Per PEP8, a blank line should separate the final line of a multi-line > docstring and the triple quotes. Really? That seems like a waste of space. Okay, learn something new ... > > @@ -111,16 +111,17 @@ def process_output(port, test_info, test_types, > test_args, configuration, > time.time() - start_diff_time) > > total_time_for_all_diffs = time.time() - start_diff_time > - return TestStats(test_info.filename, failures, test_run_time, > + return TestResult(test_info.filename, failures, test_run_time, > total_time_for_all_diffs, time_for_diffs) > > The second line should line up with the character following the left > parenthesis. Also, when calling a function with many parameters, it's good > to use named parameters. This is a good safety check (especially when used > in conjunction with unit tests) since it adds an additional > level of checking. > We frequently follow a slightly different convention where wrapped lines are indented four spaces instead of lining up with the parenthesis (see the Google Python Style Guide), if it helps readability. In this case, it's a bit of a toss-up, so I'll change the indentation. I think it is only necessary to use the parameter names if the variable names aren't clear. Granted, there is still a chance that you get variables in the wrong order, but I'll take that chance over writing things like total_time_for_all_diffs=total_time_for_all_diffs. > -class TestStats: > +class TestResult: > > Classes at the bottom of your class hierarchy should really inherit from > "object". See, for example-- > done. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Classes > > > def __init__(self, filename, failures, test_run_time, > total_time_for_all_diffs, time_for_diffs): > self.filename = filename > self.failures = failures > + self.type = test_failures.determine_result_type(failures) > self.test_run_time = test_run_time > self.total_time_for_all_diffs = total_time_for_all_diffs > self.time_for_diffs = time_for_diffs > > In constructors, it is good to alphabetize the lines that set data attributes > by data attribute name. I find that grouping by functionality can trump this, but this isn't one of those cases. Done. > > @@ -157,14 +158,14 @@ class SingleTestThread(threading.Thread): > driver.run_test(test_info.uri.strip(), test_info.timeout, > test_info.image_hash) > end = time.time() > - self._test_stats = process_output(self._port, > + self._test_result = process_output(self._port, > test_info, self._test_types, self._test_args, > self._configuration, self._output_dir, crash, timeout, end - > start, > actual_checksum, output, error) > driver.stop() > > Again, align with the left parenthesis if possible and use named parameters > for long parameter lists. This is a case where I'm not going to align with the left parenthesis - the function gets too vertically long. > > - def get_test_stats(self): > - return self._test_stats > + def get_test_result(self): > + return self._test_result > > > class TestShellThread(threading.Thread): > @@ -201,7 +202,7 @@ class TestShellThread(threading.Thread): > self._canceled = False > self._exception_info = None > self._directory_timing_stats = {} > - self._test_stats = [] > + self._test_results = [] > self._num_tests = 0 > self._start_time = 0 > self._stop_time = 0 > > Alphabetize by data attribute name. > this is probably a good thing to do but I"m not going to change it now to minimize patch creep. > @@ -218,10 +219,10 @@ class TestShellThread(threading.Thread): > (number of tests in that directory, time to run the tests)""" > return self._directory_timing_stats > > - def get_individual_test_stats(self): > - """Returns a list of (test_filename, time_to_run_test, > - total_time_for_all_diffs, time_for_diffs) tuples.""" > - return self._test_stats > + def get_test_results(self): > + """Returns the list of all tests run on this thread. Used to calculate > + per-thread statistics.""" > + return self._test_results > > Again, phrase the first line of a doc string as a command (e.g. "Return the > list..." rather than "Returns....") and restrict the first line to one line > followed by a blank line. > > def cancel(self): > """Set a flag telling this thread to quit.""" > @@ -317,27 +318,29 @@ class TestShellThread(threading.Thread): > batch_count += 1 > self._num_tests += 1 > if self._options.run_singly: > - failures = self._run_test_singly(test_info) > + result = self._run_test_singly(test_info) > else: > - failures = self._run_test(test_info) > + result = self._run_test(test_info) > > You may wish to use the ternary operator here. > I don't like the ternary operator. It can't be used to set a breakpoint on one half of the branch, and it doesn't show up in code coverage listing (usually). > filename = test_info.filename > tests_run_file.write(filename + "\n") > - if failures: > + if result.failures: > # Check and kill DumpRenderTree if we need too. > > too -> to > > - if len([1 for f in failures if > f.should_kill_dump_render_tree()]): > + if len([1 for f in result.failures > + if f.should_kill_dump_render_tree()]): > > It seems this construct can be simplified. For example, can you use the > filter() construct instead? The 1's don't really do anything. Also > fix the alignment. > Probably, but I didn't write this code and I'm going to leave it alone for this patch (outside of fixing the line length and indentation. As to fixing the alignment, what do you think it should be? Aligned to the parenthesis? The left bracket? > # Print the error message(s). > - error_str = '\n'.join([' ' + f.message() for f in failures]) > + error_str = '\n'.join([' ' + f.message() for > + f in result.failures]) > > Alignment. Ditto. > > _log.debug("%s %s failed:\n%s" % (self.getName(), > self._port.relative_test_filename(filename), > error_str)) > else: > _log.debug("%s %s passed" % (self.getName(), > self._port.relative_test_filename(filename))) > - self._result_queue.put((filename, failures)) > + self._result_queue.put(result) > > if batch_size > 0 and batch_count > batch_size: > # Bounce the shell and reset count. > @@ -358,7 +361,7 @@ class TestShellThread(threading.Thread): > test_info: Object containing the test filename, uri and timeout > > Return: > - A list of TestFailure objects describing the error. > + A TestResult > """ > > Blank line before """. Also, you're using a style here that's different > from what you're using above "Returns: blah" versus "Return:\n blah". > done. > @@ -370,9 +373,9 @@ class TestShellThread(threading.Thread): > > worker.start() > > - # When we're running one test per DumpRenderTree process, we can > enforce > - # a hard timeout. the DumpRenderTree watchdog uses 2.5x the timeout > - # We want to be larger than that. > + # When we're running one test per DumpRenderTree process, we can > + # enforce a hard timeout. the DumpRenderTree watchdog uses 2.5x > > Per PEP8, two spaces after each period. Also capitalize the first word > of a sentence. Gah. This (two spaces) is a dumb rule in PEP 8, but whatever. > > + # the timeout We want to be larger than that. > > Does this sentence make sense? > I didn't write it, just reformatted it, but I've cleaned it up now. > worker.join(int(test_info.timeout) * 3.0 / 1000.0) > if worker.isAlive(): > # If join() returned with the thread still running, the > @@ -380,22 +383,21 @@ class TestShellThread(threading.Thread): > # more we can do with it. We have to kill all the > # DumpRenderTrees to free it up. If we're running more than > # one DumpRenderTree thread, we'll end up killing the other > - # DumpRenderTrees too, introducing spurious crashes. We accept > that > - # tradeoff in order to avoid losing the rest of this thread's > - # results. > + # DumpRenderTrees too, introducing spurious crashes. We accept > > This first sentence doesn't make sense and is not a complete sentence. > I'm not sure why you think that. The whole paragraphic is grammatical to me. > + # that tradeoff in order to avoid losing the rest of this > + # thread's results. > _log.error('Test thread hung: killing all DumpRenderTrees') > worker._driver.stop() > > try: > - stats = worker.get_test_stats() > - self._test_stats.append(stats) > - failures = stats.failures > + result = worker.get_test_result() > except AttributeError, e: > failures = [] > _log.error('Cannot get results of test: %s' % > test_info.filename) > + result = TestResult(test_info.filename, [], 0, 0, 0) > > I would use a variable more descriptive than "e" -- for example "err". > Also, is that variable even being used here? If not, just use "except > AttributeError:". Finally, I would use named parameters so the reader > knows what the zeros signify. I'm not gonna change the catch; (a) I like short names, and (b) I didn't write or change this. I will use named parameters. > > > - return failures > + return result > > def _run_test(self, test_info): > """Run a single test file using a shared DumpRenderTree process. > @@ -419,19 +421,18 @@ class TestShellThread(threading.Thread): > self._driver.run_test(test_info.uri, test_info.timeout, image_hash) > end = time.time() > > - stats = process_output(self._port, test_info, self._test_types, > + result = process_output(self._port, test_info, self._test_types, > self._test_args, self._options.configuration, > self._options.results_directory, crash, > timeout, end - start, actual_checksum, > output, error) > > Align correctly and use named parameters. > Alignment fixed, but I'm not going to add named parameters. > - > - self._test_stats.append(stats) > - return stats.failures > + self._test_results.append(result) > + return result > > def _ensure_dump_render_tree_is_running(self): > - """Start the shared DumpRenderTree, if it's not running. Not for use > when > - running tests singly, since those each start a separate DumpRenderTree > in > - their own thread. > + """Start the shared DumpRenderTree, if it's not running. Not for use > + when running tests singly, since those each start a separate > + DumpRenderTree in their own thread. > """ > > Blank line after first line. The second sentence is not a complete sentence. > Fixed. > if (not self._driver or self._driver.poll() is not None): > self._driver = self._port.start_driver( > > --- > a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py > > @@ -48,6 +48,7 @@ _log = > logging.getLogger("webkitpy.layout_tests.layout_package." > # Test expectation file update action constants > (NO_CHANGE, REMOVE_TEST, REMOVE_PLATFORM, ADD_PLATFORMS_EXCEPT_THIS) = > range(4) > > + > def result_was_expected(result, expected_results, test_needs_rebaselining, > test_is_skipped): > """Returns whether we got a result we were expecting. > > Use "command" form -- e.g. "Return whether...." > I'm going to leave this alone since I didn't change this code (patch creep). > @@ -61,11 +62,12 @@ def result_was_expected(result, expected_results, > test_needs_rebaselining, > if result in (IMAGE, TEXT, IMAGE_PLUS_TEXT) and FAIL in expected_results: > return True > if result == MISSING and test_needs_rebaselining: > - return True > + return True > if result == SKIP and test_is_skipped: > - return True > + return True > return False > > + > def remove_pixel_failures(expected_results): > """Returns a copy of the expected results for a test, except that we > drop any pixel failures and return the remaining expectations. For > example, > @@ -141,12 +143,16 @@ class TestExpectations: > retval = [] > > for expectation in expectations: > - for item in TestExpectationsFile.EXPECTATIONS.items(): > - if item[1] == expectation: > - retval.append(item[0]) > - break > + retval.append(self.expectation_to_string(expectation)) > + > + return " ".join(retval) > > - return " ".join(retval).upper() > + def expectation_to_string(self, expectation): > + """Returns the uppercased string equivalent of a given expectation.""" > > Use the command form of sentence (I'm going to stop mentioning that one). > Done. > + for item in TestExpectationsFile.EXPECTATIONS.items(): > + if item[1] == expectation: > + return item[0].upper() > + return "" > > def get_timeline_for_test(self, test): > return self._expected_failures.get_timeline_for_test(test) > @@ -834,7 +840,8 @@ class TestExpectationsFile: > if test in set_of_tests: > set_of_tests.remove(test) > > - def _already_seen_test(self, test, test_list_path, lineno, > allow_overrides): > + def _already_seen_test(self, test, test_list_path, lineno, > + allow_overrides): > """Returns true if we've already seen a more precise path for this > test > than the test_list_path. > """ > > First line on one line followed by a blank line (I'm going to stop mentioning > this one as well). > Again, not going to change this in this patch. > diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py > b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py > index 059fd09..2aca3a7 100755 > --- a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py > +++ b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py > @@ -81,8 +81,15 @@ _log = > logging.getLogger("webkitpy.layout_tests.run_webkit_tests") > # directory-by-directory feedback). > LOG_DETAILED_PROGRESS = 'detailed-progress' > > +LOG_SUMMARY = 'summary' > + > +# "Trace" the test - log the expected result, the actual result, and the > +# baselines used > +LOG_TRACE = 'trace' > + > # Log any unexpected results while running (instead of just at the end). > LOG_UNEXPECTED = 'unexpected' > +LOG_UNEXPECTED_RESULTS = 'unexpected-results' > > I would recommend using the simple "enum idiom" in Python. It goes something > like-- > > class LoggingMode(object): > > UNEXPECTED = 'unexpected' > UNEXPECTED_RESULTS = 'unexpected-results" > etc. > > Then you can do-- > > LoggingMode.UNEXPECTED, etc. > > It also eliminates the need to have a prefix as part of the constant name. > Good suggestion. I'll do this when I move the code into a separate package. > # Builder base URL where we have the archived test results. > BUILDER_BASE_URL = "http://build.chromium.org/buildbot/layout_test_results/" > @@ -135,25 +142,23 @@ class ResultSummary(object): > self.tests_by_timeline[timeline] = ( > expectations.get_tests_with_timeline(timeline)) > > - def add(self, test, failures, result, expected): > - """Add a result into the appropriate bin. > + def add(self, result, expected): > + """Add a TestResult into the appropriate bin. > > Args: > - test: test file name > - failures: list of failure objects from test execution > - result: result of test (PASS, IMAGE, etc.). > + result: TestResult from dump_render_tree_thread > > Be consistent with whether you use a period to end incomplete sentences > in comments. > fixed. > expected: whether the result was what we expected it to be. > """ > > @@ -374,8 +379,10 @@ class TestRunner: > # subtracted out of self._test_files, above), but we stub out the > # results here so the statistics can remain accurate. > for test in skip_chunk: > - result_summary.add(test, [], test_expectations.SKIP, > - expected=True) > + result = dump_render_tree_thread.TestResult(test, [], > + 0, 0, 0) > > Use named parameters, as suggested above. > done. > def update_summary(self, result_summary): > - """Update the summary while running tests.""" > + """Update the summary and print results with any completed tests.""" > while True: > try: > - (test, fail_list) = self._result_queue.get_nowait() > - result = test_failures.determine_result_type(fail_list) > - expected = self._expectations.matches_an_expected_result(test, > - result, self._options.pixel_tests) > - result_summary.add(test, fail_list, result, expected) > - if (LOG_DETAILED_PROGRESS in self._options.log and > - (self._options.experimental_fully_parallel or > - self._is_single_threaded())): > - self._display_detailed_progress(result_summary) > - else: > - if not expected and LOG_UNEXPECTED in self._options.log: > - self._print_unexpected_test_result(test, result) > - self._display_one_line_progress(result_summary) > + result = self._result_queue.get_nowait() > + expected = self._expectations.matches_an_expected_result( > + result.filename, result.type, self._options.pixel_tests) > + result_summary.add(result, expected) > + self._print_test_results(result, expected, result_summary) > except Queue.Empty: > return > > Does the try block need to enclose that many lines? I would enclose the > smallest number of lines possible. > done. > - def _display_one_line_progress(self, result_summary): > + def _print_test_results(self, result, expected, result_summary): > + "Print the result of the test as determined by the --log switches." > + if LOG_TRACE in self._options.log: > + self._print_test_trace(result) > + elif (LOG_DETAILED_PROGRESS in self._options.log and > + (self._options.experimental_fully_parallel or > + self._is_single_threaded())): > + self._print_detailed_progress(result_summary) > + else: > + if (not expected and LOG_UNEXPECTED in self._options.log): > + self._print_unexpected_test_result(result) > + self._print_one_line_progress(result_summary) > + > + def _print_test_trace(self, result): > + """Print detailed results of a test (triggered by --log trace). > + For each test, print: > + - location of the expected baselines > + - expected results > + - actual result > + - timing info > + """ > + filename = result.filename > + test_name = self._port.relative_test_filename(filename) > + _log.info('trace: %s' % test_name) > + _log.info(' txt: %s' % > + self._port.relative_test_filename( > + self._port.expected_filename(filename, '.txt'))) > > I would break this up so you're not nesting so many function calls in > one statement. > I'm not sure how much better this can be. Example? > @@ -1560,7 +1600,7 @@ def parse_args(args=None): > help="log various types of data. The param should be a " + > "comma-separated list of values from: " + > "actual,config," + LOG_DETAILED_PROGRESS + > - ",expected,timing," + LOG_UNEXPECTED + " " + > + ",expected,timing,trace" + LOG_UNEXPECTED + " " + > > It might be clearer to use a format string here instead. Also consider > using ",".join(). Also, it would be better to use a full word instead of > a (non-standard) abbreviated form like "param". > Cleaned up somewhat differently. Take a look. > > "(defaults to " + "--log detailed-progress,unexpected)"), > > Do you need to be concatenating here? No. (In reply to comment #13) Thanks for taking my feedback into account. The patch is much easier to read now! (By the way, I apologize for including such large code excerpts in comment 10. I'll try to avoid doing that in the future.) > I find it's actually more useful to give substantial feedback first, that way > you don't spend time reformatting code that ends up changing. Yes, though at the same time good style at the outset makes it a bit easier to review and quickly see what's going on. Also, even if the specific suggestions don't wind up getting used in the current patch, the style feedback and discussion is often valuable for subsequent patches. If anything major had occurred to me, though, I would be sure to let you know. > That said, it seemed like a number of your comments apply to changes not > introduced by this file (e.g., the number of parameters to process_output). > That feedback is entirely valid, but I'm not going to change it as part of this > patch to avoid feature creep. That's fine. I was under the impression that you may have written some of the existing code, so I thought it wouldn't hurt to mention it. At the least you're actively working on the code, so it made sense to tell you. :) One thing you can always do in cases like these is add FIXME's to the nearby code. That way the suggestion will get preserved for the future, with little cost to you. > I think it is only necessary to use > the parameter names if the variable names aren't clear. Granted, there is still > a chance that you get variables in the wrong order, but I'll take that chance > over writing things like total_time_for_all_diffs=total_time_for_all_diffs. I've found that it helps most with longer lists of parameters (e.g. more than 4-5). It also has the nice effect of discouraging longer lists of parameters. :) > > In constructors, it is good to alphabetize the lines that set data attributes > > by data attribute name. > > I find that grouping by functionality can trump this, but this isn't one of > those cases. Done. Thanks. I should have qualified my suggestion, too. Personally, I prefer the attributes set from parameter values to be grouped separately from the ones with default (e.g. hard-coded) values. In any case, I think we both agree that alphabetizing within groups makes sense (no matter how we group). > > class TestShellThread(threading.Thread): > > @@ -201,7 +202,7 @@ class TestShellThread(threading.Thread): > > self._canceled = False > > self._exception_info = None > > self._directory_timing_stats = {} > > - self._test_stats = [] > > + self._test_results = [] > > self._num_tests = 0 > > self._start_time = 0 > > self._stop_time = 0 > > > > Alphabetize by data attribute name. > > > > this is probably a good thing to do but I"m not going to change it now to > minimize patch creep. One thing you can always do is insert just the new one into the appropriate order (i.e. as best can be done with the existing ordering). > > - if len([1 for f in failures if > > f.should_kill_dump_render_tree()]): > > + if len([1 for f in result.failures > > + if f.should_kill_dump_render_tree()]): > As to fixing the > alignment, what do you think it should be? Aligned to the parenthesis? The left > bracket? In webkitpy, we've been going with aligning to the innermost applicable symbol. So in this case, the "i" in "if" would be lined up underneath the 1 after the left bracket. I didn't know Google had a different alignment style. I had thought PEP8 spoke to this, but I could be wrong. > Gah. This (two spaces) is a dumb rule in PEP 8, but whatever. I didn't make them up. :) > > This first sentence doesn't make sense and is not a complete sentence. > > I'm not sure why you think that. The whole paragraphic is grammatical to me. That was my bad, sorry! > > + _log.info(' txt: %s' % > > + self._port.relative_test_filename( > > + self._port.expected_filename(filename, '.txt'))) > > > > I would break this up so you're not nesting so many function calls in > > one statement. > > I'm not sure how much better this can be. Example? I might have included too large of an excerpt. I simply meant breaking the statement up by, say, calculating the filename on a separate line. That way you won't have as many levels of nesting -- e.g. > + rel_test_filename = ... > + _log.info(' txt: %s' % rel_test_filename) > > It might be clearer to use a format string here instead. Also consider > > using ",".join(). Also, it would be better to use a full word instead of > > a (non-standard) abbreviated form like "param". > > > > Cleaned up somewhat differently. Take a look. Thanks. I'll take a look! (In reply to comment #12) > Created an attachment (id=53638) [details] Hmm, interesting. This seems to be a diff off the previous patch. Was that intentional? It makes it easier for me to see how you modified things in response to my comments, but probably harder for others to respond to your changes as a whole. (That also explains why the patch doesn't apply.) This looks pretty good style-wise now. A few straggling nits below. > + This is used to calculate per-thread statistics. > + """ Needs intervening blank line. > + a separate DumpRenderTree in their own thread. > """ Needs intervening blank line. > +LOG_VALUES = ",".join(("actual", "config", LOG_DETAILED_PROGRESS, "expected", > + LOG_SUMMARY, "timing", LOG_TRACE, LOG_UNEXPECTED, > + LOG_UNEXPECTED_RESULTS)) The L's should be lined up underneath the quote after the parenthesis. You might want to squash and reattach so a reviewer can take a look. Or did you split this report into sub-reports?? E.g. https://bugs.webkit.org/show_bug.cgi?id=37785 (In reply to comment #15) > (In reply to comment #12) > > Created an attachment (id=53638) [details] [details] > > Hmm, interesting. This seems to be a diff off the previous patch. Was that > intentional? It makes it easier for me to see how you modified things in > response to my comments, but probably harder for others to respond to your > changes as a whole. (That also explains why the patch doesn't apply.) > No, this wasn't intentional. I probably used the wrong diff command. > This looks pretty good style-wise now. A few straggling nits below. > > > + This is used to calculate per-thread statistics. > > + """ > > Needs intervening blank line. > Fixed. > > + a separate DumpRenderTree in their own thread. > > """ > > Needs intervening blank line. > Fixed. > > +LOG_VALUES = ",".join(("actual", "config", LOG_DETAILED_PROGRESS, "expected", > > + LOG_SUMMARY, "timing", LOG_TRACE, LOG_UNEXPECTED, > > + LOG_UNEXPECTED_RESULTS)) > > The L's should be lined up underneath the quote after the parenthesis. > Fixed. Revised (full) patch attached. Created attachment 53731 [details]
revise w/ more feedback from cjerdonek, fix delta to be against baseline
Comment on attachment 53731 [details]
revise w/ more feedback from cjerdonek, fix delta to be against baseline
Looks reasonable. Thanks for the clean up too.
Created attachment 53897 [details]
updated patch that will actually apply
Committed r57934: <http://trac.webkit.org/changeset/57934> I believe this broke a python unit test. ====================================================================== ERROR: test_fast (webkitpy.layout_tests.run_webkit_tests_unittest.MainTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 51, in test_fast 'fast/html'])) File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py", line 41, in passing_run res = run_webkit_tests.main(options, args, False) File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 1621, in main num_unexpected_results = test_runner.run(result_summary, print_results) File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 697, in run result_summary) File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 1053, in _print_timing_statistics self._print_aggregate_test_statistics(write, individual_test_timings) File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 1066, in _print_aggregate_test_statistics test_types = individual_test_timings[0].time_for_diffs.keys() IndexError: list index out of range ---------------------------------------------------------------------- Preparing a patch. Created attachment 53935 [details]
Attempt to fix test-webkitpy
Committed r57960: <http://trac.webkit.org/changeset/57960> Comment on attachment 53935 [details]
Attempt to fix test-webkitpy
Change looks safe.
I'm not sure why I didn't see the failed test. I'll have to look into it a bit more.
|