Bug 137904 - PatchAnalysisTask._test_patch() needs refactoring
Summary: PatchAnalysisTask._test_patch() needs refactoring
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jake Nielsen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-20 17:39 PDT by Jake Nielsen
Modified: 2014-10-22 15:39 PDT (History)
3 users (show)

See Also:


Attachments
Refactoring! (35.41 KB, patch)
2014-10-21 18:36 PDT, Jake Nielsen
no flags Details | Formatted Diff | Diff
Adds some comments, and fixes a bug (35.91 KB, patch)
2014-10-22 11:45 PDT, Jake Nielsen
no flags Details | Formatted Diff | Diff
Adds FIXMEs to tests, and sets the test expectations to agree with current behavior. (11.61 KB, patch)
2014-10-22 15:38 PDT, Jake Nielsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Nielsen 2014-10-20 17:39:48 PDT
As it stands, there are many special cases and mystifying helper objects.
The special cases should be organized more uniformly, and the mystifying helper objects should be removed.
Comment 1 Jake Nielsen 2014-10-21 18:36:25 PDT
Created attachment 240236 [details]
Refactoring!
Comment 2 Alexey Proskuryakov 2014-10-22 09:50:38 PDT
Comment on attachment 240236 [details]
Refactoring!

View in context: https://bugs.webkit.org/attachment.cgi?id=240236&action=review

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:191
> +        if (len(results.failing_tests()) - len(self._clean_tree_results.failing_tests())) <= 5:
> +            return False

I think that this code would benefit from somewhat more comments.

E.g., here we could explain that the clean tree has so many failures that we can't be certain if having a few more means that the patch introduces a regression.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:245
> -        return False
> +        return True

I became confused here. Perhaps the best path forward is to walk over the patch line by line in person - or maybe you can get a review from someone who really knows Python and can read it more easily.
Comment 3 Jake Nielsen 2014-10-22 11:45:09 PDT
Created attachment 240285 [details]
Adds some comments, and fixes a bug
Comment 4 WebKit Commit Bot 2014-10-22 13:04:36 PDT
Comment on attachment 240285 [details]
Adds some comments, and fixes a bug

Clearing flags on attachment: 240285

Committed r175063: <http://trac.webkit.org/changeset/175063>
Comment 5 WebKit Commit Bot 2014-10-22 13:04:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Jake Nielsen 2014-10-22 15:38:48 PDT
Created attachment 240298 [details]
Adds FIXMEs to tests, and sets the test expectations to agree with current behavior.
Comment 7 Jake Nielsen 2014-10-22 15:39:37 PDT
Comment on attachment 240298 [details]
Adds FIXMEs to tests, and sets the test expectations to agree with current behavior.

Commented on the wrong bug..