----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5393/#review8372 -----------------------------------------------------------
src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18044> I think the naming here is insufficient. This is not just checking if the test has a "special" name, it's also determining whether or not the test can be run. How about something like: shouldRunTest. src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18045> You don't need to be explicit for functions that return a boolean (i.e., just use !). src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18046> I think maybe this check shouldn't be needed. The test should only be built on Linux anyway (thanks to the Makefile), so this check is probably redundant. src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18042> I think you should update the comment that gtest _expects_ the filters to be positive followed by negative. src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18043> I think using C++ strings here would be easier to read: const string filter = ::testing::GTEST_FLAG(filter); size_t index = filter.find('-'); string positive; string negative; if (index != string::npos) { positive = filter.substr(0, index); negative = filter.substr(index + 1); } else { positive = filter; } // Use the universal filter if no filter was specified. if (positive == "") { positive = "*"; } src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18047> I'm back peddling a bit on whether or not we want to set these tests as DISABLED. What about just using ROOT and CGROUPS and appending them to the negative filter if 'shouldRunTest' returns false? Here's my reasoning. We might actually have a ROOT test that we really want to disable for now. But we can't do DISABLE_DISABLE_ROOT_... So instead, I think we actually want to be able to DISABLE_ that test. src/tests/main.cpp <https://reviews.apache.org/r/5393/#comment18048> Given my above comment, we won't need to set this. - Benjamin Hindman On June 18, 2012, 6:19 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5393/ > ----------------------------------------------------------- > > (Updated June 18, 2012, 6:19 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Sometimes, you may want to enable a test only on Linux platforms, or enable a > test only if cgroups module is enabled. This patch addresses this issue. > > If you want to declare a test that only runs on Linux, you should set its > name to: DISABLED_LINUX_XXX. This test will only run on Linux platforms > (ignore the 'DISABLED' prefix). > > Similarly, if you want to declare a test that only runs when cgroups is > enabled, you should set its name to: DISABLED_CGROUPS_XXX. > > It is implemented by using the gtest filter mechanism. > > > Diffs > ----- > > src/tests/main.cpp 8593f4a > > Diff: https://reviews.apache.org/r/5393/diff/ > > > Testing > ------- > > Tested on Linux and Mac. > > > Thanks, > > Jie Yu > >
