Consolidates error messaging

ESMCI · May 7, 2024 · d6491ea · d6491ea
1 parent 3b771a9
commit d6491ea
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 45 deletions.
diff --git 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
@@ -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):