You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/06/18 20:19:23 UTC
Review Request: A gtest hack to support selective testing based on the
runtime system information.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5393/
-----------------------------------------------------------
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
Re: Review Request: A gtest hack to support selective testing based on the
runtime system information.
Posted by Jie Yu <yu...@gmail.com>.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 48
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line48>
> >
> > You don't need to be explicit for functions that return a boolean (i.e., just use !).
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 43
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line43>
> >
> > 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.
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 54
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line54>
> >
> > 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.
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 77
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line77>
> >
> > I think you should update the comment that gtest _expects_ the filters to be positive followed by negative.
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, lines 79-92
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line79>
> >
> > 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 = "*";
> > }
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 104
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line104>
> >
> > 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.
Done.
> On June 19, 2012, 1:42 a.m., Benjamin Hindman wrote:
> > src/tests/main.cpp, line 119
> > <https://reviews.apache.org/r/5393/diff/1/?file=111879#file111879line119>
> >
> > Given my above comment, we won't need to set this.
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5393/#review8372
-----------------------------------------------------------
On June 19, 2012, 4:03 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5393/
> -----------------------------------------------------------
>
> (Updated June 19, 2012, 4:03 a.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
>
>
Re: Review Request: A gtest hack to support selective testing based on the
runtime system information.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request: A gtest hack to support selective testing based on the
runtime system information.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5393/#review8398
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On June 19, 2012, 4:03 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5393/
> -----------------------------------------------------------
>
> (Updated June 19, 2012, 4:03 a.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
>
>
Re: Review Request: A gtest hack to support selective testing based on the
runtime system information.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5393/
-----------------------------------------------------------
(Updated June 19, 2012, 4:03 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Addressed Ben's review feedback.
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 (updated)
-----
src/tests/main.cpp 8593f4a
Diff: https://reviews.apache.org/r/5393/diff/
Testing
-------
Tested on Linux and Mac.
Thanks,
Jie Yu