app: Add unittests for TreeNodeModel.

app: Add unittests for TreeNodeModel.

BUG=None
TEST=out/Debug/app_unittests --gtest_filter=TreeNodeModelTest.*

Signed-off-by: Thiago Farina <tfarina@chromium.org>;

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55179

[tfarina, 2010-07-23 03:16:46]

Hi Pawel, could you review this to me?

[Paweł Hajdan Jr., 2010-07-23 18:28:13]

+sky. Scott, could you take a look? I'm not really familiar with the code being
tested here.

[sky, 2010-07-23 19:10:14]

http://codereview.chromium.org/3017023/diff/4001/5002
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/4001/5002#newcode8
app/tree_node_model_unittest.cc:8: TEST(TreeNodeModelTest, AddNode) {
Couple of comments:
. Please add a description above each test summarizing what the test does.
. You should also add coverage that the observer is notified.
. Add coverage of HasAncestor, GetTotalNodeCount.

http://codereview.chromium.org/3017023/diff/4001/5002#newcode91
app/tree_node_model_unittest.cc:91: root.RemoveAll();
This leaks.

[tfarina, 2010-07-25 00:44:33]

On 2010/07/23 19:10:14, sky wrote:
> http://codereview.chromium.org/3017023/diff/4001/5002
> File app/tree_node_model_unittest.cc (right):
> 
> http://codereview.chromium.org/3017023/diff/4001/5002#newcode8
> app/tree_node_model_unittest.cc:8: TEST(TreeNodeModelTest, AddNode) {
> Couple of comments:
> . Please add a description above each test summarizing what the test does.
> . You should also add coverage that the observer is notified.
> . Add coverage of HasAncestor, GetTotalNodeCount.
> 
Covered all items above.

> http://codereview.chromium.org/3017023/diff/4001/5002#newcode91
> app/tree_node_model_unittest.cc:91: root.RemoveAll();
> This leaks.
Scott, I need some help here to fix this. (I tried to use 'delete', but I got an
error from process_util_posix.cc).

Pawel/Scott, I looked into cookies_tree_model_unittest.cc and it's using
ChromeThread. Is this related to the failure I got using 'delete' (also using
scoped_ptr the same thing happens)?

[tfarina, 2010-08-01 01:09:30]

git blame you Adam. Could you take a look at this please?

[agl, 2010-08-02 14:40:54]

This code is nothing to do with me, no matter what git blame may say. None the
less, if you can't find a better reviewer, then LGTM.

[sky, 2010-08-02 17:57:50]

Sorry for the delay, was on vacation last week.

http://codereview.chromium.org/3017023/diff/13003/15002
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/13003/15002#newcode9
app/tree_node_model_unittest.cc:9: public:
Add a constructor that sets the fields to 0.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode45
app/tree_node_model_unittest.cc:45: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3017023/diff/13003/15002#newcode52
app/tree_node_model_unittest.cc:52: TreeNodeModel< TreeNodeWithValue<int> >*
model =
You really only need a space between >>.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode72
app/tree_node_model_unittest.cc:72: // Create the second child node.
child -> root child

http://codereview.chromium.org/3017023/diff/13003/15002#newcode166
app/tree_node_model_unittest.cc:166: ASSERT_NE(0, model->IndexOfChild(root,
child2));
Nuke this as it's covered by line 168.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode169
app/tree_node_model_unittest.cc:169: ASSERT_NE(1, model->IndexOfChild(root,
child1));
Nuke this as it's covered by line 165.

[sky, 2010-08-02 17:58:19]

On Sat, Jul 24, 2010 at 5:44 PM,  <tfarina@chromium.org> wrote:
> On 2010/07/23 19:10:14, sky wrote:
>>
>> http://codereview.chromium.org/3017023/diff/4001/5002
>> File app/tree_node_model_unittest.cc (right):
>
>> http://codereview.chromium.org/3017023/diff/4001/5002#newcode8
>> app/tree_node_model_unittest.cc:8: TEST(TreeNodeModelTest, AddNode) {
>> Couple of comments:
>> . Please add a description above each test summarizing what the test does.
>> . You should also add coverage that the observer is notified.
>> . Add coverage of HasAncestor, GetTotalNodeCount.
>
> Covered all items above.
>
>> http://codereview.chromium.org/3017023/diff/4001/5002#newcode91
>> app/tree_node_model_unittest.cc:91: root.RemoveAll();
>> This leaks.
>
> Scott, I need some help here to fix this. (I tried to use 'delete', but I
> got an
> error from process_util_posix.cc).

What error did you get?

  -Scott

[tfarina, 2010-08-03 01:59:34]

On Mon, Aug 2, 2010 at 2:58 PM, Scott Violet <sky@chromium.org> wrote:
> On Sat, Jul 24, 2010 at 5:44 PM,  <tfarina@chromium.org> wrote:
>> On 2010/07/23 19:10:14, sky wrote:
>>>
>>> http://codereview.chromium.org/3017023/diff/4001/5002
>>> File app/tree_node_model_unittest.cc (right):
>>
>>> http://codereview.chromium.org/3017023/diff/4001/5002#newcode8
>>> app/tree_node_model_unittest.cc:8: TEST(TreeNodeModelTest, AddNode) {
>>> Couple of comments:
>>> . Please add a description above each test summarizing what the test does.
>>> . You should also add coverage that the observer is notified.
>>> . Add coverage of HasAncestor, GetTotalNodeCount.
>>
>> Covered all items above.
>>
>>> http://codereview.chromium.org/3017023/diff/4001/5002#newcode91
>>> app/tree_node_model_unittest.cc:91: root.RemoveAll();
>>> This leaks.
>>
>> Scott, I need some help here to fix this. (I tried to use 'delete', but I
>> got an
>> error from process_util_posix.cc).
>
> What error did you get?
>
I got this:

[22782:22782:0802/225749:93052193375:ERROR:base/process_util_posix.cc(107)]
Received signal 11
	StackTrace::StackTrace() [0x1d63ae]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x20a19e]
	0x622400
	TreeNodeModel<>::~TreeNodeModel() [0x8098603]
	TreeNodeModelTest_RemoveAllNodes_Test::TestBody() [0x809617a]
	testing::Test::Run() [0x9a97a2]
	testing::internal::TestInfoImpl::Run() [0x9a9c87]
	testing::TestCase::Run() [0x9aa193]
	testing::internal::UnitTestImpl::RunAllTests() [0x9adf62]
	testing::UnitTest::Run() [0x9acec6]
	TestSuite::Run() [0x8083edd]
	main [0x8083717]
	0x68c4bd6
	0x806a731

I think is my mistake. I got this when I do:

delete model;

in the RemoveAll test.

-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[tfarina, 2010-08-03 02:29:05]

http://codereview.chromium.org/3017023/diff/13003/15002
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/13003/15002#newcode9
app/tree_node_model_unittest.cc:9: public:
On 2010/08/02 17:57:50, sky wrote:
> Add a constructor that sets the fields to 0.

Done.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode45
app/tree_node_model_unittest.cc:45: };
On 2010/08/02 17:57:50, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode52
app/tree_node_model_unittest.cc:52: TreeNodeModel< TreeNodeWithValue<int> >*
model =
On 2010/08/02 17:57:50, sky wrote:
> You really only need a space between >>.

Done.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode72
app/tree_node_model_unittest.cc:72: // Create the second child node.
On 2010/08/02 17:57:50, sky wrote:
> child -> root child

Done.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode166
app/tree_node_model_unittest.cc:166: ASSERT_NE(0, model->IndexOfChild(root,
child2));
On 2010/08/02 17:57:50, sky wrote:
> Nuke this as it's covered by line 168.

Done.

http://codereview.chromium.org/3017023/diff/13003/15002#newcode169
app/tree_node_model_unittest.cc:169: ASSERT_NE(1, model->IndexOfChild(root,
child1));
On 2010/08/02 17:57:50, sky wrote:
> Nuke this as it's covered by line 165.

Done.

[sky, 2010-08-03 02:48:51]

Ya, don't delete the model just the nodes you're removing.

  -Scott

On Mon, Aug 2, 2010 at 7:29 PM,  <tfarina@chromium.org> wrote:
>
> http://codereview.chromium.org/3017023/diff/13003/15002
> File app/tree_node_model_unittest.cc (right):
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode9
> app/tree_node_model_unittest.cc:9: public:
> On 2010/08/02 17:57:50, sky wrote:
>>
>> Add a constructor that sets the fields to 0.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode45
> app/tree_node_model_unittest.cc:45: };
> On 2010/08/02 17:57:50, sky wrote:
>>
>> DISALLOW_COPY_AND_ASSIGN
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode52
> app/tree_node_model_unittest.cc:52: TreeNodeModel<
> TreeNodeWithValue<int> >* model =
> On 2010/08/02 17:57:50, sky wrote:
>>
>> You really only need a space between >>.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode72
> app/tree_node_model_unittest.cc:72: // Create the second child node.
> On 2010/08/02 17:57:50, sky wrote:
>>
>> child -> root child
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode166
> app/tree_node_model_unittest.cc:166: ASSERT_NE(0,
> model->IndexOfChild(root, child2));
> On 2010/08/02 17:57:50, sky wrote:
>>
>> Nuke this as it's covered by line 168.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/13003/15002#newcode169
> app/tree_node_model_unittest.cc:169: ASSERT_NE(1,
> model->IndexOfChild(root, child1));
> On 2010/08/02 17:57:50, sky wrote:
>>
>> Nuke this as it's covered by line 165.
>
> Done.
>
> http://codereview.chromium.org/3017023/show
>

[tfarina, 2010-08-03 02:55:20]

On Mon, Aug 2, 2010 at 11:48 PM, Scott Violet <sky@chromium.org> wrote:
> Ya, don't delete the model just the nodes you're removing.
>
Currently I'm only explicit deleting the root in the RemoveAll test.
Should I expand this to the others nodes and for all the tests?

[sky, 2010-08-03 15:08:06]

On Mon, Aug 2, 2010 at 7:55 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Mon, Aug 2, 2010 at 11:48 PM, Scott Violet <sky@chromium.org> wrote:
>> Ya, don't delete the model just the nodes you're removing.
>>
> Currently I'm only explicit deleting the root in the RemoveAll test.
> Should I expand this to the others nodes and for all the tests?
>

See these two comments:

  // Removes the node by index. This does NOT delete the specified node, it is
  // up to the caller to delete it when done.
  virtual NodeType* Remove(int index) {

  // Removes all the children from this node. This does NOT delete the nodes.
    void RemoveAll() {

  -Scott

[tfarina, 2010-08-04 00:56:15]

On 2010/08/03 15:08:06, sky wrote:
> On Mon, Aug 2, 2010 at 7:55 PM, Thiago Farina <mailto:tfarina@chromium.org>
wrote:
> > On Mon, Aug 2, 2010 at 11:48 PM, Scott Violet <mailto:sky@chromium.org>
wrote:
> >> Ya, don't delete the model just the nodes you're removing.
> >>
> > Currently I'm only explicit deleting the root in the RemoveAll test.
> > Should I expand this to the others nodes and for all the tests?
> >
> 
> See these two comments:
> 
>   // Removes the node by index. This does NOT delete the specified node, it is
>   // up to the caller to delete it when done.
>   virtual NodeType* Remove(int index) {
> 
>   // Removes all the children from this node. This does NOT delete the nodes.
>     void RemoveAll() {

Oh, I see, please take another look.

Thanks.

[sky, 2010-08-04 18:33:45]

http://codereview.chromium.org/3017023/diff/17002/25003
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/17002/25003#newcode66
app/tree_node_model_unittest.cc:66: TreeNodeModel<TreeNodeWithValue<int> >*
model =
model leaks in this test.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode106
app/tree_node_model_unittest.cc:106: TreeNodeModel<TreeNodeWithValue<int> >*
model =
this test leaks model.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode141
app/tree_node_model_unittest.cc:141: TreeNodeModel<TreeNodeWithValue<int> >*
model =
and this one.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode166
app/tree_node_model_unittest.cc:166: delete root;
you shouldn't have to delete root.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode179
app/tree_node_model_unittest.cc:179: TreeNodeModel<TreeNodeWithValue<int> >*
model =
this leaks model.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode209
app/tree_node_model_unittest.cc:209: TreeNodeWithValue<int>* root =
this leaks root, child1, and child2.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode264
app/tree_node_model_unittest.cc:264: TreeNodeWithValue<int>* bar1 =
all nodes are leaked in this test.

[tfarina, 2010-08-05 11:04:45]

Fixed all memory leaks in patchset 7. Scott, could you take another look?

Thanks.

http://codereview.chromium.org/3017023/diff/17002/25003
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/17002/25003#newcode66
app/tree_node_model_unittest.cc:66: TreeNodeModel<TreeNodeWithValue<int> >*
model =
On 2010/08/04 18:33:45, sky wrote:
> model leaks in this test.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode106
app/tree_node_model_unittest.cc:106: TreeNodeModel<TreeNodeWithValue<int> >*
model =
On 2010/08/04 18:33:45, sky wrote:
> this test leaks model.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode141
app/tree_node_model_unittest.cc:141: TreeNodeModel<TreeNodeWithValue<int> >*
model =
On 2010/08/04 18:33:45, sky wrote:
> and this one.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode166
app/tree_node_model_unittest.cc:166: delete root;
On 2010/08/04 18:33:45, sky wrote:
> you shouldn't have to delete root.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode179
app/tree_node_model_unittest.cc:179: TreeNodeModel<TreeNodeWithValue<int> >*
model =
On 2010/08/04 18:33:45, sky wrote:
> this leaks model.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode209
app/tree_node_model_unittest.cc:209: TreeNodeWithValue<int>* root =
On 2010/08/04 18:33:45, sky wrote:
> this leaks root, child1, and child2.

Done.

http://codereview.chromium.org/3017023/diff/17002/25003#newcode264
app/tree_node_model_unittest.cc:264: TreeNodeWithValue<int>* bar1 =
On 2010/08/04 18:33:45, sky wrote:
> all nodes are leaked in this test.

Done.

[sky, 2010-08-05 15:41:48]

Before I take another look. Can you run your test through valgrind to
make sure you've caught them all?

Thanks,

  -Scott

On Thu, Aug 5, 2010 at 4:04 AM,  <tfarina@chromium.org> wrote:
> Fixed all memory leaks in patchset 7. Scott, could you take another look?
>
> Thanks.
>
>
> http://codereview.chromium.org/3017023/diff/17002/25003
> File app/tree_node_model_unittest.cc (right):
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode66
> app/tree_node_model_unittest.cc:66: TreeNodeModel<TreeNodeWithValue<int>
>>
>> * model =
>
> On 2010/08/04 18:33:45, sky wrote:
>>
>> model leaks in this test.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode106
> app/tree_node_model_unittest.cc:106:
> TreeNodeModel<TreeNodeWithValue<int> >* model =
> On 2010/08/04 18:33:45, sky wrote:
>>
>> this test leaks model.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode141
> app/tree_node_model_unittest.cc:141:
> TreeNodeModel<TreeNodeWithValue<int> >* model =
> On 2010/08/04 18:33:45, sky wrote:
>>
>> and this one.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode166
> app/tree_node_model_unittest.cc:166: delete root;
> On 2010/08/04 18:33:45, sky wrote:
>>
>> you shouldn't have to delete root.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode179
> app/tree_node_model_unittest.cc:179:
> TreeNodeModel<TreeNodeWithValue<int> >* model =
> On 2010/08/04 18:33:45, sky wrote:
>>
>> this leaks model.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode209
> app/tree_node_model_unittest.cc:209: TreeNodeWithValue<int>* root =
> On 2010/08/04 18:33:45, sky wrote:
>>
>> this leaks root, child1, and child2.
>
> Done.
>
> http://codereview.chromium.org/3017023/diff/17002/25003#newcode264
> app/tree_node_model_unittest.cc:264: TreeNodeWithValue<int>* bar1 =
> On 2010/08/04 18:33:45, sky wrote:
>>
>> all nodes are leaked in this test.
>
> Done.
>
> http://codereview.chromium.org/3017023/show
>

[tfarina, 2010-08-05 16:00:36]

On 2010/08/05 15:41:48, sky wrote:
> Before I take another look. Can you run your test through valgrind to
> make sure you've caught them all?
> 
But I have ran. I sent to linux_valgrind bot.

[sky, 2010-08-05 22:55:01]

LGTM

http://codereview.chromium.org/3017023/diff/33001/34002
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/33001/34002#newcode164
app/tree_node_model_unittest.cc:164: delete child1;
move this before the assert so that if the test end sup failing for some reason
you still clean up memory.

[sky, 2010-08-05 22:55:57]

On Thu, Aug 5, 2010 at 9:00 AM,  <tfarina@chromium.org> wrote:
> On 2010/08/05 15:41:48, sky wrote:
>>
>> Before I take another look. Can you run your test through valgrind to
>> make sure you've caught them all?
>
> But I have ran. I sent to linux_valgrind bot.

Good. I just wanted to make sure since you had a lot of leaks initially.

  -Scott

[tfarina, 2010-08-05 23:38:22]

http://codereview.chromium.org/3017023/diff/33001/34002
File app/tree_node_model_unittest.cc (right):

http://codereview.chromium.org/3017023/diff/33001/34002#newcode164
app/tree_node_model_unittest.cc:164: delete child1;
On 2010/08/05 22:55:01, sky wrote:
> move this before the assert so that if the test end sup failing for some
reason
> you still clean up memory.

Done.