PatchAnalysisTask._test_patch() needs refactoring

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jake Nielsen]

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.

[Jake Nielsen]

Created attachment 240236 [details]
Refactoring!

[Alexey Proskuryakov]

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.

[Jake Nielsen]

Created attachment 240285 [details]
Adds some comments, and fixes a bug

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jake Nielsen]

Created attachment 240298 [details]
Adds FIXMEs to tests, and sets the test expectations to agree with current behavior.

[Jake Nielsen]

Comment on attachment 240298 [details]
Adds FIXMEs to tests, and sets the test expectations to agree with current behavior.

Commented on the wrong bug..