Continue to refactor run_remote_tests script.

run_remote_tests.sh

 #!/bin/bash
 
 # Copyright (c) 2009 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Script to run client or server tests on a live remote image.
 
 # Load common constants.  This should be the first executable line.
 # The path to common.sh should be relative to your script's location.
@@ skipping 30 matching lines @@
   cleanup_remote_access
 }
 
 # Determine if a control is for a client or server test.  Echos
 # either "server" or "client".
 # Arguments:
 #   $1 - control file path
 function read_test_type() {
   local control_file=$1
   # Assume a line starts with TEST_TYPE =
-  local type=$(egrep -m1 \
-               '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${control_file}")
-  if [[ -z "${type}" ]]; then
+  local test_type=$(egrep -m1 \
+                    '^[[:space:]]*TEST_TYPE[[:space:]]*=' "${control_file}")
+  if [[ -z "${test_type}" ]]; then
     die "Unable to find TEST_TYPE line in ${control_file}"
   fi
-  type=$(python -c "${type}; print TEST_TYPE.lower()")
-  if [[ "${type}" != "client" ]] && [[ "${type}" != "server" ]]; then
-    die "Unknown type of test (${type}) in ${control_file}"
+  test_type=$(python -c "${test_type}; print TEST_TYPE.lower()")
+  if [[ "${test_type}" != "client" ]] && [[ "${test_type}" != "server" ]]; then
+    die "Unknown type of test (${test_type}) in ${control_file}"
   fi
-  echo ${type}
+  echo ${test_type}
 }
 
 function create_tmp() {
   # Set global TMP for remote_access.sh's sake
   # and if --results_dir_root is specified,
   # set TMP and create dir appropriately
-  if [[ ${INSIDE_CHROOT} -eq 0 ]]; then
-    if [[ -n "${FLAGS_results_dir_root}" ]]; then
-      TMP=${FLAGS_chroot}${FLAGS_results_dir_root}
-      mkdir -p -m 777 ${TMP}
-    else
-      TMP=$(mktemp -d ${FLAGS_chroot}/tmp/run_remote_tests.XXXX)
-    fi
-    TMP_INSIDE_CHROOT=$(echo ${TMP#${FLAGS_chroot}})
+  if [[ -n "${FLAGS_results_dir_root}" ]]; then
+    TMP=${FLAGS_results_dir_root}
+    mkdir -p -m 777 ${TMP}
   else
-    if [[ -n "${FLAGS_results_dir_root}" ]]; then
-      TMP=${FLAGS_results_dir_root}
-      mkdir -p -m 777 ${TMP}
-    else
-      TMP=$(mktemp -d /tmp/run_remote_tests.XXXX)
-    fi
-    TMP_INSIDE_CHROOT=${TMP}
+    TMP=$(mktemp -d /tmp/run_remote_tests.XXXX)
   fi
 }
 
-function prepare_build_dir() {
-  local autotest_dir="$1"
-  INSIDE_BUILD_DIR="${TMP_INSIDE_CHROOT}/build"
-  BUILD_DIR="${TMP}/build"
-  info "Copying autotest tree into ${BUILD_DIR}."
-  sudo mkdir -p "${BUILD_DIR}"
-  sudo rsync -rl --chmod=ugo=rwx "${autotest_dir}"/ "${BUILD_DIR}"
+function setup_build_env() {
   info "Pilfering toolchain shell environment from Portage."
-  local outside_ebuild_dir="${TMP}/chromeos-base/autotest-build"
-  local inside_ebuild_dir="${TMP_INSIDE_CHROOT}/chromeos-base/autotest-build"
-  mkdir -p "${outside_ebuild_dir}"
+  local ebuild_dir="${TMP}/chromeos-base/autotest-build"
+  mkdir -p "${ebuild_dir}"
   local E_only="autotest-build-9999.ebuild"
-  cat > "${outside_ebuild_dir}/${E_only}" <<EOF
+  cat > "${ebuild_dir}/${E_only}" <<EOF
 inherit toolchain-funcs
 SLOT="0"
 EOF
   local E="chromeos-base/autotest-build/${E_only}"
-  ${ENTER_CHROOT} "ebuild-${FLAGS_board}" "${inside_ebuild_dir}/${E_only}" \
-      clean unpack 2>&1 > /dev/null
-  local P_tmp="${FLAGS_chroot}/build/${FLAGS_board}/tmp/portage/"
+  "ebuild-${FLAGS_board}" "${ebuild_dir}/${E_only}" clean unpack 2>&1 \
+      > /dev/null
+  local P_tmp="/build/${FLAGS_board}/tmp/portage/"
   local E_dir="${E%%/*}/${E_only%.*}"
-  sudo cp "${P_tmp}/${E_dir}/temp/environment" "${BUILD_DIR}"
+  . "${P_tmp}/${E_dir}/temp/environment"
+  tc-export CC CXX PKG_CONFIG

[jrbarnette, 2010/11/23 23:16:04]

This sequence here seems a bit risky:  It pulls heaven knows what
variable settings into the current script with no guarantee that there
aren't conflicts that break the script.  It would probably be good to
delay this until just before invoking autoserv, and then put it in a
subshell for protection.

[ericli, 2010/11/24 19:20:57]

Done.

 }
 
 function autodetect_build() {
   if [ ${FLAGS_use_emerged} -eq ${FLAGS_TRUE} ]; then
-    info \
-"As requested, using emerged autotests already installed in your sysroot."
     FLAGS_build=${FLAGS_FALSE}
-    return
-  fi
-  if ${ENTER_CHROOT} ./cros_workon --board=${FLAGS_board} list | \
-    grep -q autotest; then
+  elif $(dirname $0)/cros_workon --board=${FLAGS_board} list | \

[jrbarnette, 2010/11/23 23:16:04]

nit on the pre-existing code:  the '\' here isn't needed, and the style
guidelines discourage it.

[ericli, 2010/11/24 19:20:57]

Done.

+       grep -q autotest; then
+    FLAGS_build=${FLAGS_TRUE}
     info \
 "Detected cros_workon autotests, building your sources instead of emerged \
-autotest.  To use installed autotest, pass --use_emerged."
-    FLAGS_build=${FLAGS_TRUE}
+autotest. To use installed autotest, pass --use_emerged."

[jrbarnette, 2010/11/23 23:16:04]

This informational message should only be printed if the script
overrides the default setting of FLAGS_build.  As is, it will print even
if the user already specified --build.

I think the logic needed is this:
  if not --build and not --use_emerged and "cros_workon autotest"
    info "Detected cros_workon ..."
    FLAGS_build=TRUE

[kmixter1, 2010/11/23 23:30:41]

Or more simply, add a elif case ${FLAGS_build} -eq ${FLAGS_TRUE} which does
nothing but avoid showing the message.

[ericli, 2010/11/24 19:20:57]

Done.

+  fi
+
+  if [ ${FLAGS_build} -eq ${FLAGS_TRUE} ]; then
+    AUTOTEST_DIR="${SRC_ROOT}/third_party/autotest/files"
   else
+    AUTOTEST_DIR="/build/${FLAGS_board}/usr/local/autotest"
+  fi
+
+  if [ -d "${AUTOTEST_DIR}" ]; then

[kmixter1, 2010/11/23 23:30:41]

I would swap the order, if [ ! -d "${...}" ]; then
  print message
  die.

and then do the normal case outside the if afterwards.  Agree with Richard that
the emerged autotest command should only be shown if FLAGS_use_emerged was set.

+    if [ ${FLAGS_build} -eq ${FLAGS_TRUE} ]; then
+      info \
+"Build and run autotests from ${AUTOTEST_DIR}."
+      return
+    fi
     info \
-"Using emerged autotests already installed in your sysroot.  To build \
-autotests directly from your source directory instead, pass --build."
-    FLAGS_build=${FLAGS_FALSE}
+"As requested, using emerged autotests already installed at ${AUTOTEST_DIR}."

[jrbarnette, 2010/11/23 23:16:04]

The text of message originally depended on whether the user specified
--use_emerged.  I think that that earlier distinction of the messages in the
two cases was useful.

[ericli, 2010/11/24 19:20:57]

Done.

+  else
+    [ ${FLAGS_build} -eq ${FLAGS_TRUE} ] && die \
+"build flag was turned on but ${AUTOTEST_DIR} is not found. Run cros_workon \
+start autotest first."

[jrbarnette, 2010/11/23 23:16:04]

The code guidelines and I would both prefer that this test be written
  elif [ ${FLAGS_BUILD} -eq ... ]; then
    die ...
  else
    die ...
  ...

+    die \
+"Could not find pre-installed autotest, you need to emerge-${FLAGS_board} \
+autotest autotest-tests (or use --build)."
   fi
 }
 
 function main() {
   cd $(dirname "$0")
 
   FLAGS "$@" || exit 1
 
   if [[ -z "${FLAGS_ARGV}" ]]; then
     echo "Usage: $0 --remote=[hostname] [regexp...]:"
@@ skipping 15 matching lines @@
 
   set -e
 
   create_tmp
 
   trap cleanup EXIT
 
   remote_access_init
 
   learn_board
-  autotest_dir="${FLAGS_chroot}/build/${FLAGS_board}/usr/local/autotest"
-
-  ENTER_CHROOT=""
-  if [[ ${INSIDE_CHROOT} -eq 0 ]]; then
-    ENTER_CHROOT="./enter_chroot.sh --chroot ${FLAGS_chroot} --"
-  fi
-
-  if [ ${FLAGS_build} -eq ${FLAGS_FALSE} ]; then
-    autodetect_build
-  fi
-
-  if [ ${FLAGS_build} -eq ${FLAGS_TRUE} ]; then
-    autotest_dir="${SRC_ROOT}/third_party/autotest/files"
-  else
-    if [ ! -d "${autotest_dir}" ]; then
-      die "You need to emerge autotest-tests (or use --build)"
-    fi
-  fi
+  autodetect_build
 
   local control_files_to_run=""
   local chrome_autotests="${CHROME_ROOT}/src/chrome/test/chromeos/autotest/files"
   # Now search for tests which unambiguously include the given identifier
   local search_path=$(echo {client,server}/{tests,site_tests})
   # Include chrome autotest in the search path
   if [ -n "${CHROME_ROOT}" ]; then
     search_path="${search_path} ${chrome_autotests}/client/site_tests"
   fi
-  pushd ${autotest_dir} > /dev/null
+
+  pushd ${AUTOTEST_DIR} > /dev/null
   for test_request in $FLAGS_ARGV; do
     test_request=$(remove_quotes "${test_request}")
     ! finds=$(find ${search_path} -maxdepth 2 -type f \( -name control.\* -or \
       -name control \) | egrep -v "~$" | egrep "${test_request}")
     if [[ -z "${finds}" ]]; then
       die "Cannot find match for \"${test_request}\""
     fi
     local matches=$(echo "${finds}" | wc -l)
     if [[ ${matches} -gt 1 ]]; then
       echo ">>> \"${test_request}\" is an ambiguous pattern.  Disambiguate by" \
            "passing one of these patterns instead:"
       for FIND in ${finds}; do
         echo "   ^${FIND}\$"
       done
       exit 1
     fi
     for i in $(seq 1 $FLAGS_iterations); do
       control_files_to_run="${control_files_to_run} '${finds}'"
     done
   done
-  popd > /dev/null
 
   echo ""
 
   if [[ -z "${control_files_to_run}" ]]; then
     die "Found no control files"
   fi
 
-  [ ${FLAGS_build} -eq ${FLAGS_TRUE} ] && prepare_build_dir "${autotest_dir}"
+  [ ${FLAGS_build} -eq ${FLAGS_TRUE} ] && setup_build_env
 
   info "Running the following control files:"
-  for CONTROL_FILE in ${control_files_to_run}; do
-    info " * ${CONTROL_FILE}"
+  for control_file in ${control_files_to_run}; do
+    info " * ${control_file}"
   done
 
   for control_file in ${control_files_to_run}; do
     # Assume a line starts with TEST_TYPE =
     control_file=$(remove_quotes "${control_file}")
-    local type=$(read_test_type "${autotest_dir}/${control_file}")
+    local test_type=$(read_test_type "${AUTOTEST_DIR}/${control_file}")
     # Check if the control file is an absolute path (i.e. chrome autotests case)
     if [[ ${control_file:0:1} == "/" ]]; then
-      type=$(read_test_type "${control_file}")
+      test_type=$(read_test_type "${control_file}")
     fi
     local option
-    if [[ "${type}" == "client" ]]; then
+    if [[ "${test_type}" == "client" ]]; then
       option="-c"
     else
       option="-s"
     fi
     echo ""
-    info "Running ${type} test ${control_file}"
+    info "Running ${test_type} test ${control_file}"
     local control_file_name=$(basename "${control_file}")
     local short_name=$(basename $(dirname "${control_file}"))
 
     # testName/control --> testName
     # testName/control.bvt --> testName.bvt
     # testName/control.regression --> testName.regression
     # testName/some_control --> testName.some_control
     if [[ "${control_file_name}" != control ]]; then
       if [[ "${control_file_name}" == control.* ]]; then
         short_name=${short_name}.${control_file_name/control./}
       else
         short_name=${short_name}.${control_file_name}
       fi
     fi
 
     local results_dir_name="${short_name}"
-    local results_dir="${TMP_INSIDE_CHROOT}/${results_dir_name}"
+    local results_dir="${TMP}/${results_dir_name}"
     rm -rf "${results_dir}"
     local verbose=""
     if [[ ${FLAGS_verbose} -eq $FLAGS_TRUE ]]; then
       verbose="--verbose"
     fi
 
     RAN_ANY_TESTS=${FLAGS_TRUE}
 
     # Remove chrome autotest location prefix from control_file if needed
     if [[ ${control_file:0:${#chrome_autotests}} == \
           "${chrome_autotests}" ]]; then
       control_file="${control_file:${#chrome_autotests}+1}"
       info "Running chrome autotest ${control_file}"
     fi
 
-    local autoserv_test_args="${FLAGS_args}"
-    if [ -n "${autoserv_test_args}" ]; then
-      autoserv_test_args="-a \"${autoserv_test_args}\""
-    fi
     local autoserv_args="-m ${FLAGS_remote} --ssh-port ${FLAGS_ssh_port} \
         ${option} ${control_file} -r ${results_dir} ${verbose}"
-    if [ ${FLAGS_build} -eq ${FLAGS_FALSE} ]; then
-      cat > "${TMP}/run_test.sh" <<EOF
-cd /build/${FLAGS_board}/usr/local/autotest
-sudo chmod a+w ./server/{tests,site_tests}
-echo ./server/autoserv ${autoserv_args} ${autoserv_test_args}
-./server/autoserv ${autoserv_args} ${autoserv_test_args}
-EOF
-      chmod a+rx "${TMP}/run_test.sh"
-      ${ENTER_CHROOT} ${TMP_INSIDE_CHROOT}/run_test.sh >&2
-    else
-      cp "${BUILD_DIR}/environment" "${TMP}/run_test.sh"
-      GRAPHICS_BACKEND=${GRAPHICS_BACKEND:-OPENGL}
-      cat >> "${TMP}/run_test.sh" <<EOF
-export GCLIENT_ROOT=/home/${USER}/trunk
-export GRAPHICS_BACKEND=${GRAPHICS_BACKEND}
-export SSH_AUTH_SOCK=${SSH_AUTH_SOCK} TMPDIR=/tmp SSH_AGENT_PID=${SSH_AGENT_PID}
-export SYSROOT=/build/${FLAGS_board}
-tc-export CC CXX PKG_CONFIG
-cd ${INSIDE_BUILD_DIR}
-echo ./server/autoserv ${autoserv_args} ${autoserv_test_args}
-./server/autoserv ${autoserv_args} ${autoserv_test_args}
-EOF
-      sudo cp "${TMP}/run_test.sh" "${BUILD_DIR}"
-      sudo chmod a+rx "${BUILD_DIR}/run_test.sh"
-      ${ENTER_CHROOT} sudo bash -c "${INSIDE_BUILD_DIR}/run_test.sh" >&2
+    if [ -n "${FLAGS_args}" ]; then
+      autoserv_args="${autoserv_args} -a \""${FLAGS_args}"\""
     fi
+
+    sudo chmod a+w ./server/{tests,site_tests}
+    echo ./server/autoserv ${autoserv_args}
+    ./server/autoserv ${autoserv_args}
   done
+  popd > /dev/null
 
   echo ""
   info "Test results:"
   ./generate_test_report "${TMP}" --strip="${TMP}/"
 
   print_time_elapsed
 }
 
+restart_in_chroot_if_needed $*
 main "$@"