You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/10/24 22:23:54 UTC
Review Request 69146: Made fetcher tests more robust by using the test
sandbox.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Gilbert Song.
Repository: mesos
Description
-------
Fetcher tests currently rely on some hard-coded paths,
for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
Thus fetcher tests could fail if these directories already
exit. This patch makes use of the test sandboxes.
The above paths will be replaced by `fetch/` and `frameworks/`
under the sandbox.
Diffs
-----
src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
Diff: https://reviews.apache.org/r/69146/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210005
-----------------------------------------------------------
PASS: Mesos patch 69146 was successfully built and tested.
Reviews applied: `['69146']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2524/mesos-review-69146
- Mesos Reviewbot Windows
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210021
-----------------------------------------------------------
src/tests/fetcher_tests.cpp
Line 120 (original), 132 (patched)
<https://reviews.apache.org/r/69146/#comment294656>
How about doing this:
```
Fetcher fetcher(CreateSlaveFlags());
```
Then we can get rid of the `flags` local variable?
I'm not opening an issue since this is minor.
Ditto below.
- Chun-Hung Hsiao
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 11:54 p.m., Benjamin Bannier wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 98-109 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line98>
> >
> > We can avoid duplication here by making `FetcherTest` a `MesosTest` (that one would already today take care of `launcher_dir` and `fetcher_cache_dir`). This function would than at most need to invoke `MesosTest::CreateSlaveFlags` and make its non-general additions.
> >
> > It probably even make sense to set `frameworks_home` in `CreateSlaveFlags` as well. That would remove the need for this function completely.
Sounds good.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210022
-----------------------------------------------------------
On Oct. 25, 2018, 12:34 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 12:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
> src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995
>
>
> Diff: https://reviews.apache.org/r/69146/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210022
-----------------------------------------------------------
src/tests/fetcher_tests.cpp
Lines 98-109 (patched)
<https://reviews.apache.org/r/69146/#comment294657>
We can avoid duplication here by making `FetcherTest` a `MesosTest` (that one would already today take care of `launcher_dir` and `fetcher_cache_dir`). This function would than at most need to invoke `MesosTest::CreateSlaveFlags` and make its non-general additions.
It probably even make sense to set `frameworks_home` in `CreateSlaveFlags` as well. That would remove the need for this function completely.
- Benjamin Bannier
On Oct. 25, 2018, 1:12 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 1:12 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210055
-----------------------------------------------------------
PASS: Mesos patch 69146 was successfully built and tested.
Reviews applied: `['69146']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2530/mesos-review-69146
- Mesos Reviewbot Windows
On Oct. 25, 2018, 7:34 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 7:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
> src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995
>
>
> Diff: https://reviews.apache.org/r/69146/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210143
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Oct. 25, 2018, 12:34 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 12:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
> src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995
>
>
> Diff: https://reviews.apache.org/r/69146/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/
-----------------------------------------------------------
(Updated Oct. 25, 2018, 12:34 p.m.)
Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
Changes
-------
Addressed review comments.
Repository: mesos
Description (updated)
-------
Fetcher tests currently rely on some hard-coded paths,
for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
Thus fetcher tests could fail if these directories already
exit. This patch makes `FetcherTest` a subclass of
`MesosTest` which has a `createSlaveFlags()` helper to
utilize the test sandboxes for related directories to
avoid interference. The above paths will be
replaced by `fetch/` and `frameworks/` under the sandbox.
Diffs (updated)
-----
src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995
Diff: https://reviews.apache.org/r/69146/diff/3/
Changes: https://reviews.apache.org/r/69146/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 4:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
>
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
For the second part, I think using two `CHECK_NOTNONE` is more readable to me. Otherwise, we will end up with two unguarded `get()` (I am a little bit allergic to unguarded `get()` :) )
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 4:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 4:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 4:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
>
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
>
> Meng Zhu wrote:
> For the second part, I think using two `CHECK_NOTNONE` is more readable to me. Otherwise, we will end up with two unguarded `get()` (I am a little bit allergic to unguarded `get()` :) )
>
> Till Toenshoff wrote:
> How about
> ```
> CHECK_NOTNONE(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
> ```
I am not sure why it is preferred over the current code. I view `CHECK_NOTNONE` more as a safe "get()" operation rather than a pure check.
If we want to save one check, we can do
```
ASSERT_SOME(sandbox);
flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
flags.frameworks_home = path::join(sandbox.get(), "frameworks");
```
However, the reason I prefer inline `CHECK_NOTNONE` is that it removes order dependencies between statements. Otherwise when we do `get()`, we need to carry the mental burden that it has to be done after the `ASSERT`.
I feel inline `CHECK_NOTNONE` is both easy to maintain (no order dependency) and read (clearly express the intent of "can't be none" inline).
I personally prefer to never use naked `get()` given `CHECK_NOTNONE`.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 4:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 4:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Oct. 24, 2018, 11:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
>
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
>
> Meng Zhu wrote:
> For the second part, I think using two `CHECK_NOTNONE` is more readable to me. Otherwise, we will end up with two unguarded `get()` (I am a little bit allergic to unguarded `get()` :) )
>
> Till Toenshoff wrote:
> How about
> ```
> CHECK_NOTNONE(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
> ```
>
> Meng Zhu wrote:
> I am not sure why it is preferred over the current code. I view `CHECK_NOTNONE` more as a safe "get()" operation rather than a pure check.
>
> If we want to save one check, we can do
>
> ```
> ASSERT_SOME(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
>
> ```
>
> However, the reason I prefer inline `CHECK_NOTNONE` is that it removes order dependencies between statements. Otherwise when we do `get()`, we need to carry the mental burden that it has to be done after the `ASSERT`.
>
> I feel inline `CHECK_NOTNONE` is both easy to maintain (no order dependency) and read (clearly express the intent of "can't be none" inline).
>
> I personally prefer to never use naked `get()` given `CHECK_NOTNONE`.
Note that `Option::get` already has an assertion. The purpose of `CHECK_NOTNONE` to me is just to keep the line number so the check failure can be more easily located.
In general I'd prefer avoiding the `CHECK*` macros in tests as it would cause the test program to crash, thus no test cleanup would be performed. So an `ASSERT_SOME` is "safer" in this sense. That said, this check do seems a bit unnecessary because it's (practically) guaranteed that it is not none. Also you cannot use `ASSERT_*` in a non-void function. You have to do things like this: https://github.com/apache/mesos/blob/master/src/tests/storage_local_resource_provider_tests.cpp#L299-L301
And this limitation makes me incline to not do the assertion a bit.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
> On Oct. 24, 2018, 11:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
>
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
>
> Meng Zhu wrote:
> For the second part, I think using two `CHECK_NOTNONE` is more readable to me. Otherwise, we will end up with two unguarded `get()` (I am a little bit allergic to unguarded `get()` :) )
How about
```
CHECK_NOTNONE(sandbox);
flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
flags.frameworks_home = path::join(sandbox.get(), "frameworks");
```
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
> On Oct. 24, 2018, 11:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Oct. 24, 2018, 4:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
A good chunk of tests have the following pattern:
```
path::join(sandbox.get(), ...);
```
I've been leaning towards this because it forgoes the syscall.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 24, 2018, 4:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 4:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Oct. 24, 2018, 4:31 p.m., Till Toenshoff wrote:
> > src/tests/fetcher_tests.cpp
> > Lines 105-106 (patched)
> > <https://reviews.apache.org/r/69146/diff/2/?file=2102314#file2102314line105>
> >
> > While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
> > Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
> > ```
> > path::join(os::getcwd(), "fetcher");
> > path::join(os::getcwd(), "frameworks");
> > ```
> > would be consistent.
> >
> > Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
>
> Joseph Wu wrote:
> A good chunk of tests have the following pattern:
> ```
> path::join(sandbox.get(), ...);
> ```
>
> I've been leaning towards this because it forgoes the syscall.
>
> Till Toenshoff wrote:
> Definitely more efficient, yes - wish we could "fix" that in one swoop - but am not attached to the first part of my comment due to these reasons.
>
> Meng Zhu wrote:
> For the second part, I think using two `CHECK_NOTNONE` is more readable to me. Otherwise, we will end up with two unguarded `get()` (I am a little bit allergic to unguarded `get()` :) )
>
> Till Toenshoff wrote:
> How about
> ```
> CHECK_NOTNONE(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
> ```
>
> Meng Zhu wrote:
> I am not sure why it is preferred over the current code. I view `CHECK_NOTNONE` more as a safe "get()" operation rather than a pure check.
>
> If we want to save one check, we can do
>
> ```
> ASSERT_SOME(sandbox);
> flags.fetcher_cache_dir = path::join(sandbox.get(), "fetcher");
> flags.frameworks_home = path::join(sandbox.get(), "frameworks");
>
> ```
>
> However, the reason I prefer inline `CHECK_NOTNONE` is that it removes order dependencies between statements. Otherwise when we do `get()`, we need to carry the mental burden that it has to be done after the `ASSERT`.
>
> I feel inline `CHECK_NOTNONE` is both easy to maintain (no order dependency) and read (clearly express the intent of "can't be none" inline).
>
> I personally prefer to never use naked `get()` given `CHECK_NOTNONE`.
>
> Chun-Hung Hsiao wrote:
> Note that `Option::get` already has an assertion. The purpose of `CHECK_NOTNONE` to me is just to keep the line number so the check failure can be more easily located.
>
> In general I'd prefer avoiding the `CHECK*` macros in tests as it would cause the test program to crash, thus no test cleanup would be performed. So an `ASSERT_SOME` is "safer" in this sense. That said, this check do seems a bit unnecessary because it's (practically) guaranteed that it is not none. Also you cannot use `ASSERT_*` in a non-void function. You have to do things like this: https://github.com/apache/mesos/blob/master/src/tests/storage_local_resource_provider_tests.cpp#L299-L301
> And this limitation makes me incline to not do the assertion a bit.
Followed Benjamin's suggestion to use MesosTest which already has a `createSlaveFlag()` function. Using naked `get()` for consistency. Dropping this issue.
But it has been a good discussion. Unlike std assert, `CHECK_NOTNONE()` utilizes the glog interface, so the error log message will be more consistent, with timestamp and etc. In addition, I take `CHECK_NOTNONE` as an inline expression of intention -- the programmer assumes it cannot be NONE, not that she forgot to check.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
On Oct. 25, 2018, 12:34 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2018, 12:34 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch makes `FetcherTest` a subclass of
> `MesosTest` which has a `createSlaveFlags()` helper to
> utilize the test sandboxes for related directories to
> avoid interference. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
> src/tests/mesos.cpp c0ab2f7d86c4059f08e730d29456e6cc84c85995
>
>
> Diff: https://reviews.apache.org/r/69146/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Till Toenshoff via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/#review210000
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/fetcher_tests.cpp
Lines 105-106 (patched)
<https://reviews.apache.org/r/69146/#comment294639>
While this solution appears to be the most efficient it also is not entirely consistent with how we do it in other places;
Note that `getcwd()` returns the test-sandbox directory after the fixture did its job.
```
path::join(os::getcwd(), "fetcher");
path::join(os::getcwd(), "frameworks");
```
would be consistent.
Alternatively, we could at least do away with one `CHECK_NOTNONE` and pull it in front of these two uses of `sandbox`.
- Till Toenshoff
On Oct. 24, 2018, 11:12 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69146/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:12 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fetcher tests currently rely on some hard-coded paths,
> for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
> Thus fetcher tests could fail if these directories already
> exit. This patch adds a helper function `createSlaveFlags()`
> to make use of the test sandboxes. The above paths will be
> replaced by `fetch/` and `frameworks/` under the sandbox.
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
>
>
> Diff: https://reviews.apache.org/r/69146/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69146: Made fetcher tests more robust by using the
test sandbox.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69146/
-----------------------------------------------------------
(Updated Oct. 24, 2018, 4:12 p.m.)
Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gilbert Song, and Till Toenshoff.
Changes
-------
Added a helper function `CreateSlaveFlags()`.
Repository: mesos
Description (updated)
-------
Fetcher tests currently rely on some hard-coded paths,
for example, `/tmp/mesos/fetcher` and `/tmp/frameworks`.
Thus fetcher tests could fail if these directories already
exit. This patch adds a helper function `createSlaveFlags()`
to make use of the test sandboxes. The above paths will be
replaced by `fetch/` and `frameworks/` under the sandbox.
Diffs (updated)
-----
src/tests/fetcher_tests.cpp 283238cdda17a94e034baa195bd9d4b57e363b8a
Diff: https://reviews.apache.org/r/69146/diff/2/
Changes: https://reviews.apache.org/r/69146/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu