webkitpy: committers_unittest fuzzy matching is really slow
Created attachment 128105 [details] Patch
rniwa ... let me know what you think of this approach. I think I've convinced myself it's safe. Assuming you like it, I will rework the actual code so that instead of calling assert_fuzzy_match() over and over again, we just pass in a list of test cases (much like I did in https://bugs.webkit.org/show_bug.cgi?id=79159 ); I left things this way to minimize the diff so you could better understand that change.
We can probably do something similar for webkitpy.tool.steps.preparechangelog_unittest.PrepareChangeLogTest.test_fuzzy_reviewer_match as well (haven't really looked yet, but that test is also quite slow).
Comment on attachment 128105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128105&action=review > Tools/Scripts/webkitpy/common/config/committers_unittest.py:117 > + contributors_used_in_tests = [all_committers.contributor_by_name(c) for c in self._contributors if c] I don't think this change is safe. A big part of this test is so that we don't mismatch new contributors to alias of old contributors. e.g. darin shouldn't match Darin Fisher.
Comment on attachment 128105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128105&action=review >> Tools/Scripts/webkitpy/common/config/committers_unittest.py:117 >> + contributors_used_in_tests = [all_committers.contributor_by_name(c) for c in self._contributors if c] > > I don't think this change is safe. A big part of this test is so that we don't mismatch new contributors to alias of old contributors. e.g. darin shouldn't match Darin Fisher. If I understand you correctly, you want to ensure that 'Darin' will always match 'Darin Adler' and not 'Darin Fisher' as long as 'Darin Adler' as they are both still in the contributor list, right? And since 'Darin Fisher' is not actually listed as one of the tests, below, this case is not adequately covered by my optimization? Presumably we could address that by adding a self._fuzz_match('Darin Fisher', 'Darin Fisher', 0) test case, or have some other mechanism to include "interesting" names in the list to ensure we were covering them. That said, it seems then that we're actually testing two different things here (which is what I feared). The first, is correctness of the fuzzy-matching algorithm (regardless of the contents of committers.py). This could be done with presumably far fewer test cases than we have. The second is testing for specific resolutions based on the actual (current) contents of the list of contributors, right? It seems like, if that was the case, it would be a lot clearer to break those out into individual (and different) test cases to be clearer about that (and in that case we should probably list the disambiguation we're worried about). Does that make sense?
(In reply to comment #5) > (From update of attachment 128105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128105&action=review > > >> Tools/Scripts/webkitpy/common/config/committers_unittest.py:117 > >> + contributors_used_in_tests = [all_committers.contributor_by_name(c) for c in self._contributors if c] > > > > I don't think this change is safe. A big part of this test is so that we don't mismatch new contributors to alias of old contributors. e.g. darin shouldn't match Darin Fisher. > > If I understand you correctly, you want to ensure that 'Darin' will always match 'Darin Adler' and not 'Darin Fisher' as long as 'Darin Adler' as they are both still in the contributor list, right? And since 'Darin Fisher' is not actually listed as one of the tests, below, this case is not adequately covered by my optimization? Presumably we could address that by adding a self._fuzz_match('Darin Fisher', 'Darin Fisher', 0) test case, or have some other mechanism to include "interesting" names in the list to ensure we were covering them. Right. That sounds like a reasonable solution to me. > That said, it seems then that we're actually testing two different things here (which is what I feared). The first, is correctness of the fuzzy-matching algorithm (regardless of the contents of committers.py). This could be done with presumably far fewer test cases than we have. > > The second is testing for specific resolutions based on the actual (current) contents of the list of contributors, right? > > It seems like, if that was the case, it would be a lot clearer to break those out into individual (and different) test cases to be clearer about that (and in that case we should probably list the disambiguation we're worried about). Right. But testing that properly involves including all contributors in the list, no?
(In reply to comment #6) > > The second is testing for specific resolutions based on the actual (current) contents of the list of contributors, right? > > > > It seems like, if that was the case, it would be a lot clearer to break those out into individual (and different) test cases to be clearer about that (and in that case we should probably list the disambiguation we're worried about). > > Right. But testing that properly involves including all contributors in the list, no? Probably, although I might think about it more, just to see if I've missed something else. If I break things up into, say, one test per person, at least it'll parallelize nicely. We could leave the "algorithm is correct" tests as unit tests and make the "match this person" tests optional integration tests that could be skipped if you were in a hurry (but are run by default).
(In reply to comment #7) > Probably, although I might think about it more, just to see if I've missed something else. If I break things up into, say, one test per person, at least it'll parallelize nicely. We could leave the "algorithm is correct" tests as unit tests and make the "match this person" tests optional integration tests that could be skipped if you were in a hurry (but are run by default). "algorithm is correct" is synonymous to "match this person". I purposefully avoided white-box testing. Anyway, I'm fine with skipping the entire test by default since there's no significant user of this function at the moment (changelog parser uses but it's still in the development).
Created attachment 128795 [details] Patch
Comment on attachment 128795 [details] Patch So you're splitting these down so that we can more easily shard this test?
rniwa - please take another look? I've left the original logic alone but chunked the tests up so that most either test a specific name or take less than ~1/3 of a second (which is good enough for me for now since I can run them all in parallel. Also, we had a few duplicate tests that were slowing things down ... single threaded, the file now takes ~4.5 seconds; in parallel I can run it in under 1.
(In reply to comment #10) > (From update of attachment 128795 [details]) > So you're splitting these down so that we can more easily shard this test? From the ChangeLog: + Break the fuzzy matching tests into individual routines for + each contributor so that the intent is a little clearer, so + that it's easier to test individual names (and identify + duplicate tests), and so that we can eventually run + them in parallel. Yup :) We can't easily speed up each individual match as long as we want to run the match against the entire list of committers :( We could mock out lists of committers (or provide a shorter list), but that won't achieve the testing goal rniwa wants.
(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 128795 [details] [details]) > > So you're splitting these down so that we can more easily shard this test? > > From the ChangeLog: > > + Break the fuzzy matching tests into individual routines for > + each contributor so that the intent is a little clearer, so > + that it's easier to test individual names (and identify > + duplicate tests), and so that we can eventually run > + them in parallel. > > Yup :) We can't easily speed up each individual match as long as we want to run the match against the entire list of committers :( We could mock out lists of committers (or provide a shorter list), but that won't achieve the testing goal rniwa wants. I can definitely see how these should be viewed as integration tests then. :)
Committed r108864: <http://trac.webkit.org/changeset/108864>