Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
502009:
add script to drive setup and execution of autotest suites from dev environment (Closed)
Descriptionadd script to drive setup and execution of autotest suites from dev environment
Patch Set 1 #Patch Set 2 : Adds script to drive setup and execution of autotest suites from dev environment #
Total comments: 32
Patch Set 3 : Addressing code review comments #
Total comments: 6
Patch Set 4 : address comments, typos #Messages
Total messages: 8 (0 generated)
Still not sure about the ssh-agent stuff but here're the rest of the comments. http://codereview.chromium.org/502009/diff/1001/2001 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/1001/2001#newcode19 src/scripts/setup_autotest.sh:19: -c path to control file to run (defaults to AllTests) Clarify that -c refers to client-side control file. http://codereview.chromium.org/502009/diff/1001/2001#newcode20 src/scripts/setup_autotest.sh:20: -m machine address I believe autoserv allows here "list of machines". We don't run anything if -m is not specified, right? http://codereview.chromium.org/502009/diff/1001/2001#newcode21 src/scripts/setup_autotest.sh:21: -? Print usage help Use -h instead for consistency with the rest of the autotest scripts? http://codereview.chromium.org/502009/diff/1001/2001#newcode26 src/scripts/setup_autotest.sh:26: CONTROL_FILE="" CLIENT_CONTROL_FILE? http://codereview.chromium.org/502009/diff/1001/2001#newcode27 src/scripts/setup_autotest.sh:27: while getopts "c:m:r?" OPTVAR Why don't you use FLAGS (from third_party/shflags) for option parsing to be consistent with the rest of the chromium-os dev scripts? http://codereview.chromium.org/502009/diff/1001/2001#newcode47 src/scripts/setup_autotest.sh:47: AUTOTEST_SRC="../third_party/autotest/files" Use ${GCLIENT_ROOT}/src/third_part/autotest/files instead. http://codereview.chromium.org/502009/diff/1001/2001#newcode51 src/scripts/setup_autotest.sh:51: if [ "${TEST_AUTH_SOCKET:0:27}" == "/tmp/chromiumos_test_agent." ] Put the file location prefix in a variable and reuse later? Then maybe use ${#name} to parametrize the prefix extraction? http://codereview.chromium.org/502009/diff/1001/2001#newcode54 src/scripts/setup_autotest.sh:54: kill ${SSH_AGENT_PID} Also, rm -f $TEST_AUTH_SOCKET? http://codereview.chromium.org/502009/diff/1001/2001#newcode62 src/scripts/setup_autotest.sh:62: if [ ! -f "${AUTOTEST_CHROOT_DEST}/server/autoserv" ] Hmm, so if autotest sources change, one would need to manually remove the installed autotest in order for the copy to kick in? For now it's fine, but probably add a TODO: we could add an option to force the copy or, maybe better, add rsync to the chroot developer environment and you rsync to always install. http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ Use ${GCLIENT_ROOT}? http://codereview.chromium.org/502009/diff/1001/2001#newcode87 src/scripts/setup_autotest.sh:87: /usr/bin/ssh-add ../platform/testing/testing_rsa 2> /dev/null Put the testing_rsa file location in a variable, use ${GCLIENT_ROOT} and reuse the variable? http://codereview.chromium.org/502009/diff/1001/2001#newcode89 src/scripts/setup_autotest.sh:89: if [ -n "${MACHINE}" ] If -m is not specified, maybe we should print where autotest is installed, a sample autoserv command-line, the location of the root private key. http://codereview.chromium.org/502009/diff/1001/2001#newcode101 src/scripts/setup_autotest.sh:101: echo "running autosrv: " ${autosrv_cmd} Nit: you're actually running autoserv.
One more comment. http://codereview.chromium.org/502009/diff/1001/2001 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ Given that your test refactoring change is in, copy both client_tests to client/site_tests and server_tests to server/site_tests. And do we need to sudo for installing autotest -- both here and above for the main autotest code?
http://codereview.chromium.org/502009/diff/1001/2001 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ nit: 80 chars here and below. http://codereview.chromium.org/502009/diff/1001/2001#newcode91 src/scripts/setup_autotest.sh:91: # run only a specific test/suite if requested indentation http://codereview.chromium.org/502009/diff/1001/2001#newcode95 src/scripts/setup_autotest.sh:95: CONTROL_FILE="${AUTOTEST_CHROOT_DEST}/client/site_tests/AllTests/control" Where is this generated?
Addressed comments. http://codereview.chromium.org/502009/diff/1001/2001 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/1001/2001#newcode20 src/scripts/setup_autotest.sh:20: -m machine address On 2009/12/15 19:21:22, petkov wrote: > I believe autoserv allows here "list of machines". > We don't run anything if -m is not specified, right? > That's right, -m is passed through directly to autotest (I want to maintain similar flags UI) Usage string updated. http://codereview.chromium.org/502009/diff/1001/2001#newcode26 src/scripts/setup_autotest.sh:26: CONTROL_FILE="" On 2009/12/15 19:21:22, petkov wrote: > CLIENT_CONTROL_FILE? Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode27 src/scripts/setup_autotest.sh:27: while getopts "c:m:r?" OPTVAR On 2009/12/15 19:21:22, petkov wrote: > Why don't you use FLAGS (from third_party/shflags) for option parsing to be > consistent with the rest of the chromium-os dev scripts? Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode47 src/scripts/setup_autotest.sh:47: AUTOTEST_SRC="../third_party/autotest/files" On 2009/12/15 19:21:22, petkov wrote: > Use ${GCLIENT_ROOT}/src/third_part/autotest/files instead. > Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode51 src/scripts/setup_autotest.sh:51: if [ "${TEST_AUTH_SOCKET:0:27}" == "/tmp/chromiumos_test_agent." ] On 2009/12/15 19:21:22, petkov wrote: > Put the file location prefix in a variable and reuse later? Then maybe use > ${#name} to parametrize the prefix extraction? Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode54 src/scripts/setup_autotest.sh:54: kill ${SSH_AGENT_PID} On 2009/12/15 19:21:22, petkov wrote: > Also, rm -f $TEST_AUTH_SOCKET? ssh-agent -k takes care of that. http://codereview.chromium.org/502009/diff/1001/2001#newcode62 src/scripts/setup_autotest.sh:62: if [ ! -f "${AUTOTEST_CHROOT_DEST}/server/autoserv" ] On 2009/12/15 19:21:22, petkov wrote: > Hmm, so if autotest sources change, one would need to manually remove the > installed autotest in order for the copy to kick in? For now it's fine, but > probably add a TODO: we could add an option to force the copy or, maybe better, > add rsync to the chroot developer environment and you rsync to always install. Noted, and added a flag -f, force reinstallation. http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ On 2009/12/15 19:21:22, petkov wrote: > Use ${GCLIENT_ROOT}? Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ On 2009/12/17 00:53:44, kmixter1 wrote: > nit: 80 chars here and below. Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode74 src/scripts/setup_autotest.sh:74: sudo cp -rp ../platform/testing/systemtests/* ${AUTOTEST_CHROOT_DEST}/client/site_tests/ On 2009/12/15 19:35:50, petkov wrote: > Given that your test refactoring change is in, copy both client_tests to > client/site_tests and server_tests to server/site_tests. And do we need to sudo > for installing autotest -- both here and above for the main autotest code? Only sudo because /usr/local isn't world writeable. We could install it into /home, actually. http://codereview.chromium.org/502009/diff/1001/2001#newcode87 src/scripts/setup_autotest.sh:87: /usr/bin/ssh-add ../platform/testing/testing_rsa 2> /dev/null On 2009/12/15 19:21:22, petkov wrote: > Put the testing_rsa file location in a variable, use ${GCLIENT_ROOT} and reuse > the variable? Made it a flag, in case someone likes to use their own root key on their test device. http://codereview.chromium.org/502009/diff/1001/2001#newcode89 src/scripts/setup_autotest.sh:89: if [ -n "${MACHINE}" ] On 2009/12/15 19:21:22, petkov wrote: > If -m is not specified, maybe we should print where autotest is installed, a > sample autoserv command-line, the location of the root private key. Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode91 src/scripts/setup_autotest.sh:91: # run only a specific test/suite if requested On 2009/12/17 00:53:44, kmixter1 wrote: > indentation Done. http://codereview.chromium.org/502009/diff/1001/2001#newcode95 src/scripts/setup_autotest.sh:95: CONTROL_FILE="${AUTOTEST_CHROOT_DEST}/client/site_tests/AllTests/control" On 2009/12/17 00:53:44, kmixter1 wrote: > Where is this generated? We should generate this here, but will leave it out for now. http://codereview.chromium.org/502009/diff/1001/2001#newcode101 src/scripts/setup_autotest.sh:101: echo "running autosrv: " ${autosrv_cmd} On 2009/12/15 19:21:22, petkov wrote: > Nit: you're actually running autoserv. Done.
LGTM if you address the comments below. It's a good start, we'll need to butcher it later though. http://codereview.chromium.org/502009/diff/5002/5003 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/5002/5003#newcode14 src/scripts/setup_autotest.sh:14: DEFINE_string client_control "client test case to execute" "c" Missing default. http://codereview.chromium.org/502009/diff/5002/5003#newcode17 src/scripts/setup_autotest.sh:17: DEFINE_string test_key "${GCLIENT_ROOT}/src/platform/testing/testing_rsa" "k" Missing description. http://codereview.chromium.org/502009/diff/5002/5003#newcode83 src/scripts/setup_autotest.sh:83: /client/site_tests/AllTests/control" Can you break the line after = and preserve the indentation?
Addressed comments. Submitting. http://codereview.chromium.org/502009/diff/5002/5003 File src/scripts/setup_autotest.sh (right): http://codereview.chromium.org/502009/diff/5002/5003#newcode14 src/scripts/setup_autotest.sh:14: DEFINE_string client_control "client test case to execute" "c" On 2009/12/18 17:55:40, petkov wrote: > Missing default. Done. http://codereview.chromium.org/502009/diff/5002/5003#newcode17 src/scripts/setup_autotest.sh:17: DEFINE_string test_key "${GCLIENT_ROOT}/src/platform/testing/testing_rsa" "k" On 2009/12/18 17:55:40, petkov wrote: > Missing description. > Done. http://codereview.chromium.org/502009/diff/5002/5003#newcode83 src/scripts/setup_autotest.sh:83: /client/site_tests/AllTests/control" On 2009/12/18 17:55:40, petkov wrote: > Can you break the line after = and preserve the indentation? Done. |
