> On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1340 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1340> > > > > where is allocateRSSMemory and deallocateRSSMemory, I can't see them in > > this review or r30545
now in MemoryTestHelper. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1346 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1346> > > > > why do you need to deallocate? shouldn't everything get cleaned up when > > the test completes? > > > > I assume that each test is independent...? MemoryTestHelper doesn't have it anymore. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1364 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1364> > > > > +1 the test should be run in a temporary directory that always gets > > cleaned up in tear down, see TemporaryDirectoryTest fixed in the previous patch too; cgroup_tests now a TemporaryDirectoryTest > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1371 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1371> > > > > NULL pointer is converted to false, so ASSERT_TRUE(rss) should work doesn't apply anymore with the use of memory test helper. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1353 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1353> > > > > ditto doesn't apply anymore with the use of memory test helper. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1396 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1396> > > > > path doesn't get cleaned up. TemporaryDirectoryTest now. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, lines 1410-1411 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1410> > > > > generally it's cleaner, i.e., avoids extra indentation to structure as: > > > > if (pid == 0) { > > // do whatever in the child > > // exit or loop > > } > > > > // continue in parent fixed in memory test helper patch. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, line 1444 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1444> > > > > why keep trying here but not in the parent...? memory test helper used subprocess. > On March 24, 2015, 11:34 p.m., Ian Downes wrote: > > src/tests/cgroups_tests.cpp, lines 1343-1344 > > <https://reviews.apache.org/r/31012/diff/1/?file=863527#file863527line1343> > > > > (default - delta) < stat("rss") < (default + delta) > > > > is equivalent, and much more clearly expressed as: > > > > |stat - default| < delta revised the test now this is not needed. wasn't sure about the policy of using abs function from std libarary. - Chi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31012/#review77678 ----------------------------------------------------------- On April 1, 2015, 11:09 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31012/ > ----------------------------------------------------------- > > (Updated April 1, 2015, 11:09 p.m.) > > > Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. > > > Bugs: mesos-2136 > https://issues.apache.org/jira/browse/mesos-2136 > > > Repository: mesos > > > Description > ------- > > cgroups: added tests for memory statistics. > > > Diffs > ----- > > src/tests/cgroups_tests.cpp e18aed1feca182da89a117f81bed0897a00fb0ef > > Diff: https://reviews.apache.org/r/31012/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
