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>
Just a curiosity question, why push for gtest vs Boost Test or a different test suite? I normally use Boost, and their test suite, so I'm more familiar with that. So I was wondering is one better then the other, or is it just that someone makes a patch for it? Regards Mark Kromis On Dec 27, 2008, at 12:26 AM, Keir Mierle wrote:> > > 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 > > _______________________________________________ > 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/20081227/b52b2c92/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: PGP.sig Type: application/pgp-signature Size: 832 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081227/b52b2c92/attachment.sig>
2008/12/27 Mark Kromis <greybird at mac.com>> Just a curiosity question, why push for gtest vs Boost Test or > a different test suite? > I normally use Boost, and their test suite, so I'm more familiar with that. > So I was wondering is one better then the other, or is it just that someone > makes a patch for it? >It seems that the original patch author is most familiar with gtest (as am I). I looked at boost's testing facilities, as described here: http://gamesfromwithin.com/?p=29#boost and it seems to be quite more verbose for setting up test suites, with what looks like a bit of boilerplate code. Compare this with the way new tests are written in gtest with TEST_F() macro -- see the original patch by Talin. Note that there's no suite-setup code in your unittest file in gtest, which is required in boost. In fact, the only gtest-related code is the #include statement, not counting the common main() that's linked into every unittest binary -- unless you want to do some custom global-setup code, and hence you'll write your own main(). Misha -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081227/dc4ae263/attachment.html>
2008/12/27 Mark Kromis <greybird at mac.com>> Just a curiosity question, why push for gtest vs Boost Test or > a different test suite? > I normally use Boost, and their test suite, so I'm more familiar with that. > So I was wondering is one better then the other, or is it just that someone > makes a patch for it? >I looked more into Boost.Test to see what's in it. Boost.Test doesn't seem to be stand-alone -- I don't see a way to use Boost.Test without importing some other chunks of Boost that the testing library depends on. While Boost is a fine set of libraries, I don't think we want to increase the LLVM distribution by sizeof(Boost) just to enable unittesting, nor do we want to spend the time on maintaining a subset of Boost that's "just enough" to build and use the unittest library, along a modified configure/build process that Boost wants to use (Boost.Build? Boost.Jam?). Boost also seems to want to use exceptions, and LLVM does not want to. I'm not sure if there would be some difficulties in running a build where some libraries are compiled with no exceptions, some with, and the results are linked together. At the best case, it would complicate our build system to be able to support different set of flags for building LLVM libraries vs. Boost.Test (and the rest of Boost that we import). Sample usage of Boost.Test: http://svn.boost.org/svn/boost/trunk/libs/test/example/unit_test_example_12.cpp Note the code at the end setting up the test suite -- this is boilerplate code that I think shouldn't be necessary to setup and run tests. Google Test, on the other hand, has no external dependencies, and is distributed as a dozen of .h/.cc files; supports Makefile, SCons, and Xcode; and doesn't use exceptions or RTTI. Sample usage of GTest: http://code.google.com/p/googletest/source/browse/trunk/samples/sample5_unittest.cc GTest-specific LOC besides the #include statement: 0. Note that I'm not counting main() for either Boost or GTest, because both provide a standard main() for use with almost all test files. Misha -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20081227/eeff2436/attachment.html>