[LLVMdev] [Patch] Adding unit tests to LLVM

[Talin]

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
>
>