|
|
|
Created:
16 years, 6 months ago by sleeepycat Modified:
16 years, 5 months ago Reviewers:
Timur Iskhodzhanov Base URL:
http://vectorr.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionI couldn't help myself making one more issue)
Here are just an empty several sections. Can I use this structure for my unit-tests?
Patch Set 1 #
Total comments: 10
Patch Set 2 : TextFixture class removed and two asserts added #Patch Set 3 : Big patch with unit-tests for int #
Total comments: 21
Patch Set 4 : Tests changed to better obey the contracts #
Total comments: 11
Patch Set 5 : Response to previous comments & Great renaming #
Total comments: 8
Patch Set 6 : Modified PushBack test #
Total comments: 8
Patch Set 7 : Response to comments #MessagesTotal messages: 24
I couldn't help myself making one more issue) Here are just an empty several sections. Can I use this structure for my unit-tests?
Sign in to reply to this message.
Please don't include .suo file in changelists http://codereview.appspot.com/163081/diff/1/3 File mystl/Vector.h (right): http://codereview.appspot.com/163081/diff/1/3#newcode21 mystl/Vector.h:21: size_ = capacity; I think this chunk is from the previous changelist you've sent me. Be careful not to commit too much when commiting previous CL. http://codereview.appspot.com/163081/diff/1/3#newcode44 mystl/Vector.h:44: din_array_[size_++] = value; I think this chunk is from the previous changelist you've sent me. Be careful not to commit too much when commiting previous CL. http://codereview.appspot.com/163081/diff/1/3#newcode64 mystl/Vector.h:64: }; You don't need a fixture class if you don't do any code in it. Just replace TEST_F with TEST in the .cpp file - and you don't need "class VectorTest" anymore http://codereview.appspot.com/163081/diff/1/2 File mystl_test/Main.cpp (right): http://codereview.appspot.com/163081/diff/1/2#newcode9 mystl_test/Main.cpp:9: //simillar operations for different types why don't you just write these tests?
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/1/3 File mystl/Vector.h (right): http://codereview.appspot.com/163081/diff/1/3#newcode64 mystl/Vector.h:64: }; On 2009/12/02 15:27:33, Timur Iskhodzhanov wrote: > You don't need a fixture class if you don't do any code in it. Just replace > TEST_F with TEST in the .cpp file - and you don't need "class VectorTest" > anymore I wanted to write setups for testing in TEST_F. Because I thought it would be better to separate code into two different parts. Here I could write setups and it test fixtures in main I could write testing code. So dhouldn't I do this? http://codereview.appspot.com/163081/diff/1/2 File mystl_test/Main.cpp (right): http://codereview.appspot.com/163081/diff/1/2#newcode9 mystl_test/Main.cpp:9: //simillar operations for different types On 2009/12/02 15:27:33, Timur Iskhodzhanov wrote: > why don't you just write these tests? I wasn't sure If such structure of tests is good) So I decieded to write just names of tets and then make them better if the structure is bad. And after that I wanted to write tests. I tried to separate work for small steps by this. Maybe these steps are too small..) http://codereview.appspot.com/163081/diff/1/2#newcode13 mystl_test/Main.cpp:13: // (size_ >= capacity_). In this section will be Sorry! Here it should have been "(size_ == capacity_)"
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/1/3 File mystl/Vector.h (right): http://codereview.appspot.com/163081/diff/1/3#newcode64 mystl/Vector.h:64: }; In general, it is good to write common SetUp/TearDown code for a number of test. However, I think you don't really need to right now since your SetUp() method is currently empty On 2009/12/04 10:11:42, sleeepycat wrote: > On 2009/12/02 15:27:33, Timur Iskhodzhanov wrote: > > You don't need a fixture class if you don't do any code in it. Just replace > > TEST_F with TEST in the .cpp file - and you don't need "class VectorTest" > > anymore > > I wanted to write setups for testing in TEST_F. Because I thought it would be > better to separate code into two different parts. Here I could write setups and > it test fixtures in main I could write testing code. So dhouldn't I do this? http://codereview.appspot.com/163081/diff/1/2 File mystl_test/Main.cpp (right): http://codereview.appspot.com/163081/diff/1/2#newcode9 mystl_test/Main.cpp:9: //simillar operations for different types You'd better take a look at some real tests e.g. http://src.chromium.org/viewvc/chrome/trunk/src/base/ -- any file with _unittests.cc at the end or http://code.google.com/p/googletest/wiki/GoogleTestSamples And just write the tests. I don't think these two test you've prototyped would be more than 5 lines long On 2009/12/04 10:11:42, sleeepycat wrote: > On 2009/12/02 15:27:33, Timur Iskhodzhanov wrote: > > why don't you just write these tests? > I wasn't sure If such structure of tests is good) So I decieded to write just > names of tets and then make them better if the structure is bad. And after that > I wanted to write tests. I tried to separate work for small steps by this. Maybe > these steps are too small..) >
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/1/3 File mystl/Vector.h (right): http://codereview.appspot.com/163081/diff/1/3#newcode64 mystl/Vector.h:64: }; ok
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode6 Main.cpp:6: TEST(Constructor, DefaultValuesTest) { Sorry! I didn't want them to be so great, but.. They are((
Sign in to reply to this message.
General comment: You're trying to test the implementation details, not the contract. For example, http://www.cplusplus.com/reference/stl/vector/push_back/ doesn't require the capacity to be doubled for each reallocation, right? Please try writing the tests so that if you add these lines at the top of Main.cpp #define Vector std::vector #define PushBack push_back , your tests MUST pass. I think some of your proposed tests will fail. http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode6 Main.cpp:6: TEST(Constructor, DefaultValuesTest) { They are not great, I think you've meant "large"? On 2009/12/07 13:37:22, sleeepycat wrote: > Sorry! I didn't want them to be so great, but.. They are(( http://codereview.appspot.com/163081/diff/4001/2006#newcode10 Main.cpp:10: //default_val or not this particular test is pretty self-descriptive, you may delete the lines with comments. Btw, your comments are violating Google Code Style recommendations. http://codereview.appspot.com/163081/diff/4001/2006#newcode13 Main.cpp:13: Vector<int> int_vector(6, int_default_value); rename int_vector to "v" http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); please, * rename int_vectorX to "vX" * one definition per line * write all the definition with the same indentation http://codereview.appspot.com/163081/diff/4001/2006#newcode28 Main.cpp:28: EXPECT_EQ(DEFAULT_CAPACITY, int_vector.capacity()); hey, I don't see capacity() in Vector.h http://codereview.appspot.com/163081/diff/4001/2006#newcode32 Main.cpp:32: EXPECT_EQ(size, int_vector4.capacity()); Please chech that std::vector passes this test. I think it may not. http://codereview.appspot.com/163081/diff/4001/2006#newcode46 Main.cpp:46: Vector<int> int_vector; rename int_vector to "v" http://codereview.appspot.com/163081/diff/4001/2006#newcode57 Main.cpp:57: EXPECT_EQ(DEFAULT_CAPACITY*2, int_vector.capacity()); I think it's better to write a less strict test. For example, I don't think you can get some sort of DEFAULT_CAPACITY equivalent for std::vector, right? If you can't - you can't write this test for std::vector which is bad. I think it's better to use like EXPECT_LE(old_capacity, int_vector.capacity()) inside the for() loop and remove the code after the loop.
Sign in to reply to this message.
>doesn't require the capacity to be doubled for each reallocation, right? Why not? I can't get you. "Adds a new element at the end of the vector, after its current last element. The content of this new element is initialized to a copy of *x*. This effectively increases the vector size<http://www.cplusplus.com/vector::size>by one, which causes a reallocation of the internal allocated storage if the vector size <http://www.cplusplus.com/vector::size> was equal to the vector capacity <http://www.cplusplus.com/vector::capacity> before the call. Reallocations invalidate all previously obtained iterators, references and pointers." This is contract, isn't it? And here they write that "... causes a reallocation of the internal allocated storage if the vector size<http://www.cplusplus.com/vector::size>was equal to the vector capacity <http://www.cplusplus.com/vector::capacity> before the call"? Isn't this the thing that I should test?
Sign in to reply to this message.
Or do you mean that I should test just the fact that reallallocation been occured? Should do it not through capacity? If yes, how can I do this? 2009/12/8 sleeepycat <sleeepycat@gmail.com> > >doesn't require the capacity to be doubled for each reallocation, right? > Why not? I can't get you. > > "Adds a new element at the end of the vector, after its current last > element. The content of this new element is initialized to a copy of *x*. > > This effectively increases the vector size<http://www.cplusplus.com/vector::size>by one, which causes a reallocation of the internal allocated storage if the > vector size <http://www.cplusplus.com/vector::size> was equal to the > vector capacity <http://www.cplusplus.com/vector::capacity> before the > call. Reallocations invalidate all previously obtained iterators, references > and pointers." > > This is contract, isn't it? And here they write that "... causes a > reallocation of the internal allocated storage if the vector size<http://www.cplusplus.com/vector::size>was equal to the vector > capacity <http://www.cplusplus.com/vector::capacity> before the call"? > Isn't this the thing that I should test? >
Sign in to reply to this message.
I have several questions before continuing working on code http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode6 Main.cpp:6: TEST(Constructor, DefaultValuesTest) { Yes. I meant "large". "Great" was my sense of humor. In russian I would say something like this as well) http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); Timur, but how will I make difference between int and other types? Should I make "int_vX" or somethig like this? http://codereview.appspot.com/163081/diff/4001/2006#newcode28 Main.cpp:28: EXPECT_EQ(DEFAULT_CAPACITY, int_vector.capacity()); On 2009/12/07 15:55:50, Timur Iskhodzhanov wrote: hm.. But here is capacity() =)) I took a look at Vector.h in the patch 2, and there is capacity there) http://codereview.appspot.com/163081/diff/4001/2006#newcode32 Main.cpp:32: EXPECT_EQ(size, int_vector4.capacity()); it works http://codereview.appspot.com/163081/diff/4001/2006#newcode46 Main.cpp:46: Vector<int> int_vector; Here is the same question: what should I do with this int?..
Sign in to reply to this message.
> This is contract, isn't it? > And here they write that "... causes a reallocation of the internal > allocated storage if the vector size was equal to the vector capacity > before the call"? What I mean is that you test the exact implementation - your vector's capacity is doubled on each reallocation, but it is not enforced in a contract. Different implementations may behave differently, failing your tests. This may lead to same problems as described here: http://en.wikipedia.org/wiki/Mock_object#Limitations To test that reallocation occurs you can simply test EXPECT_LE(v.size(), v.capacity()); after each push_back, right? http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); Do you need to explicitly specify the types when declaring class field variables? I don't think so. Speaking of these unit-tests, they are tiny (less than 20 lines of code each), so why bother inventing long names? Of course, for bigger tests it's better to have descriptive names. On 2009/12/08 07:36:56, sleeepycat wrote: > Timur, but how will I make difference between int and other types? Should I make > "int_vX" or somethig like this? http://codereview.appspot.com/163081/diff/4001/2006#newcode28 Main.cpp:28: EXPECT_EQ(DEFAULT_CAPACITY, int_vector.capacity()); On 2009/12/08 07:36:56, sleeepycat wrote: > On 2009/12/07 15:55:50, Timur Iskhodzhanov wrote: > > hm.. But here is capacity() =)) I took a look at Vector.h in the patch 2, and > there is capacity there) Done.
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); ">Do you need to explicitly specify the types when declaring class field variables?" I thought I was need it. I wanted to create several vectors of different types in one test and after creation add elements into them. Elements' types should correspond types of vectors. I read about typed tests from http://code.google.com/p/googletest/source/browse/trunk/samples/sample6_unitt... I thought of usage of typed tests in my code. I understood how can I create vectors of defferent types automatically. But I couldn't understand what should I do with types of adding elements. So I decided just to write several vectors of different types with several adding elements of corresponding types. As it seems to me, in this case I need part "int_" in the names of vectors and adding elements http://codereview.appspot.com/163081/diff/4001/2006#newcode57 Main.cpp:57: EXPECT_EQ(DEFAULT_CAPACITY*2, int_vector.capacity()); ok. I'll try to make tests better obey the contracts. And I think, yes, there should be a loop
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); If you test different types, you may use int_ prefix for int-vector, float_ for float-vector etc. But in this particular test you use only int_ vectors, don't you? On 2009/12/09 10:02:24, sleeepycat wrote: > ">Do you need to explicitly specify the types when declaring class field > variables?" > I thought I was need it. I wanted to create several vectors of different types > in one test and after creation add elements into them. Elements' types should > correspond types of vectors. > > I read about typed tests from > http://code.google.com/p/googletest/source/browse/trunk/samples/sample6_unitt... > I thought of usage of typed tests in my code. I understood how can I create > vectors of defferent types automatically. But I couldn't understand what should > I do with types of adding elements. So I decided just to write several vectors > of different types with several adding elements of corresponding types. As it > seems to me, in this case I need part "int_" in the names of vectors and adding > elements http://codereview.appspot.com/163081/diff/4001/2006#newcode57 Main.cpp:57: EXPECT_EQ(DEFAULT_CAPACITY*2, int_vector.capacity()); There is a for loop above? On 2009/12/09 10:02:24, sleeepycat wrote: > ok. I'll try to make tests better obey the contracts. And I think, yes, there > should be a loop
Sign in to reply to this message.
Looks like you had a newer patchset uploaded. I've missed it, sorry. (next time please explicitly ask for re-review of a new patchset) http://codereview.appspot.com/163081/diff/4006/2012 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4006/2012#newcode6 Main.cpp:6: TEST(Constructor, DefaultValuesTest) { TEST(VectorTest, ConstructorWithDefaultValuesTest) http://codereview.appspot.com/163081/diff/4006/2012#newcode8 Main.cpp:8: Vector<int> int_vector(6, int_default_value); please, just "v" http://codereview.appspot.com/163081/diff/4006/2012#newcode14 Main.cpp:14: TEST(PushBack, EmptyVector) { TEST(VectorTest, PushBackTest) http://codereview.appspot.com/163081/diff/4006/2012#newcode16 Main.cpp:16: Vector<int> int_vector; please, just "v" http://codereview.appspot.com/163081/diff/4006/2012#newcode27 Main.cpp:27: TEST(PushBack, SeveralElementsVector) { what's the point of this test? Both int_vectors are processed in the same way as PushBack.EmptyVector test.
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4001/2006 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4001/2006#newcode26 Main.cpp:26: int_vector4(size, int_default_value); I want to write all the types of tests in THIS test. I have only int test here just because all the other types I wanted to write as the next step. In my plan there should be not only int http://codereview.appspot.com/163081/diff/4006/2012#newcode6 Main.cpp:6: TEST(Constructor, DefaultValuesTest) { On 2009/12/12 15:53:56, Timur Iskhodzhanov wrote: Done. http://codereview.appspot.com/163081/diff/4006/2012#newcode8 Main.cpp:8: //Vector(int capacity, const T &default_val) ok. I did it http://codereview.appspot.com/163081/diff/4006/2012#newcode14 Main.cpp:14: for(int i = 0; i < int_vector.size(); i++) Do you mean I should merge these two tests? I did http://codereview.appspot.com/163081/diff/4006/2012#newcode16 Main.cpp:16: } ok
Sign in to reply to this message.
Here is new patchset)
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4006/2012 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4006/2012#newcode14 Main.cpp:14: TEST(PushBack, EmptyVector) { Actually, no On 2009/12/14 06:17:54, sleeepycat wrote: > Do you mean I should merge these two tests? I did http://codereview.appspot.com/163081/diff/7004/7005#newcode5 Main.cpp:5: why did you create a few empty lines? http://codereview.appspot.com/163081/diff/7004/7005#newcode12 Main.cpp:12: empty line http://codereview.appspot.com/163081/diff/7004/7005#newcode14 Main.cpp:14: TEST(PushBack, EmptyVector) { The three different tests (which you've merged into one) have almost exactly the same code (arrrgh) and differ only in the vector constructor parameters. I think you should completely avoid duplicating code THREE times inside one test and better extract the duplicated code into one separate function. Then simply call it three times with different arguments. Btw, please add checks that the size of vector after N times push_back is equal to "old_size + N" http://codereview.appspot.com/163081/diff/7004/7005#newcode40 Main.cpp:40: EXPECT_EQ(int_add_value, int_vector2[i]); empty line
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/4006/2012 File Main.cpp (right): http://codereview.appspot.com/163081/diff/4006/2012#newcode27 Main.cpp:27: TEST(PushBack, SeveralElementsVector) { I made two different tests because vector wich is created as empty and vector wich is created as vector with several elements behave different
Sign in to reply to this message.
Here is response. Look through new patchset, please http://codereview.appspot.com/163081/diff/7004/7005 File Main.cpp (right): http://codereview.appspot.com/163081/diff/7004/7005#newcode5 Main.cpp:5: I thought it would be more readble) ok. I changed all doublee lines to one line (in the next patchset) http://codereview.appspot.com/163081/diff/7004/7005#newcode12 Main.cpp:12: On 2009/12/14 12:34:36, Timur Iskhodzhanov wrote: Done. http://codereview.appspot.com/163081/diff/7004/7005#newcode14 Main.cpp:14: TEST(VectorTest, PushBackTest) { Oh, yes!.. You are perfectly right. I did. http://codereview.appspot.com/163081/diff/7004/7005#newcode40 Main.cpp:40: On 2009/12/14 12:34:36, Timur Iskhodzhanov wrote: Done.
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/9002/8004 File Main.cpp (right): http://codereview.appspot.com/163081/diff/9002/8004#newcode5 Main.cpp:5: TEST(VectorTest, ConstructorWithDefaultValues) { ConstructorWithDefaultValuesTest http://codereview.appspot.com/163081/diff/9002/8004#newcode13 Main.cpp:13: void PushBackTestCycle(Vector<T> &v, T &add_value, int start_num, int end_num) { hint: in this function you don't need the absolute values of start_num and end_num, you only need their difference http://codereview.appspot.com/163081/diff/9002/8004#newcode32 Main.cpp:32: PushBackTestCycle(v2, add_value, start_size, number_of_elem); re-arrange the lines and use one line per definition: (Definitions first) int start_size = 7, default_value = 7; Vector<int> v, v1(...), v2(...); int add_value = 5, num_of_elements = 30; // I think it's a better name PushBackTestCycle(...); PushBackTestCycle(...); PushBackTestCycle(...);
Sign in to reply to this message.
http://codereview.appspot.com/163081/diff/9002/8004 File Main.cpp (right): http://codereview.appspot.com/163081/diff/9002/8004#newcode5 Main.cpp:5: TEST(VectorTest, ConstructorWithDefaultValues) { On 2009/12/15 12:27:02, Timur Iskhodzhanov wrote: Done. http://codereview.appspot.com/163081/diff/9002/8004#newcode13 Main.cpp:13: void PushBackTestCycle(Vector<T> &v, T &add_value, int start_num, int end_num) { Do you mean I can find my start_num from v.size()? I've done that http://codereview.appspot.com/163081/diff/9002/8004#newcode32 Main.cpp:32: PushBackTestCycle(v2, add_value, start_size, number_of_elem); Ok. But I don't understand, why there are int defenitions the first, then Vector<int>, then int again. In the next patch merged both int defenitions
Sign in to reply to this message.
LGTM. If all the tests pass - commit. There are a couple of subtle problems in the test code, but I think it's better to find&fix them later than trying to do everything perfect right away. http://codereview.appspot.com/163081/diff/9002/8004 File Main.cpp (right): http://codereview.appspot.com/163081/diff/9002/8004#newcode13 Main.cpp:13: void PushBackTestCycle(Vector<T> &v, T &add_value, int start_num, int end_num) { Not exactly, but you did it fine On 2009/12/16 09:47:58, sleeepycat wrote: > Do you mean I can find my start_num from v.size()? I've done that http://codereview.appspot.com/163081/diff/9002/8004#newcode32 Main.cpp:32: PushBackTestCycle(v2, add_value, start_size, number_of_elem); Your variant is OK On 2009/12/16 09:47:58, sleeepycat wrote: > Ok. But I don't understand, why there are int defenitions the first, then > Vector<int>, then int again. In the next patch merged both int defenitions
Sign in to reply to this message.
I did) 2009/12/16 <timurrrr@gmail.com> > LGTM. > If all the tests pass - commit. > > There are a couple of subtle problems in the test code, but I think it's > better to find&fix them later than trying to do everything perfect right > away. > > > > http://codereview.appspot.com/163081/diff/9002/8004 > File Main.cpp (right): > > http://codereview.appspot.com/163081/diff/9002/8004#newcode13 > Main.cpp:13: void PushBackTestCycle(Vector<T> &v, T &add_value, int > start_num, int end_num) { > Not exactly, but you did it fine > > On 2009/12/16 09:47:58, sleeepycat wrote: > >> Do you mean I can find my start_num from v.size()? I've done that >> > > http://codereview.appspot.com/163081/diff/9002/8004#newcode32 > Main.cpp:32: PushBackTestCycle(v2, add_value, start_size, > number_of_elem); > Your variant is OK > > On 2009/12/16 09:47:58, sleeepycat wrote: > >> Ok. But I don't understand, why there are int defenitions the first, >> > then > >> Vector<int>, then int again. In the next patch merged both int >> > defenitions > > http://codereview.appspot.com/163081 >
Sign in to reply to this message.
Everythig passes) 2009/12/21 sleeepycat <sleeepycat@gmail.com> > I did) > > 2009/12/16 <timurrrr@gmail.com> > > LGTM. >> If all the tests pass - commit. >> >> There are a couple of subtle problems in the test code, but I think it's >> better to find&fix them later than trying to do everything perfect right >> away. >> >> >> >> http://codereview.appspot.com/163081/diff/9002/8004 >> File Main.cpp (right): >> >> http://codereview.appspot.com/163081/diff/9002/8004#newcode13 >> Main.cpp:13: void PushBackTestCycle(Vector<T> &v, T &add_value, int >> start_num, int end_num) { >> Not exactly, but you did it fine >> >> On 2009/12/16 09:47:58, sleeepycat wrote: >> >>> Do you mean I can find my start_num from v.size()? I've done that >>> >> >> http://codereview.appspot.com/163081/diff/9002/8004#newcode32 >> Main.cpp:32: PushBackTestCycle(v2, add_value, start_size, >> number_of_elem); >> Your variant is OK >> >> On 2009/12/16 09:47:58, sleeepycat wrote: >> >>> Ok. But I don't understand, why there are int defenitions the first, >>> >> then >> >>> Vector<int>, then int again. In the next patch merged both int >>> >> defenitions >> >> http://codereview.appspot.com/163081 >> > >
Sign in to reply to this message.
|
