gce_au_worker: Avoid using colon in file names
The `test_that` script uses colon as a seperator in autotest suite names, e.g.,
`test_that -b lakitu 1.1.1.1 suite:gce-smoke`. We used to use the full suite
name as file name when saving test results, and it confused a piece of chromite
code (chromite.commands.ListFailedTests, which uses a regular expression to
match file names and colon is not deemed as a valid character) that parses test
logs for failed tests.
A sample failure can be found here (notice the 'IndexError' at the bottom of
the log):
https://uberchromegw.corp.google.com/i/chromeos/builders/lakitu%20canary/builds/712/steps/GCETest%20%28attempt%202%29/logs/stdio
While we can change the chromite code to include colon, I think it's better to
not use colon in file names at all, as it is commonly used as seperator in
things like $PATH and URL.
This CL also tweaks VerifyImage to print out the summarized test report in case
of a test failure, and includes some style smashes to better conform with the
style guide.
BUG=b:25849966
TEST=Unit test. Also ran a trybot against lakitu-pre-cq, with an intentional
test failure and verified that failed test was correctly linked in the
buildbot log, and the 'IndexError' was gone.
Change-Id: Ie4b3b002c95b81f30c4c63799b77819651050757
Reviewed-on: https://chromium-review.googlesource.com/317781
Commit-Ready: Daniel Wang <wonderfly@google.com>
Tested-by: Daniel Wang <wonderfly@google.com>
Reviewed-by: Amey Deshpande <ameyd@google.com>
diff --git a/au_test_harness/gce_au_worker.py b/au_test_harness/gce_au_worker.py
index 1789ca1..266f74d 100644
--- a/au_test_harness/gce_au_worker.py
+++ b/au_test_harness/gce_au_worker.py
@@ -102,10 +102,11 @@
Background processes that delete stale instances and images.
"""
- GS_PATH_COMMON_PREFIX = 'gs://'
- GS_URL_COMMON_PREFIX = 'https://storage.googleapis.com/'
- IMAGE_PREFIX = 'test-image-'
- INSTANCE_PREFIX = 'test-instance-'
+ _GS_PATH_COMMON_PREFIX = 'gs://'
+ _GS_URL_COMMON_PREFIX = 'https://storage.googleapis.com/'
+ _IMAGE_PREFIX = 'test-image-'
+ _INSTANCE_PREFIX = 'test-instance-'
+ _TEST_REPORT_FILENAME = 'test_report.log'
def __init__(self, options, test_results_root,
project=constants.GCE_PROJECT,
@@ -194,18 +195,18 @@
return_values = parallel.RunParallelSteps(steps, return_values=True)
passed = True
- outputs = {}
- for test, percent_passed, output in return_values:
+ test_reports = {}
+ for test, percent_passed, report in return_values:
passed &= (percent_passed == 100)
- outputs[test] = output
+ test_reports[test] = report
if not passed:
self._HandleFail(log_directory_base, fail_directory_base)
+ print ('\nSome test(s) failed. Test reports:')
+ for test, report in test_reports.iteritems():
+ print ('\nTest: %s\n%s' % (test, report or ''))
if unittest is not None:
- unittest.fail('Not all tests passed')
- for test, output in outputs.iteritems():
- print ('\nTest: %s\n' % test)
- print (output)
+ unittest.fail('Not all tests passed.')
return passed
# --- PRIVATE HELPER FUNCTIONS ---
@@ -217,20 +218,18 @@
'test_that'.
Args:
- test: (str) The test or suite to run.
- remote: (str) The hostname of the remote DUT.
- log_directory_base:
- (str) The base directory to store test logs. A sub directory specific
- to this test will be created there.
- fail_directory_base:
- (str) The base directory to store test logs in case of a test failure.
+ test: The test or suite to run.
+ remote: The hostname of the remote DUT.
+ log_directory_base: The base directory to store test logs. A sub directory
+ specific to this test will be created there.
+ fail_directory_base: The base directory to store test logs in case of a
+ test failure.
Returns:
- test:
- (str) Same as |test|. This is useful when the caller wants to
- correlate results to the test name.
- percent_passed: (int) Pass rate.
- output: (str): Original test output.
+ test: Same as |test|. This is useful when the caller wants to correlate
+ results to the test name.
+ percent_passed: Pass rate.
+ test_report: Content of the test report generated by test_that.
"""
log_directory, _ = self._GetResultsDirectoryForTest(
test, log_directory_base, fail_directory_base)
@@ -258,21 +257,44 @@
redirect_stdout=True,
cwd=constants.CROSUTILS_DIR)
percent_passed = self.ParseGeneratedTestOutput(result.output)
- return test, percent_passed, result.output
+ test_report = self._GetTestReport(log_directory)
+
+ # Returns the summarized test_report as it is more useful than the full
+ # output, plus the entire log will always be linked in the failure report.
+ return test, percent_passed, test_report
+
+ def _GetTestReport(self, results_path):
+ """Returns the content of test_report.log created by test_that.
+
+ Args:
+ results_path: Path to the directory where results are saved.
+
+ Returns:
+ Content of test_report.log, or None if report is not found.
+ """
+ report_path = os.path.join(results_path, self._TEST_REPORT_FILENAME)
+ if os.path.isfile(report_path):
+ with open(report_path) as f:
+ return f.read()
+ logging.warning('Test log not found in %s', results_path)
+ return None
def _GetResultsDirectoryForTest(self, test, log_directory_base,
fail_directory_base):
"""Gets the log and fail directories for a particular test.
Args:
- test: (str) The test or suite to get directories for.
- log_directory_base:
- (str) The base directory where all test results are saved.
- fail_directory_base:
- (str) The base directory where all test failures are recorded.
+ test: The test or suite to get directories for.
+ log_directory_base: The base directory where all test results are saved.
+ fail_directory_base: The base directory where all test failures are
+ recorded.
"""
- log_directory = os.path.join(log_directory_base, test)
- fail_directory = os.path.join(fail_directory_base, test)
+ # Avoid using colon in file names. Not that it's not allowed, but it causes
+ # confusions and inconvenience as it is used as a separator in many cases,
+ # e.g., $PATH and url.
+ sanitized_test_name = test.replace(':', '_')
+ log_directory = os.path.join(log_directory_base, sanitized_test_name)
+ fail_directory = os.path.join(fail_directory_base, sanitized_test_name)
if not os.path.exists(log_directory):
os.makedirs(log_directory)
@@ -372,7 +394,7 @@
'Error: %s' % e)
# Create an image from |image_path| and an instance from the image.
- image = '%s%s' % (self.IMAGE_PREFIX, ts)
+ image = '%s%s' % (self._IMAGE_PREFIX, ts)
try:
self.image_link = self.gce_context.CreateImage(
image, self._GsPathToUrl(self.tarball_remote))
@@ -393,7 +415,7 @@
for test in self.tests:
ts = datetime.datetime.fromtimestamp(time.time()).strftime(
'%Y-%m-%d-%H-%M-%S')
- instance = '%s%s' % (self.INSTANCE_PREFIX, ts)
+ instance = '%s%s' % (self._INSTANCE_PREFIX, ts)
kwargs = test['flags'].copy()
kwargs['description'] = 'For test %s' % test['name']
steps.append(partial(self._CreateInstance, instance,
@@ -409,13 +431,11 @@
and deletes it on true.
Args:
- resource: (str) The resource name/url to delete.
- existence_checker:
- (callable) The callable to check existence. This callable should take
- |resource| as its first argument.
- deletor:
- (callable) The callable to perform the deletion. This callable should
- take |resource| as its first argument.
+ resource: The resource name/url to delete.
+ existence_checker: A callable to check existence. This callable should
+ take |resource| as its first argument.
+ deletor: A callable to perform the deletion. This callable should take
+ |resource| as its first argument.
Raises:
ValueError if existence_checker or deletor is not callable.
@@ -508,10 +528,10 @@
Raises:
ValueError if |gs_path| is not a valid GS path.
"""
- if not gs_path.startswith(self.GS_PATH_COMMON_PREFIX):
+ if not gs_path.startswith(self._GS_PATH_COMMON_PREFIX):
raise ValueError('Invalid GCS path: %s' % gs_path)
- return gs_path.replace(self.GS_PATH_COMMON_PREFIX,
- self.GS_URL_COMMON_PREFIX, 1)
+ return gs_path.replace(self._GS_PATH_COMMON_PREFIX,
+ self._GS_URL_COMMON_PREFIX, 1)
def _WaitForBackgroundDeleteProcesses(self):
"""Waits for all background proecesses to finish."""
diff --git a/au_test_harness/gce_au_worker_unittest.py b/au_test_harness/gce_au_worker_unittest.py
index 3820a71..e88de00 100755
--- a/au_test_harness/gce_au_worker_unittest.py
+++ b/au_test_harness/gce_au_worker_unittest.py
@@ -200,7 +200,7 @@
]
actual_tests_run = []
- def _OverrideGetInstanceIP(instance, *unused_args, **unused_kwargs):
+ def _OverrideGetInstanceIP(instance, *_args, **_kwargs):
if instance == 'instance_1':
return '1.1.1.1'
elif instance == 'instance_2':
@@ -208,7 +208,7 @@
else:
return '3.3.3.3'
- def _OverrideRunCommand(cmd, *unused_args, **unused_kwargs):
+ def _OverrideRunCommand(cmd, *_args, **_kwargs):
"""A mock of cros_build_lib.RunCommand that injects arguments."""
# In this test setup, |test| and |remote| should be the third and fourth
# last argument respectively.
@@ -217,7 +217,7 @@
actual_tests_run.append(dict(remote=remote, test=test))
return cros_build_lib.CommandResult()
- def _OverrideRunParallelSteps(steps, *unused_args, **unused_kwargs):
+ def _OverrideRunParallelSteps(steps, *_args, **_kwargs):
"""Run steps sequentially."""
return_values = []
for step in steps:
