Hi, On Fri, Nov 09, 2012 at 07:33:31PM -0500, Jeff King wrote: > On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote: > > > * fc/completion-test-simplification (2012-10-29) 2 commits > > > - completion: simplify __gitcomp test helper > > > - completion: refactor __gitcomp related tests > > > > > > Clean up completion tests. > > > > > > There were some comments on the list. > > > > > > Expecting a re-roll. > > > > The second patch I can re-roll, but the first patch needs some > > external input. My preference is that tests should also be simple and > > maintainable, SZEDER's preference is that tests are better being > > explicit and verbose (even if harder to maintain) to minimize possible > > issues in the tests. > > I think it is better to keep the tests simple and maintainable. Maintainable? There is nothing to maintain here. Users' completion scripts depend on __gitcomp(), so its behavior shouldn't be changed. It can only be extended by a fifth parameter or by quoting words when necessary, but these future changes must not alter the current behavior checked by these tests, therefore even then these tests must be left intact. Simple? Currently you only need to look at __gitcomp() and the test itself to understand what's going on. With this series you'll also need to look at test_gitcomp(), figure out what its parameters are supposed to mean, and possibly get puzzled on the way why __gitcomp() is now seemingly called with only one parameter. So, I don't see much benefit in this series (except the part to use print_comp instead of "change IFS && echo", but that's already done in this patch: http://article.gmane.org/gmane.comp.version-control.git/207927). OTOH, this series has some serious drawbacks. It makes debugging more difficult. While working on the quoting issues I managed to break completion tests many-many times lately. In normal tests I could add a few debugging instructions to the failed test to find out where the breakage lies, without affecting other tests. However, if the failed test uses the test_completion() helper, then I have to add debugging instructions to test_completion() itself, too. This is bad, because many tests use this helper function and are therefore affected by the debugging instructions, producing truckloads of output making it difficult to dig out the relevant parts, or, worse yet, causing breakages in other tests. With this series the same difficulties will come to __gitcomp() tests, too. It can also encourage writing bad tests, similar to those that managed to cram many test_completion() lines into a single tests, giving me headaches to figure out what went wrong this time. Best, Gábor -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html