From d6491ea8e0cacec2e73e38f5b1afa92ea1417958 Mon Sep 17 00:00:00 2001 From: Jason Boutte Date: Tue, 7 May 2024 09:57:38 -0700 Subject: [PATCH] Consolidates error messaging --- CIME/locked_files.py | 59 +++++++++++------------ CIME/tests/test_unit_locked_files.py | 71 +++++++++++++++++++++------- 2 files changed, 85 insertions(+), 45 deletions(-) diff --git a/CIME/locked_files.py b/CIME/locked_files.py index c83b469540c..178a8af6942 100644 --- a/CIME/locked_files.py +++ b/CIME/locked_files.py @@ -154,14 +154,18 @@ def check_diff(case, filename, env_name, diff): "\t{!r} has changed from {!r} to {!r}".format(key, value[1], value[0]) ) + reset = False + rebuild = False + message = "" + clean_targets = "" + rebuild_components = [] + if env_name == "env_case": expect( False, f"Cannot change `env_case.xml`, please restore origin {filename!r}", ) elif env_name == "env_build" and diff: - case.set_value("BUILD_COMPLETE", False) - build_status = 1 if "PIO_VERSION" in diff: @@ -173,37 +177,34 @@ def check_diff(case, filename, env_name, diff): case.set_value("BUILD_STATUS", build_status) - expect( - False, - "For your changes to take effect, run:\n./case.build --clean-all\n./case.build", - ) - elif env_name == "env_batch": - expect( - False, - "Batch configuration has changed, please run `./case.setup --reset`", - ) - elif env_name == "env_mach_pes": - expect(False, "For your changes to take effect, run:\n./case.setup --reset") - else: - toggle_build_status = False + rebuild = True - rebuild_components = [] + clean_targets = "--clean-all" + elif env_name in ("env_batch", "env_mach_pes"): + reset = True - for component in case.get_values("COMP_CLASSES"): - triggers = case.get_values(f"REBUILD_TRIGGER_{component}") + for component in case.get_values("COMP_CLASSES"): + triggers = case.get_values(f"REBUILD_TRIGGER_{component}") - if any([y.startswith(x) for x in triggers for y in diff.keys()]): - toggle_build_status = True + if any([y.startswith(x) for x in triggers for y in diff.keys()]): + rebuild = True - rebuild_components.append(component) + rebuild_components.append(component) - if toggle_build_status: - case.set_value("BUILD_COMPLETE", False) + if reset: + message = "For your changes to take effect, run:\n./case.setup --reset\n" - clean_targets = " ".join([x.lower() for x in rebuild_components]) - clean_cmd = f"./case.build --clean {clean_targets}" + if rebuild: + case.set_value("BUILD_COMPLETE", False) - expect( - False, - f"A rebuild is required, please run: \n./case.setup --reset\n{clean_cmd}\n./case.build", - ) + if rebuild_components and clean_targets != "--clean-all": + clean_targets = " ".join([x.lower() for x in rebuild_components]) + + clean_targets = f"--clean {clean_targets}" + + if not reset: + message = "For your changes to take effect, run:\n" + + message = f"{message}./case.build {clean_targets}\n./case.build" + + expect(False, message) diff --git a/CIME/tests/test_unit_locked_files.py b/CIME/tests/test_unit_locked_files.py index 3ada1964bd3..05b2e9952dc 100644 --- a/CIME/tests/test_unit_locked_files.py +++ b/CIME/tests/test_unit_locked_files.py @@ -52,28 +52,62 @@ def create_fake_env(tempdir): class TestLockedFiles(unittest.TestCase): - def test_check_diff_rebuild_trigger(self): + def test_check_diff_reset_and_rebuild(self): case = mock.MagicMock() - case.get_values.side_effect = (("CPL", "ATM"), (), ("NTASKS",)) + # reset triggered by env_mach_pes + # rebuild triggered by REBUILD_TRIGGER_ATM and REBUILD_TRIGGER_LND + # COMP_CLASSES, REBUILD_TRIGGER_CPL, REBUILD_TRIGGER_ATM, REBUILD_TRIGGER_LND + case.get_values.side_effect = ( + ("CPL", "ATM", "LND"), + (), + ("NTASKS",), + ("NTASKS",), + ) diff = { "NTASKS": ("32", "16"), } - with self.assertRaisesRegex( - CIMEError, "ERROR: For your changes to take effect, run:.*" - ): + expected_msg = """ERROR: For your changes to take effect, run: +./case.setup --reset +./case.build --clean atm lnd +./case.build""" + + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_mach_pes.xml", "env_mach_pes", diff) - def test_check_diff_other_env(self): + def test_check_diff_reset_and_rebuild_single(self): case = mock.MagicMock() + # reset triggered by env_mach_pes + # rebuild triggered only by REBUILD_TRIGGER_ATM + # COMP_CLASSES, REBUILD_TRIGGER_CPL, REBUILD_TRIGGER_ATM, REBUILD_TRIGGER_LND + case.get_values.side_effect = (("CPL", "ATM", "LND"), (), ("NTASKS",), ()) + diff = { - "some_key": ("value1", "value2"), + "NTASKS": ("32", "16"), + } + + expected_msg = """ERROR: For your changes to take effect, run: +./case.setup --reset +./case.build --clean atm +./case.build""" + + with self.assertRaisesRegex(CIMEError, expected_msg): + locked_files.check_diff(case, "env_mach_pes.xml", "env_mach_pes", diff) + + def test_check_diff_env_mach_pes(self): + case = mock.MagicMock() + + diff = { + "NTASKS": ("32", "16"), } - with self.assertRaises(CIMEError): + expected_msg = """ERROR: For your changes to take effect, run: +./case.setup --reset""" + + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_mach_pes.xml", "env_mach_pes", diff) def test_check_diff_env_build_no_diff(self): @@ -95,10 +129,9 @@ def test_check_diff_env_build_pio_version(self): expected_msg = """ERROR: For your changes to take effect, run: ./case.build --clean-all -./case.build - """ +./case.build""" - with self.assertRaises(CIMEError, msg=expected_msg): + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_build.xml", "env_build", diff) case.set_value.assert_any_call("BUILD_COMPLETE", False) @@ -113,10 +146,9 @@ def test_check_diff_env_build(self): expected_msg = """ERROR: For your changes to take effect, run: ./case.build --clean-all -./case.build - """ +./case.build""" - with self.assertRaises(CIMEError, msg=expected_msg): + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_build.xml", "env_build", diff) case.set_value.assert_any_call("BUILD_COMPLETE", False) @@ -129,7 +161,10 @@ def test_check_diff_env_batch(self): "some_key": ("value1", "value2"), } - with self.assertRaises(CIMEError): + expected_msg = """ERROR: For your changes to take effect, run: +./case.setup --reset""" + + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_batch.xml", "env_batch", diff) def test_check_diff_env_case(self): @@ -139,7 +174,11 @@ def test_check_diff_env_case(self): "some_key": ("value1", "value2"), } - with self.assertRaises(CIMEError): + expected_msg = ( + "ERROR: Cannot change `env_case.xml`, please restore origin 'env_case.xml'" + ) + + with self.assertRaisesRegex(CIMEError, expected_msg): locked_files.check_diff(case, "env_case.xml", "env_case", diff) def test_diff_lockedfile_detect_difference(self):