|Summary:||PatchAnalysisTask._test_patch() needs refactoring|
|Product:||WebKit||Reporter:||Jake Nielsen <jake.nielsen.webkit>|
|Component:||Tools / Tests||Assignee:||Jake Nielsen <jake.nielsen.webkit>|
|Severity:||Normal||CC:||ap, commit-queue, glenn|
|Version:||528+ (Nightly build)|
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 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.