From f5d991e963e70ad2e90cad6ae999da5666d83cf1 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Mon, 13 Nov 2017 21:09:33 -0700 Subject: [PATCH] Add unit tests of compare_two's handling of pass/fail in comparison Previously, I had figured that it was sufficient to ensure that _component_compare_test was actually being called, figuring that the tests of _component_compare_test belong elsewhere. But, since this is such a critical aspect of the system_test_compare_two infrastructure, I'm adding some tests covering _component_compare_test here. Note that I have now removed the checks of the "Call" objects to _component_compare_test. Now that we're calling and testing the actual _component_compare_test, it is no longer possible or necessary to construct and check a "Call" object recording this call. --- .../CIME/SystemTests/system_tests_common.py | 9 +- .../test_system_tests_compare_two.py | 99 +++++++++++++------ 2 files changed, 78 insertions(+), 30 deletions(-) diff --git a/scripts/lib/CIME/SystemTests/system_tests_common.py b/scripts/lib/CIME/SystemTests/system_tests_common.py index f53698bafea..bef34e458ff 100644 --- a/scripts/lib/CIME/SystemTests/system_tests_common.py +++ b/scripts/lib/CIME/SystemTests/system_tests_common.py @@ -259,7 +259,7 @@ def _component_compare_test(self, suffix1, suffix2, success_change=False): run case needs indirection based on success. If success_change is True, success requires some files to be different """ - success, comments = compare_test(self._case, suffix1, suffix2) + success, comments = self._do_compare_test(suffix1, suffix2) if success_change: success = not success @@ -269,6 +269,13 @@ def _component_compare_test(self, suffix1, suffix2, success_change=False): self._test_status.set_status("{}_{}_{}".format(COMPARE_PHASE, suffix1, suffix2), status) return success + def _do_compare_test(self, suffix1, suffix2): + """ + Wraps the call to compare_test to facilitate replacement in unit + tests + """ + return compare_test(self._case, suffix1, suffix2) + def _get_mem_usage(self, cpllog): """ Examine memory usage as recorded in the cpl log file and look for unexpected diff --git a/scripts/lib/CIME/tests/SystemTests/test_system_tests_compare_two.py b/scripts/lib/CIME/tests/SystemTests/test_system_tests_compare_two.py index 342fe4ecd54..f356377ac91 100644 --- a/scripts/lib/CIME/tests/SystemTests/test_system_tests_compare_two.py +++ b/scripts/lib/CIME/tests/SystemTests/test_system_tests_compare_two.py @@ -53,7 +53,6 @@ METHOD_case_one_custom_postrun_action = "_case_one_custom_postrun_action" METHOD_case_two_custom_prerun_action = "_case_two_custom_prerun_action" METHOD_case_two_custom_postrun_action = "_case_two_custom_postrun_action" -METHOD_component_compare_test = "_component_compare_test" METHOD_link_to_case2_output = "_link_to_case2_output" METHOD_run_indv = "_run_indv" @@ -77,7 +76,8 @@ def __init__(self, multisubmit = False, case2setup_raises_exception = False, run_one_should_pass = True, - run_two_should_pass = True): + run_two_should_pass = True, + compare_should_pass = True): """ Initialize a SystemTestsCompareTwoFake object @@ -97,6 +97,8 @@ def __init__(self, pass for the first run. Default is True, meaning it will pass. run_two_should_pass (bool, optional): Whether the run_indv method should pass for the second run. Default is True, meaning it will pass. + compare_should_pass (bool, optional): Whether the comparison between the two + cases should pass. Default is True, meaning it will pass. """ self._case2setup_raises_exception = case2setup_raises_exception @@ -131,6 +133,8 @@ def __init__(self, if run_two_should_pass: self.run_pass_caseroot.append(self._case2.get_value('CASEROOT')) + self.compare_should_pass = compare_should_pass + self.log = [] # ------------------------------------------------------------------------ @@ -175,26 +179,12 @@ def run_indv(self, suffix="base", st_archive=False): if caseroot not in self.run_pass_caseroot: raise RuntimeError('caseroot not in run_pass_caseroot') - def _component_compare_test(self, suffix1, suffix2, success_change=False): - # Trying to use the real version of _component_compare_test would pull - # too much baggage into these tests. Since the return value from this - # method isn't important, it's sufficient for the tests of this class to - # just ensure that _component_compare_test was actually called - # correctly. - # - # An alternative would be to extract the main work of - # _component_compare_test into a different method that returns a True - # (success) / False (failure) result, with _component_compare_test then - # updating test_status appropriately. Then we could override that new - # method in this Fake class, using the true implementation of - # _component_compare_test. Then the test verification would include - # verification that TestStatus is set correctly for the COMPARE - # phase. But that seems more about testing _component_compare_test than - # testing SystemTestsCompareTwo itself, so I don't see much added value - # of that. - - self.log.append(Call(METHOD_component_compare_test, - {'suffix1': suffix1, 'suffix2': suffix2})) + def _do_compare_test(self, suffix1, suffix2): + """ + This fake implementation allows controlling whether compare_test + passes or fails + """ + return (self.compare_should_pass, "no comment") def _check_for_memleak(self): pass @@ -272,6 +262,17 @@ def get_caseroots(self, casename='mytest'): case2root = os.path.join(case1root, 'case2', casename) return case1root, case2root + def get_compare_phase_name(self, mytest): + """ + Returns a string giving the compare phase name for this test + """ + run_one_suffix = mytest._run_one_suffix + run_two_suffix = mytest._run_two_suffix + compare_phase_name = "{}_{}_{}".format(test_status.COMPARE_PHASE, + run_one_suffix, + run_two_suffix) + return compare_phase_name + def test_setup(self): # Ensure that test setup properly sets up case 1 and case 2 @@ -407,9 +408,7 @@ def test_run_phase_internal_calls(self): Call(METHOD_run_indv, {'suffix': run_two_suffix, 'CASEROOT': case2root}), Call(METHOD_case_two_custom_postrun_action, {}), - Call(METHOD_link_to_case2_output, {}), - Call(METHOD_component_compare_test, - {'suffix1': run_one_suffix, 'suffix2': run_two_suffix}) + Call(METHOD_link_to_case2_output, {}) ] self.assertEqual(expected_calls, mytest.log) @@ -443,6 +442,10 @@ def test_run_phase_internal_calls_multisubmit_phase1(self): ] self.assertEqual(expected_calls, mytest.log) + # Also verify that comparison is NOT called: + compare_phase_name = self.get_compare_phase_name(mytest) + self.assertIsNone(mytest._test_status.get_status(compare_phase_name)) + def test_run_phase_internal_calls_multisubmit_phase2(self): # Make sure that the correct calls are made to methods stubbed out by # SystemTestsCompareTwoFake (when runs succeed), when we have a @@ -457,7 +460,8 @@ def test_run_phase_internal_calls_multisubmit_phase2(self): case1 = case1, run_one_suffix = run_one_suffix, run_two_suffix = run_two_suffix, - multisubmit = True) + multisubmit = True, + compare_should_pass = True) # RESUBMIT=0 signals second phase case1.set_value("RESUBMIT", 0) @@ -470,12 +474,15 @@ def test_run_phase_internal_calls_multisubmit_phase2(self): Call(METHOD_run_indv, {'suffix': run_two_suffix, 'CASEROOT': case2root}), Call(METHOD_case_two_custom_postrun_action, {}), - Call(METHOD_link_to_case2_output, {}), - Call(METHOD_component_compare_test, - {'suffix1': run_one_suffix, 'suffix2': run_two_suffix}) + Call(METHOD_link_to_case2_output, {}) ] self.assertEqual(expected_calls, mytest.log) + # Also verify that comparison is called: + compare_phase_name = self.get_compare_phase_name(mytest) + self.assertEqual(test_status.TEST_PASS_STATUS, + mytest._test_status.get_status(compare_phase_name)) + def test_run1_fails(self): # Make sure that a failure in run1 is reported correctly @@ -508,5 +515,39 @@ def test_run2_fails(self): self.assertEqual(test_status.TEST_FAIL_STATUS, mytest._test_status.get_status(test_status.RUN_PHASE)) + def test_compare_passes(self): + # Make sure that a pass in the comparison is reported correctly + + # Setup + case1root = os.path.join(self.tempdir, 'case1') + case1 = CaseFake(case1root) + mytest = SystemTestsCompareTwoFake(case1, + compare_should_pass = True) + + # Exercise + mytest.run() + + # Verify + compare_phase_name = self.get_compare_phase_name(mytest) + self.assertEqual(test_status.TEST_PASS_STATUS, + mytest._test_status.get_status(compare_phase_name)) + + def test_compare_fails(self): + # Make sure that a failure in the comparison is reported correctly + + # Setup + case1root = os.path.join(self.tempdir, 'case1') + case1 = CaseFake(case1root) + mytest = SystemTestsCompareTwoFake(case1, + compare_should_pass = False) + + # Exercise + mytest.run() + + # Verify + compare_phase_name = self.get_compare_phase_name(mytest) + self.assertEqual(test_status.TEST_FAIL_STATUS, + mytest._test_status.get_status(compare_phase_name)) + if __name__ == "__main__": unittest.main(verbosity=2, catchbreak=True)