Re: [PATCH] t1200: remove t1200-tutorial.sh
Johannes Schindelin writes: > Hi, > > On Wed, 9 Aug 2017, Stefan Beller wrote: > >> v1.2.0~121 (New tutorial, 2006-01-22) rewrote the tutorial such that the >> original intent of 2ae6c70674 (Adapt tutorial to cygwin and add test case, >> 2005-10-13) to test the examples from the tutorial doesn't hold any more. >> >> There are dedicated tests for the commands used, even "git whatchanged", >> such that removing these tests doesn't seem like a reduction in test >> coverage. >> >> Signed-off-by: Stefan Beller >> --- > > ACK, > Dscho Thanks, both. Will queue.
Re: [PATCH] t1200: remove t1200-tutorial.sh
Hi, On Wed, 9 Aug 2017, Stefan Beller wrote: > v1.2.0~121 (New tutorial, 2006-01-22) rewrote the tutorial such that the > original intent of 2ae6c70674 (Adapt tutorial to cygwin and add test case, > 2005-10-13) to test the examples from the tutorial doesn't hold any more. > > There are dedicated tests for the commands used, even "git whatchanged", > such that removing these tests doesn't seem like a reduction in test > coverage. > > Signed-off-by: Stefan Beller > --- ACK, Dscho
Re: [PATCH] t1200: remove t1200-tutorial.sh
Jonathan Nieder writes: > Correction: the tutorial is now called gitcore-tutorial and mostly > survives. A search for -p --root failed because of v1.5.5.1~19^2 > (core-tutorial.txt: Fix showing the current behaviour, 2008-04-10). Yeah, I was wondering why neither of you find it while reading your exchanges on a phone. > That said, the conclusion that this test has mostly bitrotted as far > as its original purpose goes still applies. > > An alternative method of addressing the goal you described would be to > fuzz object-id shaped things out of the sample output. I don't have a > strong preference, given how little this test contributes to coverage > (as you mentioned) and how difficult it is to make it continue to > match the documentation it is trying to test. > > Thanks and sorry for the confusion, OK, so can one of you give the final version of the patch with updated description? It _would_ be nice to have an executable tutorial/documentation or docs with built-in tests like some other systems have, but I realize that it would be a different matter. We'd need some toolchain to mark up tests in our tutorial, extract and run them, and compare the result, which we currently do not have and it won't fit under our test framework very well anyway. Thanks.
Re: [PATCH] t1200: remove t1200-tutorial.sh
Jonathan Nieder wrote: > Stefan Beller wrote: >>> Stefan Beller wrote: Nowadays there are better tutorials out there such as "Git from bottom up" or others, easily found online. Additionally to that a tutorial in our test suite is not as easy to discover as e.g. online tutorials. [...] >> 2ae6c70674 (Adapt tutorial to cygwin and add test case, 2005-10-13) >> seemed to imply that it was testing some part for Documentation/tutorial.txt >> though. > > Oh, good point. > > v1.2.0~121 (New tutorial, 2006-01-22) means that the test is no longer > testing what is in the tutorial in any meaningful sense. That's why > my search for "git whatchanged -p --root" in manpages didn't find > anything. Correction: the tutorial is now called gitcore-tutorial and mostly survives. A search for -p --root failed because of v1.5.5.1~19^2 (core-tutorial.txt: Fix showing the current behaviour, 2008-04-10). That said, the conclusion that this test has mostly bitrotted as far as its original purpose goes still applies. An alternative method of addressing the goal you described would be to fuzz object-id shaped things out of the sample output. I don't have a strong preference, given how little this test contributes to coverage (as you mentioned) and how difficult it is to make it continue to match the documentation it is trying to test. Thanks and sorry for the confusion, Jonathan
Re: [PATCH] t1200: remove t1200-tutorial.sh
Stefan Beller wrote: > On Tue, Aug 8, 2017 at 5:07 PM, Jonathan Nieder wrote: >> Stefan Beller wrote: >>> Nowadays there are better tutorials out there such as "Git from bottom up" >>> or others, easily found online. Additionally to that a tutorial in our >>> test suite is not as easy to discover as e.g. online tutorials. [...] >>> Signed-off-by: Stefan Beller >>> --- >>> t/t1200-tutorial.sh | 268 >>> >>> 1 file changed, 268 deletions(-) >>> delete mode 100755 t/t1200-tutorial.sh >> >> Interesting. When I first saw the diffstat I assumed you were talking >> about a test that validates the examples in some manpage are correct. >> But this is not that. [...] > 2ae6c70674 (Adapt tutorial to cygwin and add test case, 2005-10-13) > seemed to imply that it was testing some part for Documentation/tutorial.txt > though. Oh, good point. v1.2.0~121 (New tutorial, 2006-01-22) means that the test is no longer testing what is in the tutorial in any meaningful sense. That's why my search for "git whatchanged -p --root" in manpages didn't find anything. So what your patch does still seems reasonable (we have lived fine without a test validating the examples in that tutorial, and if we really want a test validating the examples then we should find a way to automatically extract it), but the description is misleading. With a corrected description, my Reviewed-by would apply. Thanks for catching it. Jonathan
Re: [PATCH] t1200: remove t1200-tutorial.sh
On Tue, Aug 8, 2017 at 5:07 PM, Jonathan Nieder wrote: > Hi, > > Stefan Beller wrote: > >> Nowadays there are better tutorials out there such as "Git from bottom up" >> or others, easily found online. Additionally to that a tutorial in our >> test suite is not as easy to discover as e.g. online tutorials. >> >> This test/tutorial was discovered by the patch author in the effort to >> migrate our tests in preparation to switch the hashing function. >> Transforming this tutorial to be agnostic of the underlying hash function >> would hurt its readability, hence being even less useful as a tutorial. >> >> Instead delete this test as >> (a) the functionality is tested elsewhere as well and >> (b) reducing the test suite to its core improves performance, which >> aids developers in keeping their development velocity. >> >> Signed-off-by: Stefan Beller >> --- >> t/t1200-tutorial.sh | 268 >> >> 1 file changed, 268 deletions(-) >> delete mode 100755 t/t1200-tutorial.sh > > Interesting. When I first saw the diffstat I assumed you were talking > about a test that validates the examples in some manpage are correct. > But this is not that. > > There indeed appear to be other good tests for these commands, even > "git whatchanged", so for what it's worth, > > Reviewed-by: Jonathan Nieder > > Thanks. 2ae6c70674 (Adapt tutorial to cygwin and add test case, 2005-10-13) seemed to imply that it was testing some part for Documentation/tutorial.txt though.
Re: [PATCH] t1200: remove t1200-tutorial.sh
Hi, Stefan Beller wrote: > Nowadays there are better tutorials out there such as "Git from bottom up" > or others, easily found online. Additionally to that a tutorial in our > test suite is not as easy to discover as e.g. online tutorials. > > This test/tutorial was discovered by the patch author in the effort to > migrate our tests in preparation to switch the hashing function. > Transforming this tutorial to be agnostic of the underlying hash function > would hurt its readability, hence being even less useful as a tutorial. > > Instead delete this test as > (a) the functionality is tested elsewhere as well and > (b) reducing the test suite to its core improves performance, which > aids developers in keeping their development velocity. > > Signed-off-by: Stefan Beller > --- > t/t1200-tutorial.sh | 268 > > 1 file changed, 268 deletions(-) > delete mode 100755 t/t1200-tutorial.sh Interesting. When I first saw the diffstat I assumed you were talking about a test that validates the examples in some manpage are correct. But this is not that. There indeed appear to be other good tests for these commands, even "git whatchanged", so for what it's worth, Reviewed-by: Jonathan Nieder Thanks.
