You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/06/20 21:14:13 UTC
Review Request 35697: mesos: Fixed test names.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35697/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Till Toenshoff.
Repository: mesos
Description
-------
First noticed `JsonTest.parse`, then found the other cases with the following regex: `TEST.*(.*, [a-z].*`.
Diffs
-----
src/tests/cram_md5_authentication_tests.cpp 992302365682df2c800c8828a21b9d38cb318472
src/tests/credentials_tests.cpp ed672733ff895b1f252fa75d5653a9f3527d5b5f
src/tests/registrar_tests.cpp 97d0b685dd2744cf92b3a463d586e8bcf4c0af1a
Diff: https://reviews.apache.org/r/35697/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 35697: mesos: Fixed test names.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35697/#review88808
-----------------------------------------------------------
Ship it!
Thanks a bunch for these.
- Till Toenshoff
On June 20, 2015, 7:14 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35697/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First noticed `JsonTest.parse`, then found the other cases with the following regex: `TEST.*(.*, [a-z].*`.
>
>
> Diffs
> -----
>
> src/tests/cram_md5_authentication_tests.cpp 992302365682df2c800c8828a21b9d38cb318472
> src/tests/credentials_tests.cpp ed672733ff895b1f252fa75d5653a9f3527d5b5f
> src/tests/registrar_tests.cpp 97d0b685dd2744cf92b3a463d586e8bcf4c0af1a
>
> Diff: https://reviews.apache.org/r/35697/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35697: mesos: Fixed test names.
Posted by Michael Park <mc...@gmail.com>.
> On June 22, 2015, 8:12 p.m., Ben Mahler wrote:
> > Thanks, will commit this for you.
> >
> > Also we are inconcistent on the "Test" suffix, e.g. "MasterTest" vs "CRAMMD5Authentication". See two options:
> >
> > (1) We can use "Test" to signify that there is a fixture, if that's helpful. So remove any "Test" suffixes from non-fixture tests.
> > (2) We can use "Test" as a suffix for all tests.
> >
> > Thoughts?
Ah, that's a good catch Ben!
(1) I think whether the test is a fixture or not is already clear from the use of `TEST_F` as opposed to `TEST`. So I don't think it's useful to have the `Test` suffix to signify fixtures.
(2) The fact that it's a test is obvious from `TEST`, `TEST_F`, so I don't think there's a benefit of having the `Test` suffix for all tests either. However, most of our tests have the `Test` suffix, and removing all of those would touch pretty much all of the tests. So I would vote for having the `Test` suffix for all tests.
There are currently 136 instances of tests without the `Test` suffix. `grep --exclude=./src/tests/script.hpp --exclude=./src/webui/master/static/js/jquery-1.7.1.min.js --exclude-dir=./build -r "TEST.*(.*," . | grep -v "TEST.*(.*Test,"`
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35697/#review88840
-----------------------------------------------------------
On June 20, 2015, 7:14 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35697/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First noticed `JsonTest.parse`, then found the other cases with the following regex: `TEST.*(.*, [a-z].*`.
>
>
> Diffs
> -----
>
> src/tests/cram_md5_authentication_tests.cpp 992302365682df2c800c8828a21b9d38cb318472
> src/tests/credentials_tests.cpp ed672733ff895b1f252fa75d5653a9f3527d5b5f
> src/tests/registrar_tests.cpp 97d0b685dd2744cf92b3a463d586e8bcf4c0af1a
>
> Diff: https://reviews.apache.org/r/35697/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35697: mesos: Fixed test names.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35697/#review88840
-----------------------------------------------------------
Ship it!
Thanks, will commit this for you.
Also we are inconcistent on the "Test" suffix, e.g. "MasterTest" vs "CRAMMD5Authentication". See two options:
(1) We can use "Test" to signify that there is a fixture, if that's helpful. So remove any "Test" suffixes from non-fixture tests.
(2) We can use "Test" as a suffix for all tests.
Thoughts?
- Ben Mahler
On June 20, 2015, 7:14 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35697/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First noticed `JsonTest.parse`, then found the other cases with the following regex: `TEST.*(.*, [a-z].*`.
>
>
> Diffs
> -----
>
> src/tests/cram_md5_authentication_tests.cpp 992302365682df2c800c8828a21b9d38cb318472
> src/tests/credentials_tests.cpp ed672733ff895b1f252fa75d5653a9f3527d5b5f
> src/tests/registrar_tests.cpp 97d0b685dd2744cf92b3a463d586e8bcf4c0af1a
>
> Diff: https://reviews.apache.org/r/35697/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35697: mesos: Fixed test names.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35697/#review88682
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35697]
All tests passed.
- Mesos ReviewBot
On June 20, 2015, 7:14 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35697/
> -----------------------------------------------------------
>
> (Updated June 20, 2015, 7:14 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> First noticed `JsonTest.parse`, then found the other cases with the following regex: `TEST.*(.*, [a-z].*`.
>
>
> Diffs
> -----
>
> src/tests/cram_md5_authentication_tests.cpp 992302365682df2c800c8828a21b9d38cb318472
> src/tests/credentials_tests.cpp ed672733ff895b1f252fa75d5653a9f3527d5b5f
> src/tests/registrar_tests.cpp 97d0b685dd2744cf92b3a463d586e8bcf4c0af1a
>
> Diff: https://reviews.apache.org/r/35697/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>