Both inside and after this line: if self._results_failed_different_tests(first_results, second_results): there is fairly similar logic to determine whether the failing tests are present without the patch applied. They should be condensed into one code path somehow.
Created attachment 240695 [details] Adds a function to condense the code paths.
Attachment 240695 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:261: trailing whitespace [pep8/W291] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240713 [details] Makes code more readable (hopefully?) as discussed in person
bump
Comment on attachment 240713 [details] Makes code more readable (hopefully?) as discussed in person View in context: https://bugs.webkit.org/attachment.cgi?id=240713&action=review > Tools/ChangeLog:4 > + Remove wetness from PatchAnalysisTask, in particular the calls to > + _build_and_test_without_patch() and the logic that follows I take you meant to write WETness instead of wetness. Maybe a better title would be: Extract code to build and test without patch from PatchAnalysisTask._test_patch into a common function Or Remove duplicate code from PatchAnalysisTask._test_patch ? > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:199 > + def _should_defer_patch_or_throw(self, failures, results_archive, script_error, failure_id): The name of this function, in particular, the suffix "or throw" seems ambiguous. Maybe a more descriptive name would be _should_defer_patch_or_report_failure? Maybe failures_with_patch would be a more descriptive name for the parameter failures? Maybe results_archive_for_failures_with_patch would be a more descriptive name for the parameter results_archive. > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:204 > + # If the clean tree exceeded the failure limit, then we can't know > + # whether the failures we saw are expected, so we must defer. This sentence doesn't read well. In particular, the first clause, "If the clean tree", does not read well and I inferred it to be a restatement of the code. For your consideration, I suggest we omit the first clause and move the comment to be inside the body of the if-statement such that it reads: if self._clean_tree_results.did_exceed_test_failure_limit(): # We cannot know whether the failures we saw in the test runs with the patch are expected. return True > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:209 > + # If there were failures that appeared consistently without the patch, but > + # they were not present without the patch, then we must report the failure, and This sentence does not read well. > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:261 > + # Returning false causes the patch to be defered. > + return False This is OK as-is. By the way, is defer the right terminology here? I would have used an inline comment and written this as: return False # Defer patch We may want to consider adding an analogous inline comment to each return True and return False statement in this function. We don't need to do this in this patch. > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:268 > return False For consistency, I suggest adding a similar comment as on line 260.
(In reply to comment #5) > Comment on attachment 240713 [details] > Makes code more readable (hopefully?) as discussed in person > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240713&action=review > > > Tools/ChangeLog:4 > > + Remove wetness from PatchAnalysisTask, in particular the calls to > > + _build_and_test_without_patch() and the logic that follows > > I take you meant to write WETness instead of wetness. Maybe a better title > would be: > > Extract code to build and test without patch from > PatchAnalysisTask._test_patch into a common function > > Or > > Remove duplicate code from PatchAnalysisTask._test_patch > > ? I'm liking your second suggestion. > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:199 > > + def _should_defer_patch_or_throw(self, failures, results_archive, script_error, failure_id): > > The name of this function, in particular, the suffix "or throw" seems > ambiguous. Maybe a more descriptive name would be > _should_defer_patch_or_report_failure? I like that the suffix "or_throw" speaks to the direct result. the "report_failure" suffix doesn't necessarily imply that the function might throw until the reader looks up the report_failure function. > > Maybe failures_with_patch would be a more descriptive name for the parameter > failures? > Maybe results_archive_for_failures_with_patch would be a more descriptive > name for the parameter results_archive. Agreed. > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:204 > > + # If the clean tree exceeded the failure limit, then we can't know > > + # whether the failures we saw are expected, so we must defer. > > This sentence doesn't read well. In particular, the first clause, "If the > clean tree", does not read well and I inferred it to be a restatement of the > code. For your consideration, I suggest we omit the first clause and move > the comment to be inside the body of the if-statement such that it reads: > > if self._clean_tree_results.did_exceed_test_failure_limit(): > # We cannot know whether the failures we saw in the test runs with the > patch are expected. > return True Agreed. > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:209 > > + # If there were failures that appeared consistently without the patch, but > > + # they were not present without the patch, then we must report the failure, and > > This sentence does not read well. Yeah, I think that's actually a mistype on my part. > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:261 > > + # Returning false causes the patch to be defered. > > + return False > > This is OK as-is. By the way, is defer the right terminology here? I would > have used an inline comment and written this as: I think defer is the right way to put it. In this case the patch will be re-evaluated at a later time, AKA deferred. > > return False # Defer patch > > We may want to consider adding an analogous inline comment to each return > True and return False statement in this function. We don't need to do this > in this patch. That would definitely help the readability. > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:268 > > return False > > For consistency, I suggest adding a similar comment as on line 260.
Created attachment 240900 [details] Addresses comments
Comment on attachment 240900 [details] Addresses comments View in context: https://bugs.webkit.org/attachment.cgi?id=240900&action=review > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:211 > + failures_introduced_by_patch = frozenset(failures_with_patch) - frozenset(self._clean_tree_results.failing_test_results()) > + if failures_introduced_by_patch: > + self.failure_status_id = failure_id > + # report_failure will either throw or return false. > + return not self.report_failure(results_archive_for_failures_with_patch, failures_introduced_by_patch, script_error) As far as I can tell, PatchAnalysisTask.report_failure() expects to be passed a LayoutTestResults object as its second argument (*). (*) Notice that PatchAnalysisTask.report_failure() assigns failures_introduced_by_patch to PatchAnalysisTask._results_from_patch_test_run (whose public getter is PatchAnalysisTask.results_from_patch_test_run()), EarlyWarningSystemTask is a derived class of PatchAnalysisTask and EarlyWarningSystemTask does not override PatchAnalysisTask.results_from_patch_test_run(). From looking at the implementation of AbstractEarlyWarningSystem._failing_tests_message(), AbstractEarlyWarningSystem expects that PatchAnalysisTask.results_from_patch_test_run() returns a LayoutTestResults object or at least an object that implements a function of the form LayoutTestResults.failing_tests() by <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py?rev=175063#L65>.
(In reply to comment #8) > Comment on attachment 240900 [details] > Addresses comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240900&action=review > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:211 > > + failures_introduced_by_patch = frozenset(failures_with_patch) - frozenset(self._clean_tree_results.failing_test_results()) > > + if failures_introduced_by_patch: > > + self.failure_status_id = failure_id > > + # report_failure will either throw or return false. > > + return not self.report_failure(results_archive_for_failures_with_patch, failures_introduced_by_patch, script_error) > > As far as I can tell, PatchAnalysisTask.report_failure() expects to be > passed a LayoutTestResults object as its second argument (*). > > (*) Notice that PatchAnalysisTask.report_failure() assigns > failures_introduced_by_patch to > PatchAnalysisTask._results_from_patch_test_run (whose public getter is > PatchAnalysisTask.results_from_patch_test_run()), EarlyWarningSystemTask is > a derived class of PatchAnalysisTask and EarlyWarningSystemTask does not > override PatchAnalysisTask.results_from_patch_test_run(). From looking at > the implementation of AbstractEarlyWarningSystem._failing_tests_message(), > AbstractEarlyWarningSystem expects that > PatchAnalysisTask.results_from_patch_test_run() returns a LayoutTestResults > object or at least an object that implements a function of the form > LayoutTestResults.failing_tests() by > <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/commands/ > earlywarningsystem.py?rev=175063#L65>. Wow, good catch. I'll take a look at this tomorrow morning.
Created attachment 241045 [details] Bug fix, and unit test addition.
Comment on attachment 241045 [details] Bug fix, and unit test addition. View in context: https://bugs.webkit.org/attachment.cgi?id=241045&action=review > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:211 > + return not self.report_failure(results_archive_for_failures_with_patch, LayoutTestResults(failures_introduced_by_patch, did_exceed_test_failure_limit=False), script_error) Are you planning to remove the logic in {AbstractEarlyWarningSystem, CommitQueue}._failing_tests_message() for computing the equivalent of failures_introduced_by_patch? As far as I can tell the logic in the aforementioned functions makes it unnecessary to compute failures_introduced_by_patch in this function.
(In reply to comment #11) > Comment on attachment 241045 [details] > Bug fix, and unit test addition. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241045&action=review > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:211 > > + return not self.report_failure(results_archive_for_failures_with_patch, LayoutTestResults(failures_introduced_by_patch, did_exceed_test_failure_limit=False), script_error) > > Are you planning to remove the logic in {AbstractEarlyWarningSystem, > CommitQueue}._failing_tests_message() for computing the equivalent of > failures_introduced_by_patch? As far as I can tell the logic in the > aforementioned functions makes it unnecessary to compute > failures_introduced_by_patch in this function. That's a good point. I'll remove it now.
Created attachment 241078 [details] Removes results_from_test_run_without_patch altogether.
Comment on attachment 241078 [details] Removes results_from_test_run_without_patch altogether. View in context: https://bugs.webkit.org/attachment.cgi?id=241078&action=review > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:209 > + return not self.report_failure(results_archive_for_failures_with_patch, LayoutTestResults(failures_introduced_by_patch, did_exceed_test_failure_limit=False), script_error) Is it acceptable to pass a frozenset (failures_introduced_by_patch) to LayoutTestResults? Better question, is it acceptable that LayoutTestResults.test_results() returns an arbitrary data type? Notice that LayoutTestResults() assigns its first argument to LayoutTestResults._test_results (whose public getter is LayoutTestResults.test_results()). From briefly searching the code, every caller that instantiates a LayoutTestResults passes a list data type. Although I could not find a caller that depends on LayoutTestResults.test_results() returning a Python list or list-like data type, it seems weird that LayoutTestResults.test_results() may return an arbitrary data type. With respect to this bug, it seems weird that some callers of LayoutTestResults.test_results() may get a frozenset and some callers may get a list depending on the data type of the first argument passed to the LayoutTestResults constructor.
Created attachment 241131 [details] Makes LayoutTestResults.test_results always return a list.
Comment on attachment 241131 [details] Makes LayoutTestResults.test_results always return a list. View in context: https://bugs.webkit.org/attachment.cgi?id=241131&action=review > Tools/ChangeLog:7 > + Remove duplicate code from PatchAnalysisTask._test_patch. > + https://bugs.webkit.org/show_bug.cgi?id=138229 > + > + Reviewed by NOBODY (OOPS!). > + This patch doesn't just remove duplicate code. It also fixes a correctness issue where the EWS and CommitQueue were being passed a non LayoutTestResults object as implied by my remark in comment #8. I suggest that we add a remark to the change log entry and/or update the bug title to make it clear to a reader that this patch both fixes a correctness issue (which we added a unit test in file Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py) and removes duplicate/unnecessary code. > Tools/ChangeLog:37 > + (MockCommitQueueTask.results_from_patch_test_run): Nit: You didn't change this function. I take it that prepareChangeLog inadvertently thought you changed this method? > Tools/Scripts/webkitpy/common/net/layouttestresults.py:65 > - return self._test_results > + return [result for result in self._test_results] This is OK as-is. We could be smarter and only convert self._test_results to a list if necessary (overwriting self._test_results with the conversion to make subsequent calls to test_results() return the cached result). Alternatively, we could have added a assert in the constructor ensure self._test_results is a list-like data structure and modified _should_defer_patch_or_throw() to pass a list. Even better, remove this need for this getter entirely (since it exposes the implementation details of this class) by exposing a function(s) similar to the other member functions (e.g. failing_tests()) that filters self._test_results for the passing tests/results and other non-failing tests/results (if there are other test/result types other than passing and failing tests). > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:55 > + self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nfoo.html\nbar.html") Nit: You may want to consider sorting the failing tests for aesthetics. It's not necessary for landing this patch.
(In reply to comment #16) > Comment on attachment 241131 [details] > Makes LayoutTestResults.test_results always return a list. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241131&action=review > > > Tools/ChangeLog:7 > > + Remove duplicate code from PatchAnalysisTask._test_patch. > > + https://bugs.webkit.org/show_bug.cgi?id=138229 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > This patch doesn't just remove duplicate code. It also fixes a correctness > issue where the EWS and CommitQueue were being passed a non > LayoutTestResults object as implied by my remark in comment #8. I suggest > that we add a remark to the change log entry and/or update the bug title to > make it clear to a reader that this patch both fixes a correctness issue > (which we added a unit test in file > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py) and removes > duplicate/unnecessary code. I'll make the appropriate changes. > > > Tools/ChangeLog:37 > > + (MockCommitQueueTask.results_from_patch_test_run): > > Nit: You didn't change this function. I take it that prepareChangeLog > inadvertently thought you changed this method? yeah, prepareChangeLog decided that I did. I'll just remove it manually. > > > Tools/Scripts/webkitpy/common/net/layouttestresults.py:65 > > - return self._test_results > > + return [result for result in self._test_results] > > This is OK as-is. We could be smarter and only convert self._test_results to > a list if necessary (overwriting self._test_results with the conversion to > make subsequent calls to test_results() return the cached result). > Alternatively, we could have added a assert in the constructor ensure > self._test_results is a list-like data structure and modified > _should_defer_patch_or_throw() to pass a list. Even better, remove this need > for this getter entirely (since it exposes the implementation details of > this class) by exposing a function(s) similar to the other member functions > (e.g. failing_tests()) that filters self._test_results for the passing > tests/results and other non-failing tests/results (if there are other > test/result types other than passing and failing tests). Let's get this patch submitted. It's been flapping in the wind for way too long. > > > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:55 > > + self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nfoo.html\nbar.html") > > Nit: You may want to consider sorting the failing tests for aesthetics. It's > not necessary for landing this patch.
Created attachment 241145 [details] Changelog fix
(In reply to comment #17) > > > Tools/ChangeLog:37 > > > + (MockCommitQueueTask.results_from_patch_test_run): > > > > Nit: You didn't change this function. I take it that prepareChangeLog > > inadvertently thought you changed this method? > yeah, prepareChangeLog decided that I did. I'll just remove it manually. > > We should file a bug about this issue (if there isn't one already).
Comment on attachment 241145 [details] Changelog fix View in context: https://bugs.webkit.org/attachment.cgi?id=241145&action=review > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:192 > - if (len(results.failing_tests()) - len(self._clean_tree_results.failing_tests())) <= 5: > + if (len(results.failing_tests()) - len(self._delegate.test_results().failing_tests())) <= 5: Does it ever make sense to call this function when results.did_exceed_test_failure_limit() is false? If not then I suggest that we assert this condition. We can make such a change in a separate bug/patch.. > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:55 > + self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nfoo.html\nbar.html") I take it you disagreed with my remark (comment #16) about sorting the results since you didn't update this portion of the patch.
(In reply to comment #20) > Comment on attachment 241145 [details] > Changelog fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241145&action=review > > > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:192 > > - if (len(results.failing_tests()) - len(self._clean_tree_results.failing_tests())) <= 5: > > + if (len(results.failing_tests()) - len(self._delegate.test_results().failing_tests())) <= 5: > > Does it ever make sense to call this function when > results.did_exceed_test_failure_limit() is false? If not then I suggest that > we assert this condition. We can make such a change in a separate bug/patch.. As the name suggests, results must have exceeded the failure limit. > > > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:55 > > + self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nfoo.html\nbar.html") > > I take it you disagreed with my remark (comment #16) about sorting the > results since you didn't update this portion of the patch. I tend to agree with you. I just thought you wanted it in another patch.
Comment on attachment 241145 [details] Changelog fix Clearing flags on attachment: 241145 Committed r175735: <http://trac.webkit.org/changeset/175735>
All reviewed patches have been landed. Closing bug.