Hooray! I'm thinking that getting unit tests for the classes in ADT should be an early goal. I also wanted to mention a point about the general philosophy of unit testing, which is that the presence of such tests alters the calculation of risk when making changes to a code base. Programmers have various rules of thumb for estimating risk - for example, a change which affects a large number of lines of code is generally perceived to be riskier than a change which affects a small number of lines. But at the same time, some very large changes can carry almost no risk - such as changing the name of a symbol which occurs in thousands of places in the code - because the compiler will tell you if you made a mistake anywhere. What unit tests do is allow that same kind of assurance at the semantic level that the compiler's syntax checking gives you at the syntactical level. And since reduction of risk in one area allows taking greater risks elsewhere, it can act as an enabler for global refactorings and other radical improvements that would have been perceived as too risky otherwise. This ability of unit tests to reduce perceived risk works best if the tests are comprehensive, testing even trivial aspects of the target class (such as brand new container correctly reporting empty()). -- Talin Misha Brukman wrote:> 2008/12/31 Chris Lattner <clattner at apple.com> > > >> On Dec 30, 2008, at 2:54 PM, Talin wrote: >> >> OK changes made and new patch attached. >> >> +++ utils/unittest/Makefile (revision 0) >> >> ... >> +# This has to come after Makefile.common, since it doesn't allow us to >> +# override the VPATH value unless we set PROJECT_NAME, which we don't want >> +# to do. >> +VPATH = $(LLVM_SRC_ROOT)/utils/unittest/googletest/src/ >> >> >> Why play with VPATH here? What is this doing? Can this be handled a >> different way? Similarly in makefile.unittest. >> >> > > Removed VPATH in this file by moving GTest up one directory, and added > VPATH-less Makefiles: > http://llvm.org/viewvc/llvm-project?view=rev&revision=61539 > http://llvm.org/viewvc/llvm-project?view=rev&revision=61540 (minor revision) > > Also removed VPATH from Makefile.unittest (see link below). > > +++ unittests/ADT/DenseMapTest.cpp (revision 0) > >> +namespace { >> + >> +using namespace llvm; >> >> >> Please put the 'using' right after the #includes. >> >> > > Done. > > > >> Otherwise, the patch looks good to me. Please verify that it works with >> both a SRCDIR=OBJDIR and SRCDIR!=OBJDIR build. >> >> > > Done for both; passes both with flying colors. > Submitted: http://llvm.org/viewvc/llvm-project?view=rev&revision=61541 > > Thanks to Talin for the original patch. > > Now everyone can go write some unittests! > > Misha > > P.S. Why are you still reading? Go! Write! Test! > > > ------------------------------------------------------------------------ > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
On Jan 1, 2009, at 1:21 AM, Talin wrote:> Hooray! > > I'm thinking that getting unit tests for the classes in ADT should > be an > early goal. >Definitely! This is an area of "low hanging fruit".> I also wanted to mention a point about the general philosophy of unit > testing, which is that the presence of such tests alters the > calculation > of risk when making changes to a code base. Programmers have various > rules of thumb for estimating risk - for example, a change which > affects > a large number of lines of code is generally perceived to be riskier > than a change which affects a small number of lines. But at the same > time, some very large changes can carry almost no risk - such as > changing the name of a symbol which occurs in thousands of places in > the > code - because the compiler will tell you if you made a mistake > anywhere. What unit tests do is allow that same kind of assurance at > the > semantic level that the compiler's syntax checking gives you at the > syntactical level. And since reduction of risk in one area allows > taking > greater risks elsewhere, it can act as an enabler for global > refactorings and other radical improvements that would have been > perceived as too risky otherwise. > > This ability of unit tests to reduce perceived risk works best if the > tests are comprehensive, testing even trivial aspects of the target > class (such as brand new container correctly reporting empty()). >I completely agree with all you said here. LLVM has been lacking in "quality assurance" for a long time. Only recently have we've set up buildbot systems to make sure that incremental changes won't break obvious things. The unit testing will make sure that invariant assumptions about the code will remain after each change. Those people who maintain buildbot systems should add the unit tests as part of the testing process. Thanks for spearheading this, Talin! -bw
On Jan 1, 2009, at 1:21 AM, Talin wrote:> Hooray! > > I'm thinking that getting unit tests for the classes in ADT should > be an > early goal.Can I suggest the AP[S]Int class? If you look in llvm/test/Integer you'll see a whole bunch of tests that really want to be unit tests but that aren't. :( -Chris
2009/1/1 Chris Lattner <clattner at apple.com>> On Jan 1, 2009, at 1:21 AM, Talin wrote: > > I'm thinking that getting unit tests for the classes in ADT should > > be an early goal. > > Can I suggest the AP[S]Int class? If you look in llvm/test/Integer > you'll see a whole bunch of tests that really want to be unit tests > but that aren't. :(Chris, do you mean *all* the .ll files in that dir should become unittests, or are you talking about some specific files that should be unittests, and others that should stay as llvm-as/llvm-dis tests? I'm assuming a##.* are prime candidates, what about the others? Are the double llvm-as/llvm-dis tests about bitcode printing/parsing or are they really testing some auto-simplification built-in to the APSInt class? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090105/abe1854f/attachment.html>