WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
138229
Remove duplicate code from PatchAnalysisTask._test_patch and fix bug regarding incorrect call to PatchAnalysisTask.report_failure
https://bugs.webkit.org/show_bug.cgi?id=138229
Summary
Remove duplicate code from PatchAnalysisTask._test_patch and fix bug regardin...
Jake Nielsen
Reported
2014-10-30 14:16:27 PDT
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.
Attachments
Adds a function to condense the code paths.
(5.15 KB, patch)
2014-10-30 14:19 PDT
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Makes code more readable (hopefully?) as discussed in person
(5.58 KB, patch)
2014-10-30 16:24 PDT
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Addresses comments
(5.31 KB, patch)
2014-11-03 19:13 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Bug fix, and unit test addition.
(6.94 KB, patch)
2014-11-05 11:44 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Removes results_from_test_run_without_patch altogether.
(15.40 KB, patch)
2014-11-05 18:07 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Makes LayoutTestResults.test_results always return a list.
(16.40 KB, patch)
2014-11-06 13:56 PST
,
Jake Nielsen
dbates
: review+
dbates
: commit-queue-
Details
Formatted Diff
Diff
Changelog fix
(16.44 KB, patch)
2014-11-06 16:51 PST
,
Jake Nielsen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Jake Nielsen
Comment 1
2014-10-30 14:19:24 PDT
Created
attachment 240695
[details]
Adds a function to condense the code paths.
WebKit Commit Bot
Comment 2
2014-10-30 14:22:40 PDT
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.
Jake Nielsen
Comment 3
2014-10-30 16:24:03 PDT
Created
attachment 240713
[details]
Makes code more readable (hopefully?) as discussed in person
Jake Nielsen
Comment 4
2014-10-31 11:43:10 PDT
bump
Daniel Bates
Comment 5
2014-11-03 16:00:25 PST
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.
Jake Nielsen
Comment 6
2014-11-03 18:52:01 PST
(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.
Jake Nielsen
Comment 7
2014-11-03 19:13:59 PST
Created
attachment 240900
[details]
Addresses comments
Daniel Bates
Comment 8
2014-11-04 14:11:23 PST
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
>.
Jake Nielsen
Comment 9
2014-11-04 21:59:06 PST
(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.
Jake Nielsen
Comment 10
2014-11-05 11:44:14 PST
Created
attachment 241045
[details]
Bug fix, and unit test addition.
Daniel Bates
Comment 11
2014-11-05 15:04:43 PST
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.
Jake Nielsen
Comment 12
2014-11-05 17:35:22 PST
(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.
Jake Nielsen
Comment 13
2014-11-05 18:07:10 PST
Created
attachment 241078
[details]
Removes results_from_test_run_without_patch altogether.
Daniel Bates
Comment 14
2014-11-06 12:59:02 PST
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.
Jake Nielsen
Comment 15
2014-11-06 13:56:09 PST
Created
attachment 241131
[details]
Makes LayoutTestResults.test_results always return a list.
Daniel Bates
Comment 16
2014-11-06 16:31:58 PST
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.
Jake Nielsen
Comment 17
2014-11-06 16:49:25 PST
(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.
Jake Nielsen
Comment 18
2014-11-06 16:51:22 PST
Created
attachment 241145
[details]
Changelog fix
Daniel Bates
Comment 19
2014-11-06 17:25:34 PST
(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).
Daniel Bates
Comment 20
2014-11-06 17:47:50 PST
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.
Jake Nielsen
Comment 21
2014-11-06 18:12:11 PST
(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.
WebKit Commit Bot
Comment 22
2014-11-06 18:27:43 PST
Comment on
attachment 241145
[details]
Changelog fix Clearing flags on attachment: 241145 Committed
r175735
: <
http://trac.webkit.org/changeset/175735
>
WebKit Commit Bot
Comment 23
2014-11-06 18:27:49 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug