Check in to run unittests that uses src_test to build and run unittests.

Check in to run unittests that uses src_test to build and run unittests.

TEST=Ran unittests with 2 test failures (powerd and update engine).
Will check with authors to fix these before moving the default over
to this.

[sosa, 2010-06-15 17:13:18]

A little more information about what unittests will be run:

This script finds all packages that are in the dependency tree of the base
chrome os image, filters this list on packages in chromeos-base and then
builds/runs the unittests specified by the src_test stanza of the corresponding
ebuild.

[petkov, 2010-06-15 23:14:54]

Mostly nits...

http://codereview.chromium.org/2837004/diff/8001/9001
File cros_run_unittests (right):

http://codereview.chromium.org/2837004/diff/8001/9001#newcode7
cros_run_unittests:7: # This script builds and runs chromiumos unittests.  Note
that this script
unit tests should be two words, no? throughout...

http://codereview.chromium.org/2837004/diff/8001/9001#newcode18
cros_run_unittests:18: DEFINE_string build_root "${DEFAULT_BUILD_ROOT}" \
nit: keep flags sorted?

http://codereview.chromium.org/2837004/diff/8001/9001#newcode29
cros_run_unittests:29: echo Error: --board required
Use the "error" function from common.sh instead.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode35
cros_run_unittests:35: function check_src_test() {
You're mixing function declarations with code. Can you move the functions up?

http://codereview.chromium.org/2837004/diff/8001/9001#newcode36
cros_run_unittests:36: grep src_test "${1}" > /dev/null
Maybe use grep -w to look for src_test as a word. Or,

egrep '^src_test() {'

http://codereview.chromium.org/2837004/diff/8001/9001#newcode55
cros_run_unittests:55: # List of packages that with unittest failures.
that with?

http://codereview.chromium.org/2837004/diff/8001/9001#newcode73
cros_run_unittests:73: warn "The following packages have no unittests
${NO_UNITTESTS}"
Add colon after unit tests:
Maybe also a new line?

http://codereview.chromium.org/2837004/diff/8001/9001#newcode77
cros_run_unittests:77: warn "Run completed with ${TEST_FAILURES}/${TEST_COUNT}
test failures"
period at the end.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode79
cros_run_unittests:79: warn "${FAILED_PACKAGES}"
Maybe switch this warn to error?

http://codereview.chromium.org/2837004/diff/8001/9001#newcode82
cros_run_unittests:82: echo "All unittests passed."
We should probably add an "info" routine to common.sh to show stuff in green :-)

[sosa, 2010-06-16 17:34:40]

Changed name of script to unit_tests, addressed comments and added a
-p[packages] to the script to make it parametrizable i.e. you can run:

./cros_run_unit_tests -p metrics

http://codereview.chromium.org/2837004/diff/8001/9001
File cros_run_unittests (right):

http://codereview.chromium.org/2837004/diff/8001/9001#newcode7
cros_run_unittests:7: # This script builds and runs chromiumos unittests.  Note
that this script
On 2010/06/15 23:14:56, petkov wrote:
> unit tests should be two words, no? throughout...
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode18
cros_run_unittests:18: DEFINE_string build_root "${DEFAULT_BUILD_ROOT}" \
On 2010/06/15 23:14:56, petkov wrote:
> nit: keep flags sorted?
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode29
cros_run_unittests:29: echo Error: --board required
On 2010/06/15 23:14:56, petkov wrote:
> Use the "error" function from common.sh instead.
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode35
cros_run_unittests:35: function check_src_test() {
On 2010/06/15 23:14:56, petkov wrote:
> You're mixing function declarations with code. Can you move the functions up?
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode36
cros_run_unittests:36: grep src_test "${1}" > /dev/null
On 2010/06/15 23:14:56, petkov wrote:
> Maybe use grep -w to look for src_test as a word. Or,
> 
> egrep '^src_test() {'
> 
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode55
cros_run_unittests:55: # List of packages that with unittest failures.
On 2010/06/15 23:14:56, petkov wrote:
> that with?
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode73
cros_run_unittests:73: warn "The following packages have no unittests
${NO_UNITTESTS}"
On 2010/06/15 23:14:56, petkov wrote:
> Add colon after unit tests:
> Maybe also a new line?
> 

Done.

http://codereview.chromium.org/2837004/diff/8001/9001#newcode82
cros_run_unittests:82: echo "All unittests passed."
On 2010/06/15 23:14:56, petkov wrote:
> We should probably add an "info" routine to common.sh to show stuff in green
:-)
> 

Done.

[petkov, 2010-06-16 18:23:38]

A few more nits -- sorry for not noticing most of these the first time around.

http://codereview.chromium.org/2837004/diff/15001/16001
File common.sh (right):

http://codereview.chromium.org/2837004/diff/15001/16001#newcode306
common.sh:306: V_GREEN="\e[32m"
reorder: 31, 32, 33... Or keep the color names sorted.

http://codereview.chromium.org/2837004/diff/15001/16001#newcode311
common.sh:311: echo -e "${V_GREEN}INFO: $1${V_VIDOFF}"
Can you actually see the message if it's not bold?

Also, I got screamed at when I changed these routines and didn't align the colon
across messages. So, please add spaces after "INFO" to align the ":" with the
warn's and error's ":" (in the actual output).

http://codereview.chromium.org/2837004/diff/15001/16002
File cros_run_unit_tests (right):

http://codereview.chromium.org/2837004/diff/15001/16002#newcode7
cros_run_unit_tests:7: # This script builds and runs chromiumos unit tests. 
Note that this script
s/chromiumos/Chromium OS/

http://codereview.chromium.org/2837004/diff/15001/16002#newcode22
cros_run_unit_tests:22: DEFINE_string packages "" "Optional list of packages to
run unit tests" p
space separated list?

http://codereview.chromium.org/2837004/diff/15001/16002#newcode53
cros_run_unit_tests:53: error "--board required"
Ah, actually you want "die" here -- error doesn't exit.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode63
cros_run_unit_tests:63: PACKAGE_LIST=$( ./get_package_list chromeos | grep
chromeos-base )
egrep '^chromeos-base/'

http://codereview.chromium.org/2837004/diff/15001/16002#newcode84
cros_run_unit_tests:84: warn "Run completed with ${TEST_FAILURES}/${TEST_COUNT}
test failures."
s/warn/error/

http://codereview.chromium.org/2837004/diff/15001/16002#newcode85
cros_run_unit_tests:85: warn "Following packages had failing tests:"
s/warn/error/

http://codereview.chromium.org/2837004/diff/15001/16002#newcode86
cros_run_unit_tests:86: error "${FAILED_PACKAGES}"
s/error/die/

[sosa, 2010-06-16 21:01:51]

Addressed your comments.  Also checked for extra parameters so users aren't
confused why cros_run_unit_tests metrics runs all the tests (need to use -p)

http://codereview.chromium.org/2837004/diff/15001/16002
File cros_run_unit_tests (right):

http://codereview.chromium.org/2837004/diff/15001/16002#newcode7
cros_run_unit_tests:7: # This script builds and runs chromiumos unit tests. 
Note that this script
On 2010/06/16 18:23:39, petkov wrote:
> s/chromiumos/Chromium OS/
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode22
cros_run_unit_tests:22: DEFINE_string packages "" "Optional list of packages to
run unit tests" p
On 2010/06/16 18:23:39, petkov wrote:
> space separated list?
> 

da

http://codereview.chromium.org/2837004/diff/15001/16002#newcode53
cros_run_unit_tests:53: error "--board required"
On 2010/06/16 18:23:39, petkov wrote:
> Ah, actually you want "die" here -- error doesn't exit.
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode63
cros_run_unit_tests:63: PACKAGE_LIST=$( ./get_package_list chromeos | grep
chromeos-base )
On 2010/06/16 18:23:39, petkov wrote:
> egrep '^chromeos-base/'
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode84
cros_run_unit_tests:84: warn "Run completed with ${TEST_FAILURES}/${TEST_COUNT}
test failures."
On 2010/06/16 18:23:39, petkov wrote:
> s/warn/error/
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode85
cros_run_unit_tests:85: warn "Following packages had failing tests:"
On 2010/06/16 18:23:39, petkov wrote:
> s/warn/error/
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16002#newcode86
cros_run_unit_tests:86: error "${FAILED_PACKAGES}"
On 2010/06/16 18:23:39, petkov wrote:
> s/error/die/
> 

Done.

[sosa, 2010-06-16 21:11:54]

PTAL, common.sh addressed

http://codereview.chromium.org/2837004/diff/15001/16001
File common.sh (right):

http://codereview.chromium.org/2837004/diff/15001/16001#newcode306
common.sh:306: V_GREEN="\e[32m"
On 2010/06/16 18:23:39, petkov wrote:
> reorder: 31, 32, 33... Or keep the color names sorted.
> 
> 

Done.

http://codereview.chromium.org/2837004/diff/15001/16001#newcode311
common.sh:311: echo -e "${V_GREEN}INFO: $1${V_VIDOFF}"
On 2010/06/16 18:23:39, petkov wrote:
> Can you actually see the message if it's not bold?
> 
> Also, I got screamed at when I changed these routines and didn't align the
colon
> across messages. So, please add spaces after "INFO" to align the ":" with the
> warn's and error's ":" (in the actual output).
> 

Done.

[petkov, 2010-06-16 21:16:16]

LGTM

[kmixter1, 2010-06-16 22:52:22]

LGTM

On Wed, Jun 16, 2010 at 2:16 PM,  <petkov@chromium.org> wrote:
> LGTM
>
>
> http://codereview.chromium.org/2837004/show
>