(Forwarding this to llvm-dev) This patch adds a unit test framework to LLVM, along with a sample unit test for DenseMap. I don't expect this patch to be accepted as-is, this is mainly a trial balloon and proof of concept. Some notes about the patch: 1) For the testing framework, I went with Google Test, since it's the one I have the most experience with. I fully expect an extended bikeshed discussion to result from this. 2) Both the test framework and the tests are optional build targets, they will not be built with the normal "make all". To build and run the unit tests, use "make check-unit". 3) I did not actually include the testing framework in the patch; It will need to be checked in separately. There are two approaches to this. One approach is to use the svn:external feature to create a link to the googletest svn repository from the LLVM svn repository. The other approach is to take a snapshot of googletest and check it in to the LLVM repository. The GoogleTest tar archive is here: http://code.google.com/p/googletest/downloads/list. I've located it within the LLVM source tree in the location "third-party/googletest". 4) I've included makefiles for building the test framework and unit tests. I had to do a bit of makefile hacking to get this to work, suggestions on how to improve this are welcome. -- -- Talin -- -- Talin -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081222/963953be/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: unittest.patch Type: application/octet-stream Size: 7599 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081222/963953be/attachment.obj>
Nice work! On Mon, Dec 22, 2008 at 4:34 PM, Talin <viridia at gmail.com> wrote:> (Forwarding this to llvm-dev) > > This patch adds a unit test framework to LLVM, along with a sample unit > test for DenseMap. I don't expect this patch to be accepted as-is, this is > mainly a trial balloon and proof of concept. > Some notes about the patch: > > 1) For the testing framework, I went with Google Test, since it's the one I > have the most experience with. I fully expect an extended bikeshed > discussion to result from this. >+1. Google test is pretty nice (not that I'm unbiased, as I am a gtest contributor :)> 2) Both the test framework and the tests are optional build targets, they > will not be built with the normal "make all". To build and run the unit > tests, use "make check-unit". >"make unittest"? I would never guess check-unit.> > 3) I did not actually include the testing framework in the patch; It will > need to be checked in separately. There are two approaches to this. One > approach is to use the svn:external feature to create a link to the > googletest svn repository from the LLVM svn repository. The other approach > is to take a snapshot of googletest and check it in to the LLVM repository. > > The GoogleTest tar archive is here: > http://code.google.com/p/googletest/downloads/list. I've located it within > the LLVM source tree in the location "third-party/googletest". >I suggest checking a specific version of Google test into the source. There isn't much clear benfit to tracking upstream gtest. On the other hand, there is benefit to not having mysterious breakages due to upstream (as unlikely as that is due to gtest API guarentees). Also, for projects adding the gtest source directly to their tree, there is a special gtest-all.c which contains the entire gtest codebase in a single file (it's not that much code). Keir> > 4) I've included makefiles for building the test framework and unit tests. > I had to do a bit of makefile hacking to get this to work, suggestions on > how to improve this are welcome. > > -- > -- Talin > > > > -- > -- Talin > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081222/457007f0/attachment.html>
So, any response on whether this patch is likely to be accepted? I'm willing to substantially rework things if requested. I should note that part of my motivation for doing this is that I have been wanting to do some work on the various LLVM container classes; However, I strongly feel that without some kind of unit test framework in place such work would mostly be a waste of time. Talin wrote:> (Forwarding this to llvm-dev) > > This patch adds a unit test framework to LLVM, along with a sample unit test > for DenseMap. I don't expect this patch to be accepted as-is, this is mainly > a trial balloon and proof of concept. > Some notes about the patch: > > 1) For the testing framework, I went with Google Test, since it's the one I > have the most experience with. I fully expect an extended bikeshed > discussion to result from this. > > 2) Both the test framework and the tests are optional build targets, they > will not be built with the normal "make all". To build and run the unit > tests, use "make check-unit". > > 3) I did not actually include the testing framework in the patch; It will > need to be checked in separately. There are two approaches to this. One > approach is to use the svn:external feature to create a link to the > googletest svn repository from the LLVM svn repository. The other approach > is to take a snapshot of googletest and check it in to the LLVM repository. > > The GoogleTest tar archive is here: > http://code.google.com/p/googletest/downloads/list. I've located it within > the LLVM source tree in the location "third-party/googletest". > > 4) I've included makefiles for building the test framework and unit tests. I > had to do a bit of makefile hacking to get this to work, suggestions on how > to improve this are welcome. > >
Hi Talin, I just got around to looking at this. Sorry for the delay. I like the idea of unit tests *A LOT*. Adequate testing has been a major sticking point for LLVM for a long time. In my opinion, once the color of the bike shed is agreed upon ;-), this will be a welcome addition. I have never worked with Google's unit testing infrastucture so I don't know how it works or if its license is compatible with ours, etc. I did a very quick look at your code and it seems fine for a first stab. Let's try to get everyone's input before you do a ton of work. But I for one think that it will be accepted when people are satisfied with the form it takes. -bw On Dec 25, 2008, at 11:32 PM, Talin <viridia at gmail.com> wrote:> So, any response on whether this patch is likely to be accepted? I'm > willing to substantially rework things if requested. > > I should note that part of my motivation for doing this is that I have > been wanting to do some work on the various LLVM container classes; > However, I strongly feel that without some kind of unit test framework > in place such work would mostly be a waste of time. > > Talin wrote: >> (Forwarding this to llvm-dev) >> >> This patch adds a unit test framework to LLVM, along with a sample >> unit test >> for DenseMap. I don't expect this patch to be accepted as-is, this >> is mainly >> a trial balloon and proof of concept. >> Some notes about the patch: >> >> 1) For the testing framework, I went with Google Test, since it's >> the one I >> have the most experience with. I fully expect an extended bikeshed >> discussion to result from this. >> >> 2) Both the test framework and the tests are optional build >> targets, they >> will not be built with the normal "make all". To build and run the >> unit >> tests, use "make check-unit". >> >> 3) I did not actually include the testing framework in the patch; >> It will >> need to be checked in separately. There are two approaches to this. >> One >> approach is to use the svn:external feature to create a link to the >> googletest svn repository from the LLVM svn repository. The other >> approach >> is to take a snapshot of googletest and check it in to the LLVM >> repository. >> >> The GoogleTest tar archive is here: >> http://code.google.com/p/googletest/downloads/list. I've located it >> within >> the LLVM source tree in the location "third-party/googletest". >> >> 4) I've included makefiles for building the test framework and unit >> tests. I >> had to do a bit of makefile hacking to get this to work, >> suggestions on how >> to improve this are welcome. >> >> > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Dec 22, 7:34 pm, Talin <viri... at gmail.com> wrote:> (Forwarding this to llvm-dev) > > This patch adds a unit test framework to LLVM, along with a sample unit test > for DenseMap. I don't expect this patch to be accepted as-is, this is mainly > a trial balloon and proof of concept.I think this is a great idea! As Keir already noted, I would also agree with LLVM snapshotting a copy of googletest, but I think it should live in llvm/test/googletest (rather than top-level), since it's not going to be linked into anything outside of llvm/test. On the assumption that we'll end up using googletest, here are some comments on your patch: * s/Insure/Ensure/ * LLVM uses "llvm/foo.h" for inclusion rather than <llvm/foo.h> * You should use the same format for gtest headers * If reverse iteration isn't supported, you should either have an ASSERT_DEATH() on the decrement, or not have the code there (that's commented out) at all. * Instead of this: EXPECT_TRUE(uintMap[0] == 1); you should use EXPECT_EQ() * Instead of this: EXPECT_TRUE(uintMap.find(0u) == uintMap.begin()); is it possible to use EXPECT_EQ() as well? * In this test: TEST_F(DenseMapTest, IterationTest) { you use the array "int numbers[100];" as an array of booleans; why not make it "bool visited[100];" to make clear what it for, how it's used, and maybe be slightly more efficient?> 1) For the testing framework, I went with Google Test, since it's the one I > have the most experience with. I fully expect an extended bikeshed > discussion to result from this. > > 2) Both the test framework and the tests are optional build targets, they > will not be built with the normal "make all". To build and run the unit > tests, use "make check-unit".I am with Keir on this one: "check-unit" isn't the most intuitive name to me -- "unittest" sounds fine, but this isn't a big deal to me. I'll also volunteer to help maintain the LLVM branch of googletest and review the unittests being checked in, as I also have experience with using googletest, and I think LLVM could derive a lot of value from unittesting. Misha
On Fri, Dec 26, 2008 at 8:06 PM, Misha Brukman <brukman at gmail.com> wrote:> On Dec 22, 7:34 pm, Talin <viri... at gmail.com> wrote: > > (Forwarding this to llvm-dev) > > > > This patch adds a unit test framework to LLVM, along with a sample unit > test > > for DenseMap. I don't expect this patch to be accepted as-is, this is > mainly > > a trial balloon and proof of concept. > > I think this is a great idea! As Keir already noted, I would also > agree with LLVM snapshotting a copy of googletest, but I think it > should live in llvm/test/googletest (rather than top-level), since > it's not going to be linked into anything outside of llvm/test. > > On the assumption that we'll end up using googletest, here are some > comments on your patch: > > * s/Insure/Ensure/ > > * LLVM uses "llvm/foo.h" for inclusion rather than <llvm/foo.h> > * You should use the same format for gtest headers > > * If reverse iteration isn't supported, you should either have an > ASSERT_DEATH() on the decrement, or not have the code there (that's > commented out) at all. > > * Instead of this: > EXPECT_TRUE(uintMap[0] == 1); > you should use EXPECT_EQ() > > * Instead of this: > EXPECT_TRUE(uintMap.find(0u) == uintMap.begin()); > is it possible to use EXPECT_EQ() as well? > > * In this test: > TEST_F(DenseMapTest, IterationTest) { > you use the array "int numbers[100];" as an array of booleans; why not > make it "bool visited[100];" to make clear what it for, how it's used, > and maybe be slightly more efficient? > > > 1) For the testing framework, I went with Google Test, since it's the one > I > > have the most experience with. I fully expect an extended bikeshed > > discussion to result from this. > > > > 2) Both the test framework and the tests are optional build targets, they > > will not be built with the normal "make all". To build and run the unit > > tests, use "make check-unit". > > I am with Keir on this one: "check-unit" isn't the most intuitive name > to me -- "unittest" sounds fine, but this isn't a big deal to me. > > I'll also volunteer to help maintain the LLVM branch of googletest and > review the unittests being checked in, as I also have experience with > using googletest, and I think LLVM could derive a lot of value from > unittesting.If we make useful changes to gtest, I volunteer to push them upstream. Keir> > > Misha > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081226/1abf452b/attachment.html>
Misha Brukman wrote:> On Dec 22, 7:34 pm, Talin <viri... at gmail.com> wrote: > >> (Forwarding this to llvm-dev) >> >> This patch adds a unit test framework to LLVM, along with a sample unit test >> for DenseMap. I don't expect this patch to be accepted as-is, this is mainly >> a trial balloon and proof of concept. >> > > I think this is a great idea! As Keir already noted, I would also > agree with LLVM snapshotting a copy of googletest, but I think it > should live in llvm/test/googletest (rather than top-level), since > it's not going to be linked into anything outside of llvm/test. >So as far as putting things in llvm/test, I have a question - the makefile in that directory contains a whole bunch of rules for interfacing with DejaGNU. The unit tests don't (and, I think, shouldn't) require any dependence on DejaGNU -- in fact, I'm hoping it will be possible to run the unit tests with only the base LLVM package, without all of the additional package installations required to run the other LLVM tests. I'm just wondering if it will be a problem organizing the makefile so that the unit tests don't have any dependence on the other tests.> On the assumption that we'll end up using googletest, here are some > comments on your patch: > > * s/Insure/Ensure/ > > * LLVM uses "llvm/foo.h" for inclusion rather than <llvm/foo.h> > * You should use the same format for gtest headers > > * If reverse iteration isn't supported, you should either have an > ASSERT_DEATH() on the decrement, or not have the code there (that's > commented out) at all. >I'll probably just remove it.> * Instead of this: > EXPECT_TRUE(uintMap[0] == 1); > you should use EXPECT_EQ() >Sure.> * Instead of this: > EXPECT_TRUE(uintMap.find(0u) == uintMap.begin()); > is it possible to use EXPECT_EQ() as well? >In order to use EXPECT_EQ, both arguments have to be printable, although you can make anything printable by adding the necessary stream operator overloads. In this particular case, I decided that the printed representation of an iterator wouldn't be meaningful, so I didn't bother defining those overloads.> * In this test: > TEST_F(DenseMapTest, IterationTest) { > you use the array "int numbers[100];" as an array of booleans; why not > make it "bool visited[100];" to make clear what it for, how it's used, > and maybe be slightly more efficient? >OK>> 1) For the testing framework, I went with Google Test, since it's the one I >> have the most experience with. I fully expect an extended bikeshed >> discussion to result from this. >> >> 2) Both the test framework and the tests are optional build targets, they >> will not be built with the normal "make all". To build and run the unit >> tests, use "make check-unit". >> > > I am with Keir on this one: "check-unit" isn't the most intuitive name > to me -- "unittest" sounds fine, but this isn't a big deal to me. > > I'll also volunteer to help maintain the LLVM branch of googletest and > review the unittests being checked in, as I also have experience with > using googletest, and I think LLVM could derive a lot of value from > unittesting. > > Misha > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >