From 4bf85d17e38c3f36f64c761a0743591d296f41a5 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Fri, 23 Sep 2016 12:49:19 -0700 Subject: [crosperf] Make generate_report report actual failures. We would never increment `num_success`, so we'd always return failure. This passed tests, since there were no tests that checked for success. Also, testRunActionsRunsAllActionsRegardlessOfExceptions wasn't really doing what it was supposed to; exceptions wouldn't get raised (since the mocked WriteFile turns into a nop), so all of the WriteFile actions were "passing". Because of the aforementioned bug, this test seemed to work, as well. BUG=None TEST=generate_report_unittest.py passes; cros lint is happy. Change-Id: If4d71af3415d97be8d5ac3bec796fe4589d50aaa Reviewed-on: https://chrome-internal-review.googlesource.com/289802 Commit-Ready: George Burgess Tested-by: George Burgess Reviewed-by: Caroline Tice --- crosperf/generate_report.py | 5 +++-- crosperf/generate_report_unittest.py | 28 +++++++++++++++++----------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/crosperf/generate_report.py b/crosperf/generate_report.py index 0fd9fa43..e0add994 100755 --- a/crosperf/generate_report.py +++ b/crosperf/generate_report.py @@ -198,7 +198,7 @@ def WriteFile(output_prefix, extension, get_contents, overwrite, verbose): def RunActions(actions, benchmark_results, output_prefix, overwrite, verbose): """Runs `actions`, returning True if all succeeded.""" - num_success = 0 + failed = False report_ctor = None # Make the linter happy for report_ctor, extension in actions: @@ -208,8 +208,9 @@ def RunActions(actions, benchmark_results, output_prefix, overwrite, verbose): except Exception: # Complain and move along; we may have more actions that might complete # successfully. + failed = True traceback.print_exc() - return num_success == len(actions) + return not failed def PickInputFile(input_name): diff --git a/crosperf/generate_report_unittest.py b/crosperf/generate_report_unittest.py index 7f556320..a5d00635 100755 --- a/crosperf/generate_report_unittest.py +++ b/crosperf/generate_report_unittest.py @@ -118,21 +118,27 @@ class GenerateReportTests(unittest.TestCase): ctors = [ctor for ctor, _ in mock_run_actions.call_args[0][0]] self.assertItemsEqual(ctors, [results_report.HTMLResultsReport]) - @mock.patch('generate_report.WriteFile') - def testRunActionsRunsAllActionsRegardlessOfExceptions(self, mock_write_file): - def raise_error(_): - raise Exception('Oh nooo') - actions = [ - (raise_error, 'json'), - (raise_error, 'html'), - (raise_error, 'text'), - (raise_error, 'email'), - ] + # We only mock print_exc so we don't have exception info printed to stdout. + @mock.patch('generate_report.WriteFile', side_effect=ValueError('Oh noo')) + @mock.patch('traceback.print_exc') + def testRunActionsRunsAllActionsRegardlessOfExceptions(self, mock_print_exc, + mock_write_file): + actions = [(None, 'json'), (None, 'html'), (None, 'text'), (None, 'email')] output_prefix = '-' ok = generate_report.RunActions(actions, {}, output_prefix, overwrite=False, verbose=False) self.assertFalse(ok) - self.assertEqual(mock_write_file.call_count, 4) + self.assertEqual(mock_write_file.call_count, len(actions)) + self.assertEqual(mock_print_exc.call_count, len(actions)) + + @mock.patch('generate_report.WriteFile') + def testRunActionsReturnsTrueIfAllActionsSucceed(self, mock_write_file): + actions = [(None, 'json'), (None, 'html'), (None, 'text')] + output_prefix = '-' + ok = generate_report.RunActions(actions, {}, output_prefix, overwrite=False, + verbose=False) + self.assertEqual(mock_write_file.call_count, len(actions)) + self.assertTrue(ok) if __name__ == '__main__': -- cgit v1.2.3