Here’s the version of the unit test patch, incorporating the feedback I have received so far.
Some notes on the patch:
This patch doesn’t include googletest itself, that will need to be checked in separately. The source distribution will live in llvm/utils/unittest/googletest. (The reason for the extra directory level is so that the LLVM-specific makefiles can live in llvm/utils/unittest, while the googletest directory itself can be the pristine, unmodified distribution.)
The individual tests are in llvm/unittests (note plural).
The makefile target to build and run the tests is “make unittest” (I wasn’t sure if you could have a synthetic target that was the same as the name of a directory, so I made it “unittest” instead of “unittests”)
There’s a common Makefile and a common TestMain.cpp in llvm/unittests. The individual tests are in llvm/unittests/ where is the name of an LLVM library such as “ADT”. Each of these subdirs has a tiny makefile which sets TESTNAME= and then includes the common Makefile.unittest. Each subdir under unittests creates a separate executable. (I didn’t use the default googletest main because I figured at some point we might want to customize main().)
I updated the LICENSE.txt and mkpatch, but I haven’t done the HTML docs yet because I am still thinking about what to write.
I probably made some mistakes in setting up the makefile rules - that is what took the most time - so it will merit heightened scrutiny.
Here’s the version of the unit test patch, incorporating the feedback I have received so far.
Looks good, a few comments below.
Some notes on the patch:
This patch doesn’t include googletest itself, that will need to be checked in separately. The source distribution will live in llvm/utils/unittest/googletest. (The reason for the extra directory level is so that the LLVM-specific makefiles can live in llvm/utils/unittest, while the googletest directory itself can be the pristine, unmodified distribution.)
Are you going to send a separate patch for this, or should I do this?
The individual tests are in llvm/unittests (note plural).
The makefile target to build and run the tests is “make unittest” (I wasn’t sure if you could have a synthetic target that was the same as the name of a directory, so I made it “unittest” instead of “unittests”)
I don’t think that’s an issue – to recurse into a directory, you have to say “make -C dir [target]”.
There’s a common Makefile and a common TestMain.cpp in llvm/unittests. The individual tests are in llvm/unittests/ where is the name of an LLVM library such as “ADT”. Each of these subdirs has a tiny makefile which sets TESTNAME= and then includes the common Makefile.unittest. Each subdir under unittests creates a separate executable. (I didn’t use the default googletest main because I figured at some point we might want to customize main().)
I updated the LICENSE.txt and mkpatch, but I haven’t done the HTML docs yet because I am still thinking about what to write.
I probably made some mistakes in setting up the makefile rules - that is what took the most time - so it will merit heightened scrutiny.
Here’s the version of the unit test patch, incorporating the feedback I have received so far.
Looks good, a few comments below.
Some notes on the patch:
This patch doesn’t include googletest itself, that will need to be checked in separately. The source distribution will live in llvm/utils/unittest/googletest. (The reason for the extra directory level is so that the LLVM-specific makefiles can live in llvm/utils/unittest, while the googletest directory itself can be the pristine, unmodified distribution.)
Are you going to send a separate patch for this, or should I do this?
Please go ahead and do this. It seems silly (to me anyway) to convert the source tarball into a patch file.
The individual tests are in llvm/unittests (note plural).
The makefile target to build and run the tests is “make unittest” (I wasn’t sure if you could have a synthetic target that was the same as the name of a directory, so I made it “unittest” instead of “unittests”)
I don’t think that’s an issue – to recurse into a directory, you have to say “make -C dir [target]”.
There’s a common Makefile and a common TestMain.cpp in llvm/unittests. The individual tests are in llvm/unittests/ where is the name of an LLVM library such as “ADT”. Each of these subdirs has a tiny makefile which sets TESTNAME= and then includes the common Makefile.unittest. Each subdir under unittests creates a separate executable. (I didn’t use the default googletest main because I figured at some point we might want to customize main().)
I updated the LICENSE.txt and mkpatch, but I haven’t done the HTML docs yet because I am still thinking about what to write.
I probably made some mistakes in setting up the makefile rules - that is what took the most time - so it will merit heightened scrutiny.
That’s two dirs: One for the current directory, and one for the directory one level above. “…” won’t work here, so I have to create an absolute path so that it can find TestMain.cpp.
This patch doesn’t include googletest itself, that will need to be checked in separately. The source distribution will live in llvm/utils/unittest/googletest. (The reason for the extra directory level is so that the LLVM-specific makefiles can live in llvm/utils/unittest, while the googletest directory itself can be the pristine, unmodified distribution.)
Are you going to send a separate patch for this, or should I do this?
Please go ahead and do this. It seems silly (to me anyway) to convert the source tarball into a patch file.
Yes, I figured this out after trying to see what’s involved there. I’m thinking we don’t need to keep everything in the tarball, because it’ll just bloat the LLVM distribution. I think we should be fine with just the include/ and src/ directories, and I’ll keep the COPYING file since it’s the license, and add a README.LLVM file. Sound good?
The individual tests are in llvm/unittests (note plural).
The makefile target to build and run the tests is “make unittest” (I wasn’t sure if you could have a synthetic target that was the same as the name of a directory, so I made it “unittest” instead of “unittests”)
I don’t think that’s an issue – to recurse into a directory, you have to say “make -C dir [target]”.
So can we call the target to build and run unittests “unittests” or does it create problems?
+# 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.
...
+# 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.
This may be the result of my inability to completely figure out how to do certain things within the LLVM makefile framework. Specifically, I was trying to leave the googletest directory untouched, and since there's already a Makefile in that dir (which won't work for our purposes), I needed to build the gtest library using the makefile two directory levels up (in llvm/utils/unittest). Similarly, in the case of the Makefile.unittest, I was trying to include TestMain.cpp in the builds for all the subdirs.
In order to work with SRCDIR != OBJDIR, the files in SOURCES need to have relative paths so that the .o and .d files go in the right places in OBJDIR; But in order to find the sources, we have to give $wildcard an absolute path to SRCDIR. I see that in some places in LLVM, this is accomplished by stripping off the directory portion entirely from the output of $wildcard, however that only works for source files that are in the same directory as the makefile.
So messing with VPATH allowed me to use relative paths everywhere. I couldn't figure out how to do it without VPATH.
+# 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.
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()).
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.
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.
