From 2edd4a1bf58fdd2dcfc8ae77a84417d5e738a015 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:28:54 -0400
Subject: [PATCH 01/13] refactor codebase for check_test_state by making use of
 a dictionary mapping for calling each check function instead of having many
 if statements. Add support for --strict option in 'buildtest build' that will
 be used for toggling 'set -eo pipefail'.

This feature is supported with profiles and using rerun feature (--rerun).

Add example buildspec used for showing how --strict option will work
---
 buildtest/builders/base.py             | 85 ++++++++++++++++++++++----
 buildtest/builders/script.py           |  6 +-
 buildtest/builders/spack.py            |  5 +-
 buildtest/buildsystem/builders.py      | 10 ++-
 buildtest/cli/__init__.py              |  7 +++
 buildtest/cli/build.py                 |  8 +++
 buildtest/main.py                      |  1 +
 buildtest/schemas/settings.schema.json |  3 +-
 buildtest/utils/file.py                |  5 +-
 tutorials/strict_example.yml           |  0
 10 files changed, 108 insertions(+), 22 deletions(-)
 create mode 100644 tutorials/strict_example.yml

diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py
index fbfd4e855..7774bc668 100644
--- a/buildtest/builders/base.py
+++ b/buildtest/builders/base.py
@@ -296,19 +296,6 @@ def is_container_executor(self):
         # from buildtest.executors.container import ContainerExecutor
         return self.buildexecutor.executors[self.executor].type == "container"
 
-    def is_slurm_executor(self):
-        """Return True if current builder executor type is LocalExecutor otherwise returns False.
-
-        Returns:
-            bool: returns True if builder is using executor type LocalExecutor otherwise returns False
-
-        """
-
-        # import issue when putting this at top of file
-        from buildtest.executors.slurm import SlurmExecutor
-
-        return isinstance(self.buildexecutor.executors[self.executor], SlurmExecutor)
-
     def is_batch_job(self):
         """Return True/False if builder.job attribute is of type Job instance if not returns False.
         This method indicates if builder has a job submitted to queue
@@ -1074,6 +1061,78 @@ def check_test_state(self):
 
         self.metadata["result"]["state"] = "FAIL"
 
+        if self.metadata["result"]["returncode"] == 0:
+            self.metadata["result"]["state"] = "PASS"
+
+        # if status is defined in Buildspec, then check for returncode and regex
+        if self.status:
+            # if 'state' property is specified explicitly honor this value regardless of what is calculated
+            if self.status.get("state"):
+                self.metadata["result"]["state"] = self.status["state"]
+                return
+
+            # Define a dictionary mapping status keys to their corresponding check functions
+            status_checks = {
+                "returncode": returncode_check,
+                "regex": regex_check,
+                "runtime": runtime_check,
+                "file_regex": file_regex_check,
+                "assert_ge": lambda: comparison_check(
+                    builder=self, comparison_type="ge"
+                ),
+                "assert_le": lambda: comparison_check(
+                    builder=self, comparison_type="le"
+                ),
+                "assert_gt": lambda: comparison_check(
+                    builder=self, comparison_type="gt"
+                ),
+                "assert_lt": lambda: comparison_check(
+                    builder=self, comparison_type="lt"
+                ),
+                "assert_eq": lambda: comparison_check(
+                    builder=self, comparison_type="eq"
+                ),
+                "assert_ne": lambda: comparison_check(
+                    builder=self, comparison_type="ne"
+                ),
+                "assert_range": assert_range_check,
+                "contains": lambda: contains_check(
+                    builder=self, comparison_type="contains"
+                ),
+                "not_contains": lambda: contains_check(
+                    builder=self, comparison_type="not_contains"
+                ),
+                "is_symlink": is_symlink_check,
+                "exists": exists_check,
+                "is_dir": is_dir_check,
+                "is_file": is_file_check,
+                "file_count": file_count_check,
+                "linecount": linecount_check,
+                "file_linecount": file_linecount_check,
+            }
+
+            # Iterate over the status_checks dictionary and perform the checks
+            for key, check_func in status_checks.items():
+                if key in self.status:
+                    self.metadata["check"][key] = check_func(self)
+
+            # filter out any None values from status check
+            status_checks = [
+                value for value in self.metadata["check"].values() if value is not None
+            ]
+
+            state = (
+                all(status_checks)
+                if self.status.get("mode") in ["AND", "and"]
+                else any(status_checks)
+            )
+            self.metadata["result"]["state"] = "PASS" if state else "FAIL"
+
+    def check_test_state1(self):
+        """This method is responsible for detecting state of test (PASS/FAIL) based on returncode or regular expression."""
+
+        self.metadata["result"]["state"] = "FAIL"
+
         if self.metadata["result"]["returncode"] == 0:
             self.metadata["result"]["state"] = "PASS"
 
diff --git a/buildtest/builders/script.py b/buildtest/builders/script.py
index cd90ee1fb..033b7b254 100644
--- a/buildtest/builders/script.py
+++ b/buildtest/builders/script.py
@@ -26,6 +26,7 @@ def __init__(
         numprocs=None,
         numnodes=None,
         compiler=None,
+        strict=None,
     ):
         super().__init__(
             name=name,
@@ -41,7 +42,7 @@ def __init__(
         self.compiler_settings = {"vars": None, "env": None, "modules": None}
 
         self.configuration = configuration
-
+        self.strict = strict
         self.compiler_section = self.recipe.get("compilers")
 
         # if 'compilers' property defined resolve compiler logic
@@ -132,7 +133,8 @@ def generate_script(self):
             if data_warp_lines:
                 script_lines += data_warp_lines
 
-        script_lines.append(self._emit_set_command())
+        if self.strict:
+            script_lines.append(self._emit_set_command())
 
         if self.shell.name == "python":
             script_lines = ["#!/bin/bash"]
diff --git a/buildtest/builders/spack.py b/buildtest/builders/spack.py
index 8f51b6b89..357d2380a 100644
--- a/buildtest/builders/spack.py
+++ b/buildtest/builders/spack.py
@@ -27,6 +27,7 @@ def __init__(
         testdir=None,
         numprocs=None,
         numnodes=None,
+        strict=None,
     ):
         super().__init__(
             name=name,
@@ -38,6 +39,7 @@ def __init__(
             numprocs=numprocs,
             numnodes=numnodes,
         )
+        self.strict = strict
         self.status = deep_get(
             self.recipe, "executors", self.executor, "status"
         ) or self.recipe.get("status")
@@ -54,7 +56,8 @@ def generate_script(self):
         if sched_lines:
             lines += sched_lines
 
-        lines.append(self._emit_set_command())
+        if self.strict:
+            lines.append(self._emit_set_command())
 
         var_lines = self._get_variables(self.recipe.get("vars"))
         env_lines = self._get_environment(self.recipe.get("env"))
diff --git a/buildtest/buildsystem/builders.py b/buildtest/buildsystem/builders.py
index fcde81f8a..3b26c2440 100644
--- a/buildtest/buildsystem/builders.py
+++ b/buildtest/buildsystem/builders.py
@@ -34,6 +34,7 @@ def __init__(
         numnodes=None,
         executor_type=None,
         exclude_tags=None,
+        strict=None,
     ):
         """Based on a loaded Buildspec file, return the correct builder
         for each based on the type. Each type is associated with a known
@@ -50,6 +51,7 @@ def __init__(
             numnodes (list, optional): List of processor values to create builder objects specified via ``buildtest build --nodes``
             executor_type (str, optional): Filter test by executor type (local, batch)
             exclude_tags (list, optional): List of tags to exclude tests from buildspec file
+            strict (bool, optional): This is a boolean used for enable strict mode for test that will run the 'set' command in test.
         """
 
         self.configuration = configuration
@@ -62,7 +64,7 @@ def __init__(
         self.numnodes = numnodes
         self.executor_type = executor_type
         self.exclude_tags = exclude_tags
-
+        self.strict = strict
         self.bp = bp
         self.bc = buildtest_compilers
         self.filters = filters
@@ -169,6 +171,7 @@ def create_script_builders(
             configuration=self.configuration,
             testdir=self.testdir,
             compiler=compiler_name,
+            strict=self.strict,
         )
         builders.append(builder)
 
@@ -184,6 +187,7 @@ def create_script_builders(
                     testdir=self.testdir,
                     numnodes=node,
                     compiler=compiler_name,
+                    strict=self.strict,
                 )
                 builders.append(builder)
         if procs:
@@ -198,6 +202,7 @@ def create_script_builders(
                     testdir=self.testdir,
                     numprocs=proc,
                     compiler=compiler_name,
+                    strict=self.strict,
                 )
                 builders.append(builder)
 
@@ -222,6 +227,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
             buildspec=self.bp.buildspec,
             buildexecutor=self.buildexecutor,
             testdir=self.testdir,
+            strict=self.strict,
         )
         builders.append(builder)
 
@@ -235,6 +241,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
                     buildexecutor=self.buildexecutor,
                     testdir=self.testdir,
                     numnodes=node,
+                    strict=self.strict,
                 )
                 builders.append(builder)
         if procs:
@@ -247,6 +254,7 @@ def create_spack_builders(self, name, recipe, executor, nodes=None, procs=None):
                     buildexecutor=self.buildexecutor,
                     testdir=self.testdir,
                     numprocs=proc,
+                    strict=self.strict,
                 )
                 builders.append(builder)
 
diff --git a/buildtest/cli/__init__.py b/buildtest/cli/__init__.py
index bdde2d216..46b5a19a0 100644
--- a/buildtest/cli/__init__.py
+++ b/buildtest/cli/__init__.py
@@ -952,6 +952,13 @@ def build_menu(self):
                         "help": "Save buildtest command options into a profile and update configuration file"
                     },
                 ),
+                (
+                    ["--strict"],
+                    {
+                        "action": "store_true",
+                        "help": "Enable strict mode for test by setting 'set -eo pipefail' in test script",
+                    },
+                ),
                 (
                     ["--testdir"],
                     {
diff --git a/buildtest/cli/build.py b/buildtest/cli/build.py
index 75c934d83..0ca45e44f 100644
--- a/buildtest/cli/build.py
+++ b/buildtest/cli/build.py
@@ -626,6 +626,7 @@ def __init__(
         save_profile=None,
         profile=None,
         max_jobs=None,
+        strict=None,
         verbose=None,
         write_config_file=None,
     ):
@@ -665,6 +666,7 @@ def __init__(
             save_profile (str, optional): Save profile to buildtest configuration specified by ``buildtest build --save-profile``
             profile (str, optional): Profile to load from buildtest configuration specified by ``buildtest build --profile``
             max_jobs (int, optional): Maximum number of jobs to run concurrently. This option is specified by ``buildtest build --max-jobs``
+            strict (bool, optional): Enable strict mode for buildtest. This option is specified by ``buildtest build --strict``
             verbose (bool, optional): Enable verbose output for buildtest that is specified by ``buildtest --verbose``
             write_config_file (str, optional): Write configuration file to specified location. This is specified by ``buildtest build --write-config-file``
         """
@@ -742,6 +744,7 @@ def __init__(
         self.save_profile = save_profile
         self.profile = profile
         self.max_jobs = max_jobs
+        self.strict = strict
         self.write_config_file = write_config_file
 
         # this variable contains the detected buildspecs that will be processed by buildtest.
@@ -893,6 +896,7 @@ def load_rerun_file(self):
         self.timeout = content["timeout"]
         self.limit = content["limit"]
         self.max_jobs = content["max_jobs"]
+        self.strict = content["strict"]
 
     def save_rerun_file(self):
         """Record buildtest command options and save them into rerun file which is read when invoking ``buildtest build --rerun``."""
@@ -924,6 +928,7 @@ def save_rerun_file(self):
             "timeout": self.timeout,
             "limit": self.limit,
             "max_jobs": self.max_jobs,
+            "strict": self.strict,
         }
 
         with open(BUILDTEST_RERUN_FILE, "w") as fd:
@@ -980,6 +985,7 @@ def save_profile_to_configuration(self):
             "executor-type": self.executor_type,
             "max_jobs": self.max_jobs,
             "remove-stagedir": self.remove_stagedir,
+            "strict": self.strict,
         }
         # we need to set module-purge to None if it is False. We delete all keys  that are 'None' before writing to configuration file
         profile_configuration["module-purge"] = (
@@ -1063,6 +1069,7 @@ def load_profile(self):
         self.executor_type = profile_configuration.get("executor-type")
         self.max_jobs = profile_configuration.get("max_jobs")
         self.remove_stagedir = profile_configuration.get("remove-stagedir")
+        self.strict = profile_configuration.get("strict")
 
     def _validate_filters(self):
         """Check filter fields provided by ``buildtest build --filter`` are valid types and supported. Currently
@@ -1207,6 +1214,7 @@ def parse_buildspecs(self):
                 numnodes=self.numnodes,
                 executor_type=self.executor_type,
                 exclude_tags=self.exclude_tags,
+                strict=self.strict,
             )
 
             if not builder.get_builders():
diff --git a/buildtest/main.py b/buildtest/main.py
index 9ba03b20f..70efa69eb 100644
--- a/buildtest/main.py
+++ b/buildtest/main.py
@@ -325,6 +325,7 @@ def handle_build_command(args, configuration, report_file):
                 save_profile=args.save_profile,
                 profile=args.profile,
                 max_jobs=args.max_jobs,
+                strict=args.strict,
                 verbose=args.verbose,
                 write_config_file=args.write_config_file,
             )
diff --git a/buildtest/schemas/settings.schema.json b/buildtest/schemas/settings.schema.json
index 1ea1593fe..030b9f8b6 100644
--- a/buildtest/schemas/settings.schema.json
+++ b/buildtest/schemas/settings.schema.json
@@ -698,7 +698,8 @@
           "enum": ["local", "batch"]
         },
         "remove-stagedir": {"type": "boolean", "description": "Remove stage directory after test completes"},
-        "max-jobs": {"type": "integer", "minimum": 1, "description": "Maximum number of jobs that can be run at a given time for a particular executor"}
+        "max-jobs": {"type": "integer", "minimum": 1, "description": "Maximum number of jobs that can be run at a given time for a particular executor"},
+        "strict": {"type": "boolean", "description": "Enable strict mode for buildtest"}
       }
     },
     "description": {
diff --git a/buildtest/utils/file.py b/buildtest/utils/file.py
index 96a9e94c8..8eb14e6c2 100644
--- a/buildtest/utils/file.py
+++ b/buildtest/utils/file.py
@@ -77,10 +77,7 @@ def is_symlink(filename: str) -> bool:
     # apply user expansion when file includes something like  ~/example
     expanded_filepath = os.path.expanduser(expanded_filepath)
 
-    # resolve path will return canonical path eliminating any symbolic links encountered
-    resolved_sym_link = resolve_path(filename)
-
-    return os.path.islink(expanded_filepath) and resolved_sym_link
+    return os.path.islink(expanded_filepath)
 
 
 def search_files(
diff --git a/tutorials/strict_example.yml b/tutorials/strict_example.yml
new file mode 100644
index 000000000..e69de29bb

From f8bd63882a8085bce4bbca69e0caf29211a59236 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:31:09 -0400
Subject: [PATCH 02/13] add documentation on strict mode using example
 buildspec

---
 docs/gettingstarted/buildingtest.rst | 29 ++++++++++++++++++++++++++++
 tutorials/strict_example.yml         |  9 +++++++++
 2 files changed, 38 insertions(+)

diff --git a/docs/gettingstarted/buildingtest.rst b/docs/gettingstarted/buildingtest.rst
index d7f4f5182..87a7b0dff 100644
--- a/docs/gettingstarted/buildingtest.rst
+++ b/docs/gettingstarted/buildingtest.rst
@@ -617,3 +617,32 @@ then proceed to next test.
 .. dropdown:: ``buildtest build -b tutorials/hello_world.yml --rebuild=5 --max-jobs=2``
 
     .. command-output:: buildtest build -b tutorials/hello_world.yml --rebuild=5 --max-jobs=2
+
+Strict Mode
+------------
+
+Buildtest has an option to enable strict mode for test execution which can be enabled via ``--strict`` option. If this
+is set, buildtest will instead ``set -eo pipefail`` in the generated test which will cause test to exit immediately if any
+commands fail. To demonstrate this we have the following buildspec, which runs an **ls** command for an invalid path followed by
+an **echo** command.
+
+.. literalinclude:: ../tutorials/strict_example.yml
+    :language: yaml
+    :emphasize-lines: 8
+
+If we were to run this test without strict mode, we see the test will pass.
+
+.. dropdown:: ``buildtest build -b tutorials/strict_example.yml``
+
+    .. command-output:: buildtest build -b tutorials/strict_example.yml
+
+Now let's run the same test with strict mode enabled, we will see the test will fail with a different return code.
+
+.. dropdown:: ``buildtest build -b tutorials/strict_example.yml --strict``
+
+    .. commmand-output:: buildtest build -b tutorials/strict_example.yml --strict
+
+    We can see the generated test using **buildtest inspect query -t** and we will see the test script has **set -eo pipefail** in
+    the generated test.
+
+    .. command-output:: buildtest inspect query -t linux_strict_test
\ No newline at end of file
diff --git a/tutorials/strict_example.yml b/tutorials/strict_example.yml
index e69de29bb..1fa7b9249 100644
--- a/tutorials/strict_example.yml
+++ b/tutorials/strict_example.yml
@@ -0,0 +1,9 @@
+buildspecs:
+  linux_strict_test:
+    type: script
+    executor: generic.local.bash
+    description: "This example test will show how returncode will change when using --strict flag"
+    run: |
+      echo "This is a test"
+      ls -l /BAD_PATH
+      echo "This is another test"
\ No newline at end of file

From 7ecd2452eee6f9eba5dfec213ba6fdcc15e3d056 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:32:16 -0400
Subject: [PATCH 03/13] add '--strict' for bash completion

---
 bash_completion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bash_completion.sh b/bash_completion.sh
index bdc69777d..ab1ef1a65 100644
--- a/bash_completion.sh
+++ b/bash_completion.sh
@@ -190,7 +190,7 @@ _buildtest ()
   case "$next" in
     build|bd)
       local shortoption="-b -e -et -f -m -n -s -t -u -x -xt"
-      local longoption="--buildspec --dry-run --executor --executor-type --exclude --exclude-tags --filter --helpfilter --limit --maxpendtime --max-jobs --modules --module-purge --name --nodes --pollinterval --procs --profile --rerun --remove-stagedir --retry --save-profile --tags --timeout --unload-modules --validate --write-config-file"
+      local longoption="--buildspec --dry-run --executor --executor-type --exclude --exclude-tags --filter --helpfilter --limit --maxpendtime --max-jobs --modules --module-purge --name --nodes --pollinterval --procs --profile --rerun --remove-stagedir --retry --save-profile --strict --tags --timeout --unload-modules --validate --write-config-file"
       local allopts="${longoption} ${shortoption}"
 
       COMPREPLY=( $( compgen -W "$allopts" -- "${cur}" ) )

From 36da1f1ac412c817cc9bb091d8b3409dc7db5380 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:43:38 -0400
Subject: [PATCH 04/13] add regression test for strict mode and update marker
 names for cli

---
 tests/cli/test_build.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py
index 086666ea0..d91b55fdf 100644
--- a/tests/cli/test_build.py
+++ b/tests/cli/test_build.py
@@ -140,6 +140,7 @@ def test_invalid_build_filters(self):
                 filter_buildspecs={"type": "FOO"},
             )
 
+    @pytest.mark.cli
     def test_helpfilter(self):
         BuildTest(configuration=configuration, helpfilter=True)
 
@@ -251,7 +252,6 @@ def test_buildspecs_by_name(self):
         with pytest.raises(BuildTestError):
             BuildTest(configuration=configuration, name="pass_test")
 
-    @pytest.mark.cli
     def test_build_csh_executor(self):
         if not shutil.which("csh"):
             pytest.skip("Unable to run this test since it requires 'csh'")
@@ -260,7 +260,6 @@ def test_build_csh_executor(self):
         cmd = BuildTest(configuration=configuration, executors=["generic.local.csh"])
         cmd.build()
 
-    @pytest.mark.cli
     def test_skip_field(self):
         cmd = BuildTest(
             buildspecs=[os.path.join(BUILDTEST_ROOT, "tutorials", "skip_tests.yml")],
@@ -351,7 +350,6 @@ def test_verbose_mode(self):
         )
         cmd.build()
 
-    @pytest.mark.cli
     def test_invalid_buildspes(self):
         buildspec_file = [
             os.path.join(BUILDTEST_ROOT, "tutorials", "invalid_tags.yml"),
@@ -389,6 +387,14 @@ def test_remove_stagedir(self):
         )
         cmd.build()
 
+    @pytest.mark.cli
+    def test_with_strict_mode(self):
+        buildspecs = [os.path.join(BUILDTEST_ROOT, "tutorials", "strict_example.yml")]
+
+        cmd = BuildTest(configuration=configuration, buildspecs=buildspecs, strict=True)
+        cmd.build()
+
+    @pytest.mark.cli
     def test_save_profile(self):
         tf = tempfile.NamedTemporaryFile(suffix=".yml")
         print(configuration.file)

From 419cd06cd303d433931f5f602ca8f0919a5e9bf3 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:48:38 -0400
Subject: [PATCH 05/13] update profile generation test with 'strict' to ensure
 its captured in generated profile settings

---
 tests/cli/test_build.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py
index d91b55fdf..22719bc09 100644
--- a/tests/cli/test_build.py
+++ b/tests/cli/test_build.py
@@ -436,6 +436,7 @@ def test_save_profile(self):
             max_jobs=2,
             save_profile="demo",
             verbose=True,
+            strict=True,
         )
         profile_configuration = buildtest_configuration.get_profile(profile_name="demo")
         pprint(profile_configuration)

From 82bce2c440962ad07f2d8c20e20cdfa58f778ec6 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 15:56:10 -0400
Subject: [PATCH 06/13] we raise an exception when using 'buildtest build
 --write-config-file' that uses an extension type other than .yml. Added
 regression test to get coverage for this feature

---
 buildtest/cli/build.py  |  4 ++++
 tests/cli/test_build.py | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/buildtest/cli/build.py b/buildtest/cli/build.py
index 0ca45e44f..694536969 100644
--- a/buildtest/cli/build.py
+++ b/buildtest/cli/build.py
@@ -954,6 +954,10 @@ def save_profile_to_configuration(self):
                 raise BuildTestError(
                     f"[red]Configuration file {config_file_path} already exists. Please specify a new file path"
                 )
+            if not os.path.splitext(config_file_path)[1] == ".yml":
+                raise BuildTestError(
+                    f"[red]Configuration file {config_file_path} must end in .yml extension"
+                )
 
         config_file_path = config_file_path or self.configuration.file
         resolved_buildspecs = []
diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py
index 22719bc09..61c52882e 100644
--- a/tests/cli/test_build.py
+++ b/tests/cli/test_build.py
@@ -501,6 +501,19 @@ def test_save_profile_and_write_to_alternate_configuration_file(self):
                 write_config_file=tf.name,
             )
 
+        tf = tempfile.NamedTemporaryFile(suffix=".yaml")
+        tf.close()
+        # writing to file with .yaml extension is not allowed
+        with pytest.raises(BuildTestError):
+            BuildTest(
+                configuration=configuration,
+                buildspecs=[
+                    os.path.join(BUILDTEST_ROOT, "tutorials", "hello_world.yml")
+                ],
+                save_profile="demo",
+                write_config_file=tf.name,
+            )
+
     @pytest.mark.cli
     def test_retry(self):
         buildspecs = [

From 291a8fc17daa5939eea9b35f4ecc42f17c8244a4 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 16:31:52 -0400
Subject: [PATCH 07/13] add a method name check_container_runtime which can be
 called from anywhere. This allows us to invoke full path to container runtime
 after calling this method. This also works with container executors.

---
 buildtest/builders/base.py   | 142 +++--------------------------------
 buildtest/builders/script.py |  25 ++----
 buildtest/utils/tools.py     |  22 ++++++
 3 files changed, 38 insertions(+), 151 deletions(-)

diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py
index 7774bc668..539d521ae 100644
--- a/buildtest/builders/base.py
+++ b/buildtest/builders/base.py
@@ -37,9 +37,6 @@
 from buildtest.defaults import BUILDTEST_EXECUTOR_DIR, console
 from buildtest.exceptions import BuildTestError
 from buildtest.scheduler.job import Job
-from buildtest.scheduler.lsf import LSFJob
-from buildtest.scheduler.pbs import PBSJob
-from buildtest.scheduler.slurm import SlurmJob
 from buildtest.schemas.defaults import schema_table
 from buildtest.utils.command import BuildTestCommand
 from buildtest.utils.file import (
@@ -51,7 +48,7 @@
 )
 from buildtest.utils.shell import Shell, is_csh_shell
 from buildtest.utils.timer import Timer
-from buildtest.utils.tools import deep_get
+from buildtest.utils.tools import check_container_runtime, deep_get
 
 
 class BuilderBase(ABC):
@@ -569,7 +566,7 @@ def _write_build_script(self, modules=None, modulepurge=None, unload_modules=Non
 trap cleanup SIGINT SIGTERM SIGHUP SIGQUIT SIGABRT SIGKILL SIGALRM SIGPIPE SIGTERM SIGTSTP SIGTTIN SIGTTOU
 """
         lines.append(trap_msg)
-        lines += self._default_test_variables()
+        lines += self._set_default_test_variables()
         lines.append("# source executor startup script")
 
         if modulepurge:
@@ -595,8 +592,6 @@ def _write_build_script(self, modules=None, modulepurge=None, unload_modules=Non
             lines += [" ".join(self._emit_command())]
         elif self.is_container_executor():
             lines += self.get_container_invocation()
-            # lines += ["docker run -it --rm -v $PWD:$PWD -w $PWD " + f"-v {self.stage_dir}:/buildtest" + f" ubuntu bash -c {self.testpath}"]
-
         # batch executor
         else:
             launcher = self.buildexecutor.executors[self.executor].launcher_command(
@@ -661,9 +656,14 @@ def get_container_invocation(self):
         image = self.buildexecutor.executors[self.executor]._settings.get("image")
         options = self.buildexecutor.executors[self.executor]._settings.get("options")
         mounts = self.buildexecutor.executors[self.executor]._settings.get("mounts")
+
+        container_path = check_container_runtime(
+            platform, self.buildexecutor.configuration
+        )
+
         if platform in ["docker", "podman"]:
             lines += [
-                f"{platform} run -it --rm -v {self.stage_dir}:/buildtest -w /buildtest"
+                f"{container_path} run -it --rm -v {self.stage_dir}:/buildtest -w /buildtest"
             ]
 
             if mounts:
@@ -675,7 +675,7 @@ def get_container_invocation(self):
                 f"{image} bash -c {os.path.join('/buildtest', os.path.basename(self.testpath))}"
             ]
         elif platform == "singularity":
-            lines += [f"{platform} exec -B {self.stage_dir}/buildtest"]
+            lines += [f"{container_path} exec -B {self.stage_dir}/buildtest"]
             if mounts:
                 lines += [f"-B {mounts}"]
             if options:
@@ -715,7 +715,7 @@ def _emit_set_command(self):
 
         return ""
 
-    def _default_test_variables(self):
+    def _set_default_test_variables(self):
         """Return a list of lines inserted in build script that define buildtest specific variables
         that can be referenced when writing tests. The buildtest variables all start with BUILDTEST_*
         """
@@ -810,7 +810,7 @@ def _get_data_warp(self, datawarp):
             datawarp (str): Data Warp configuration specified by ``DW`` property in buildspec
 
         Returns:
-            list: List of string values containing containing ``#DW`` directives written in test
+            list: List of string values containing ``#DW`` directives written in test
         """
 
         if not datawarp:
@@ -1128,126 +1128,6 @@ def check_test_state(self):
             )
             self.metadata["result"]["state"] = "PASS" if state else "FAIL"
 
-    def check_test_state1(self):
-        """This method is responsible for detecting state of test (PASS/FAIL) based on returncode or regular expression."""
-
-        self.metadata["result"]["state"] = "FAIL"
-
-        if self.metadata["result"]["returncode"] == 0:
-            self.metadata["result"]["state"] = "PASS"
-
-        # if status is defined in Buildspec, then check for returncode and regex
-        if self.status:
-            # if 'state' property is specified explicitly honor this value regardless of what is calculated
-            if self.status.get("state"):
-                self.metadata["result"]["state"] = self.status["state"]
-                return
-
-            if "returncode" in self.status:
-                self.metadata["check"]["returncode"] = returncode_check(self)
-
-            # check regex against output or error stream based on regular expression defined in status property. Return value is a boolean
-            if self.status.get("regex"):
-                self.metadata["check"]["regex"] = regex_check(self)
-
-            if self.status.get("runtime"):
-                self.metadata["check"]["runtime"] = runtime_check(self)
-
-            if self.status.get("file_regex"):
-                self.metadata["check"]["file_regex"] = file_regex_check(self)
-
-            if self.status.get("slurm_job_state") and isinstance(self.job, SlurmJob):
-                self.metadata["check"]["slurm_job_state"] = (
-                    self.status["slurm_job_state"] == self.job.state()
-                )
-
-            if self.status.get("pbs_job_state") and isinstance(self.job, PBSJob):
-                self.metadata["check"]["pbs_job_state"] = (
-                    self.status["pbs_job_state"] == self.job.state()
-                )
-
-            if self.status.get("lsf_job_state") and isinstance(self.job, LSFJob):
-                self.metadata["check"]["lsf_job_state"] = (
-                    self.status["lsf_job_state"] == self.job.state()
-                )
-
-            if self.status.get("assert_ge"):
-                self.metadata["check"]["assert_ge"] = comparison_check(
-                    builder=self, comparison_type="ge"
-                )
-
-            if self.status.get("assert_le"):
-                self.metadata["check"]["assert_le"] = comparison_check(
-                    builder=self, comparison_type="le"
-                )
-
-            if self.status.get("assert_gt"):
-                self.metadata["check"]["assert_gt"] = comparison_check(
-                    builder=self, comparison_type="gt"
-                )
-
-            if self.status.get("assert_lt"):
-                self.metadata["check"]["assert_lt"] = comparison_check(
-                    builder=self, comparison_type="lt"
-                )
-
-            if self.status.get("assert_eq"):
-                self.metadata["check"]["assert_eq"] = comparison_check(
-                    builder=self, comparison_type="eq"
-                )
-
-            if self.status.get("assert_ne"):
-                self.metadata["check"]["assert_ne"] = comparison_check(
-                    builder=self, comparison_type="ne"
-                )
-
-            if self.status.get("assert_range"):
-                self.metadata["check"]["assert_range"] = assert_range_check(self)
-
-            if self.status.get("contains"):
-                self.metadata["check"]["contains"] = contains_check(
-                    builder=self, comparison_type="contains"
-                )
-
-            if self.status.get("not_contains"):
-                self.metadata["check"]["not_contains"] = contains_check(
-                    builder=self, comparison_type="not_contains"
-                )
-
-            if self.status.get("is_symlink"):
-                self.metadata["check"]["is_symlink"] = is_symlink_check(builder=self)
-
-            if self.status.get("exists"):
-                self.metadata["check"]["exists"] = exists_check(builder=self)
-
-            if self.status.get("is_dir"):
-                self.metadata["check"]["is_dir"] = is_dir_check(builder=self)
-
-            if self.status.get("is_file"):
-                self.metadata["check"]["is_file"] = is_file_check(builder=self)
-
-            if self.status.get("file_count"):
-                self.metadata["check"]["file_count"] = file_count_check(builder=self)
-
-            if self.status.get("linecount"):
-                self.metadata["check"]["linecount"] = linecount_check(builder=self)
-
-            if self.status.get("file_linecount"):
-                self.metadata["check"]["file_linecount"] = file_linecount_check(
-                    builder=self
-                )
-            # filter out any None values from status check
-            status_checks = [
-                value for value in self.metadata["check"].values() if value is not None
-            ]
-
-            state = (
-                all(status_checks)
-                if self.status.get("mode") in ["AND", "and"]
-                else any(status_checks)
-            )
-            self.metadata["result"]["state"] = "PASS" if state else "FAIL"
-
     def _process_compiler_config(self):
         """This method is responsible for setting cc, fc, cxx class variables based
         on compiler selection. The order of precedence is ``config``, ``default``,
diff --git a/buildtest/builders/script.py b/buildtest/builders/script.py
index 033b7b254..b2639d11c 100644
--- a/buildtest/builders/script.py
+++ b/buildtest/builders/script.py
@@ -3,10 +3,9 @@
 import shutil
 
 from buildtest.builders.base import BuilderBase
-from buildtest.exceptions import BuildTestError
 from buildtest.tools.modules import get_module_commands
 from buildtest.utils.file import write_file
-from buildtest.utils.tools import check_binaries, deep_get
+from buildtest.utils.tools import check_container_runtime, deep_get
 
 
 class ScriptBuilder(BuilderBase):
@@ -198,13 +197,12 @@ def _get_container_command(self):
         container_runtime = container_config["platform"]
         container_command = []
 
+        container_path = check_container_runtime(container_runtime, self.configuration)
         container_launch_command = {
-            "docker": ["docker", "run", "-v"],
-            "podman": ["podman", "run", "-v"],
-            "singularity": ["singularity", "run", "-B"],
+            "docker": [container_path, "run", "--rm", "-v"],
+            "podman": [container_path, "run", "--rm", "-v"],
+            "singularity": [container_path, "run", "-B"],
         }
-
-        self._check_binary_path(container_runtime)
         container_command.extend(container_launch_command[container_runtime])
         container_command.append(f"{self.stage_dir}:/buildtest")
 
@@ -221,16 +219,3 @@ def _get_container_command(self):
             container_command.append(container_config["command"])
 
         return container_command
-
-    def _check_binary_path(self, platform):
-        binary_path = check_binaries(
-            [platform],
-            custom_dirs=deep_get(self.configuration.target_config, "paths", platform),
-        )
-
-        if not binary_path[platform]:
-            raise BuildTestError(
-                f"[blue]{self}[/blue]: [red]Unable to find {platform} binary in PATH, this test will be not be executed.[/red]"
-            )
-
-        return binary_path[platform]
diff --git a/buildtest/utils/tools.py b/buildtest/utils/tools.py
index 5fc699919..77584f0e2 100644
--- a/buildtest/utils/tools.py
+++ b/buildtest/utils/tools.py
@@ -5,6 +5,7 @@
 
 from rich.color import Color, ColorParseError
 
+from buildtest.exceptions import BuildTestError
 from buildtest.utils.file import is_dir, resolve_path
 
 logger = logging.getLogger(__name__)
@@ -76,3 +77,24 @@ def check_binaries(binaries, custom_dirs=None):
         logger.debug(f"{command}: {command_fpath}")
 
     return sched_dict
+
+
+def check_container_runtime(platform, configuration):
+    """Check if container runtime exists in $PATH and any additional directories specified by
+    custom_dirs. The return is a dictionary
+
+    Args:
+        platform (str): platform to check for container runtime
+        configuration (dict): configuration dictionary
+    """
+
+    binary_path = check_binaries(
+        [platform], custom_dirs=deep_get(configuration.target_config, "paths", platform)
+    )
+
+    if not binary_path[platform]:
+        raise BuildTestError(
+            f"[red]Unable to find {platform} binary in PATH, this test will be not be executed.[/red]"
+        )
+
+    return binary_path[platform]

From 70af06771b9b261792c32bc7c3852ba24161bb5b Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Tue, 14 May 2024 16:33:57 -0400
Subject: [PATCH 08/13] remove reference to 'cobalt' in json schema which is
 not used in buildtest anymore

---
 buildtest/schemas/definitions.schema.json | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/buildtest/schemas/definitions.schema.json b/buildtest/schemas/definitions.schema.json
index 6d7542043..18fa0d696 100644
--- a/buildtest/schemas/definitions.schema.json
+++ b/buildtest/schemas/definitions.schema.json
@@ -672,10 +672,6 @@
       "$ref": "#/definitions/list_of_strings",
       "description": "This field is used for specifying #BSUB options in test script."
     },
-    "cobalt": {
-      "$ref": "#/definitions/list_of_strings",
-      "description": "This field is used for specifying #COBALT options in test script."
-    },
     "pbs": {
       "$ref": "#/definitions/list_of_strings",
       "description": "This field is used for specifying #PBS directives in test script."
@@ -703,9 +699,6 @@
             "pbs": {
               "$ref": "#/definitions/list_of_strings"
             },
-            "cobalt": {
-              "$ref": "#/definitions/list_of_strings"
-            },
             "BB": {
               "$ref": "#/definitions/BB"
             },

From 6a8b7252688f1e09f451249ed425a591506badff Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Wed, 15 May 2024 10:11:06 -0400
Subject: [PATCH 09/13] fix bug in regression test when calling lambda function

---
 buildtest/builders/base.py | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py
index 539d521ae..c8b17cbe5 100644
--- a/buildtest/builders/base.py
+++ b/buildtest/builders/base.py
@@ -1077,30 +1077,30 @@ def check_test_state(self):
                 "regex": regex_check,
                 "runtime": runtime_check,
                 "file_regex": file_regex_check,
-                "assert_ge": lambda: comparison_check(
-                    builder=self, comparison_type="ge"
+                "assert_ge": lambda builder: comparison_check(
+                    builder=builder, comparison_type="ge"
                 ),
-                "assert_le": lambda: comparison_check(
-                    builder=self, comparison_type="le"
+                "assert_le": lambda builder: comparison_check(
+                    builder=builder, comparison_type="le"
                 ),
-                "assert_gt": lambda: comparison_check(
-                    builder=self, comparison_type="gt"
+                "assert_gt": lambda builder: comparison_check(
+                    builder=builder, comparison_type="gt"
                 ),
-                "assert_lt": lambda: comparison_check(
-                    builder=self, comparison_type="lt"
+                "assert_lt": lambda builder: comparison_check(
+                    builder=builder, comparison_type="lt"
                 ),
-                "assert_eq": lambda: comparison_check(
-                    builder=self, comparison_type="eq"
+                "assert_eq": lambda builder: comparison_check(
+                    builder=builder, comparison_type="eq"
                 ),
-                "assert_ne": lambda: comparison_check(
-                    builder=self, comparison_type="ne"
+                "assert_ne": lambda builder: comparison_check(
+                    builder=builder, comparison_type="ne"
                 ),
                 "assert_range": assert_range_check,
-                "contains": lambda: contains_check(
-                    builder=self, comparison_type="contains"
+                "contains": lambda builder: contains_check(
+                    builder=builder, comparison_type="contains"
                 ),
-                "not_contains": lambda: contains_check(
-                    builder=self, comparison_type="not_contains"
+                "not_contains": lambda builder: contains_check(
+                    builder=builder, comparison_type="not_contains"
                 ),
                 "is_symlink": is_symlink_check,
                 "exists": exists_check,

From 5150b0a58103f7410fcdd29af92d015e6f7338c1 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Wed, 15 May 2024 10:19:24 -0400
Subject: [PATCH 10/13] address build error in documentation with unknown
 directive type. This was a spelling error

---
 docs/gettingstarted/buildingtest.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/gettingstarted/buildingtest.rst b/docs/gettingstarted/buildingtest.rst
index 87a7b0dff..04cff5ef9 100644
--- a/docs/gettingstarted/buildingtest.rst
+++ b/docs/gettingstarted/buildingtest.rst
@@ -640,7 +640,7 @@ Now let's run the same test with strict mode enabled, we will see the test will
 
 .. dropdown:: ``buildtest build -b tutorials/strict_example.yml --strict``
 
-    .. commmand-output:: buildtest build -b tutorials/strict_example.yml --strict
+    .. command-output:: buildtest build -b tutorials/strict_example.yml --strict
 
     We can see the generated test using **buildtest inspect query -t** and we will see the test script has **set -eo pipefail** in
     the generated test.

From 8a83141ff3121d15907f0aa063c1276d75827299 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Wed, 15 May 2024 11:26:46 -0400
Subject: [PATCH 11/13] fix issue where broken symbolic links were not working
 we needed to add os.path.exists

---
 buildtest/utils/file.py  |  2 +-
 tests/utils/test_file.py | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/buildtest/utils/file.py b/buildtest/utils/file.py
index 8eb14e6c2..8fe06b397 100644
--- a/buildtest/utils/file.py
+++ b/buildtest/utils/file.py
@@ -77,7 +77,7 @@ def is_symlink(filename: str) -> bool:
     # apply user expansion when file includes something like  ~/example
     expanded_filepath = os.path.expanduser(expanded_filepath)
 
-    return os.path.islink(expanded_filepath)
+    return os.path.islink(expanded_filepath) and os.path.exists(expanded_filepath)
 
 
 def search_files(
diff --git a/tests/utils/test_file.py b/tests/utils/test_file.py
index 35368bd50..039f90907 100644
--- a/tests/utils/test_file.py
+++ b/tests/utils/test_file.py
@@ -43,18 +43,18 @@ def test_checking_file():
 @pytest.mark.utility
 def test_is_symlink():
     # Target path of the symbolic link
-    link_target = tempfile.NamedTemporaryFile(dir=os.path.expanduser("~"))
+    source_link = tempfile.NamedTemporaryFile(dir=os.path.expanduser("~"))
 
     # Symbolic link path
-    link_path = tempfile.NamedTemporaryFile(dir=os.path.expanduser("~"))
+    dest_link = tempfile.NamedTemporaryFile(dir=os.path.expanduser("~"))
 
-    link_path.close()
+    dest_link.close()
 
     # Create symbolic link
-    os.symlink(link_target.name, link_path.name)
+    os.symlink(source_link.name, dest_link.name)
 
     # get filename from the path
-    filename = os.path.basename(link_path.name)
+    filename = os.path.basename(dest_link.name)
 
     # test for shell expansion
     assert is_symlink(os.path.join("$HOME", filename))
@@ -63,16 +63,16 @@ def test_is_symlink():
     assert is_symlink(os.path.join("~", filename))
 
     # test when link is not a symbolic link
-    assert not is_symlink(link_target.name)
+    assert not is_symlink(source_link.name)
 
     # delete the target file path
-    link_target.close()
+    source_link.close()
 
     # test for broken symbolic link
-    assert not is_symlink(link_path.name)
+    assert not is_symlink(dest_link.name)
 
     # remove the symbolic link
-    os.remove(link_path.name)
+    os.remove(dest_link.name)
 
 
 @pytest.mark.utility

From a90e762bdeceb0c79675a33f1ad76dd97ca0f314 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Wed, 15 May 2024 12:21:22 -0400
Subject: [PATCH 12/13] refactor logic for run method by decompossing it into
 several methods

---
 buildtest/builders/base.py | 170 +++++++++++++++++++------------------
 1 file changed, 89 insertions(+), 81 deletions(-)

diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py
index c8b17cbe5..f565cb1fd 100644
--- a/buildtest/builders/base.py
+++ b/buildtest/builders/base.py
@@ -152,7 +152,7 @@ def __init__(
 
         self._set_metadata_values()
         self.shell_detection()
-        self.sched_init()
+        self.set_scheduler_settings()
 
     @property
     def dependency(self):
@@ -175,7 +175,7 @@ def shell_detection(self):
         self.shebang = (
             self.recipe.get("shebang") or f"{self.shell.shebang} {self.shell.opts}"
         )
-        self.logger.debug("Using shell %s", self.shell.name)
+        self.logger.debug(f"Using shell {self.shell.name}")
         self.logger.debug(f"Shebang used for test: {self.shebang}")
 
     def _set_metadata_values(self):
@@ -330,8 +330,8 @@ def build(self, modules=None, modulepurge=None, unload_modules=None):
         self._write_build_script(modules, modulepurge, unload_modules)
 
     def run(self, cmd, timeout=None):
-        """Run the test and record the starttime and start timer. We also return the instance
-        object of type BuildTestCommand which is used by Executors for processing output and error
+        """This is the entry point for running the test. This method will prepare test to be run, then
+        run the test. Once test is complete, we also handle test results by capturing output and error.
 
         Returns:
             If success, the return type is an object of type :class:`buildtest.utils.command.BuildTestCommand`
@@ -339,42 +339,54 @@ def run(self, cmd, timeout=None):
             If their is a failure (non-zero) returncode we retry test and if it doesn't pass we
             raise exception of :class:`buildtest.exceptions.RuntimeFailure`
         """
+        self.prepare_run(cmd)
+        command_result = self.execute_run(cmd, timeout)
+        run_result = self.handle_run_result(cmd, timeout, command_result)
+        return run_result
+
+    def prepare_run(self, cmd):
+        """This method prepares the test to be run by recording starttime, setting state to running and starting the timer.
+        In additional we will write build environment into build-env.txt which is used for debugging purposes.
+        """
 
         self.metadata["command"] = cmd
-
         console.print(f"[blue]{self}[/]: Current Working Directory : {os.getcwd()}")
-        # capture output of 'env' and write to file 'build-env.sh' prior to running test
         command = BuildTestCommand("env")
         command.execute()
         content = "".join(command.get_output())
         self.metadata["buildenv"] = os.path.join(self.test_root, "build-env.txt")
         write_file(self.metadata["buildenv"], content)
-
         console.print(f"[blue]{self}[/]: Running Test via command: [cyan]{cmd}[/cyan]")
-
         self.record_starttime()
         self.running()
         self.start()
 
+    def execute_run(self, cmd, timeout):
+        """This method will execute the test and return the instance object of type
+        BuildTestCommand which is used by Executors for processing output and error"""
+
         command = BuildTestCommand(cmd)
         command.execute(timeout=timeout)
+        return command
+
+    def handle_run_result(self, cmd, timeout, command_result):
+        """This method will handle the result of running test. If the test is successful we will record endtime,
+        copy output and error file to test directory and set state to complete. If the test fails we will retry the test based on retry count.
+        If the test fails after retry we will mark test as failed.
+        """
 
         self.logger.debug(f"Running Test via command: {cmd}")
-        ret = command.returncode()
-        err_msg = command.get_error()
-        # limit error messages to 60 lines
+        ret = command_result.returncode()
+        err_msg = command_result.get_error()
+
         if len(err_msg) >= 60:
             err_msg = err_msg[-60:]
-
         if not self._retry or ret == 0:
-            return command
+            return cmd
 
         console.print(f"[red]{self}: failed to submit job with returncode: {ret}")
         console.rule(f"[red]Error Message for {self}")
         console.print(f"[red]{' '.join(err_msg)}")
-
-        ########## Retry for failed tests  ##########
-
         console.print(
             f"[red]{self}: Detected failure in running test, will attempt to retry test: {self._retry} times"
         )
@@ -385,11 +397,8 @@ def run(self, cmd, timeout=None):
                 f"[blue]{self}[/]: Running Test via command: [cyan]{cmd}[/cyan]"
             )
             command.execute(timeout=timeout)
-
             self.logger.debug(f"Running Test via command: {cmd}")
             ret = command.returncode()
-
-            # if we recieve a returncode of 0 return immediately with the instance of command
             if ret == 0:
                 return command
             console.print(f"[red]{self}: failed to submit job with returncode: {ret}")
@@ -498,11 +507,6 @@ def _build_setup(self):
 
         create_dir(self.testdir)
 
-        # num_content = len(os.listdir(self.testdir))
-        # the testid is incremented for every run, this can be done by getting
-        # length of all files in testdir and creating a directory. Subsequent
-        # runs will increment this counter
-
         self.test_root = os.path.join(self.testdir, self.testid[:8])
 
         create_dir(self.test_root)
@@ -522,10 +526,10 @@ def _build_setup(self):
         self.metadata["stagedir"] = self.stage_dir
 
         # Derive the path to the test script
-        self.testpath = "%s.%s" % (
-            os.path.join(self.stage_dir, self.name),
-            self.get_test_extension(),
+        self.testpath = (
+            os.path.join(self.stage_dir, self.name) + "." + self.get_test_extension()
         )
+
         self.testpath = os.path.expandvars(self.testpath)
 
         self.metadata["testpath"] = os.path.join(
@@ -621,8 +625,6 @@ def _write_build_script(self, modules=None, modulepurge=None, unload_modules=Non
         self.build_script = dest
         self.metadata["build_script"] = self.build_script
 
-        # console.print(f"[blue]{self}:[/] Writing build script: {self.build_script}")
-
     def _write_test(self):
         """This method is responsible for invoking ``generate_script`` that
         formulates content of testscript which is implemented in each subclass.
@@ -734,7 +736,7 @@ def _set_default_test_variables(self):
 
         return lines
 
-    def sched_init(self):
+    def set_scheduler_settings(self):
         """This method will resolve scheduler fields: 'sbatch', 'pbs', 'bsub'"""
         self.sbatch = deep_get(
             self.recipe, "executors", self.executor, "sbatch"
@@ -935,7 +937,7 @@ def _extract_line(self, linenum, content):
 
     def add_metrics(self):
         """This method will update the metrics field stored in ``self.metadata['metrics']``. The ``metrics``
-        property can be defined in the buildspdec to assign value to a metrics name based on regular expression,
+        property can be defined in the buildspec to assign value to a metrics name based on regular expression,
         environment or variable assignment.
         """
 
@@ -943,67 +945,73 @@ def add_metrics(self):
             return
 
         for key, metric in self.metrics.items():
-            # Default value of metric is an empty string
             self.metadata["metrics"][key] = ""
             regex = metric.get("regex")
             file_regex = metric.get("file_regex")
 
             if regex:
+                self.handle_regex_metric(key, regex)
+            elif file_regex:
+                self.handle_file_regex_metric(key, file_regex)
 
-                stream = regex.get("stream")
-                content_input = self._output if stream == "stdout" else self._error
+            self.metadata["metrics"][key] = str(self.metadata["metrics"][key])
 
-                linenum = regex.get("linenum")
-                content = self._extract_line(linenum, content_input)
+    def handle_regex_metric(self, key, regex):
+        """Handle metrics based on regular expressions."""
 
-                if regex.get("re") == "re.match":
-                    match = re.match(regex["exp"], content, re.MULTILINE)
-                elif regex.get("re") == "re.fullmatch":
-                    match = re.fullmatch(regex["exp"], content, re.MULTILINE)
-                else:
-                    match = re.search(regex["exp"], content, re.MULTILINE)
-                if match:
-                    try:
-                        self.metadata["metrics"][key] = match.group(
-                            regex.get("item", 0)
-                        )
-                    except IndexError:
-                        self.logger.error(
-                            f"Unable to fetch match group: {regex.get('item', 0)} for metric: {key}."
-                        )
-                        continue
-            elif file_regex:
-                fname = file_regex["file"]
-                if fname:
-                    resolved_fname = resolve_path(fname)
-                    if not is_file(resolved_fname):
-                        msg = f"[blue]{self}[/]: Unable to resolve file path: {fname} for metric: {key}"
-                        self.logger.error(msg)
-                        console.print(msg, style="red")
-                        continue
-
-                    linenum = file_regex.get("linenum")
-                    content_input = read_file(resolved_fname)
-                    content = self._extract_line(linenum, content_input)
-
-                    match = (
-                        re.search(file_regex["exp"], content, re.MULTILINE)
-                        if content
-                        else None
+        stream = regex.get("stream")
+        content_input = self._output if stream == "stdout" else self._error
+
+        linenum = regex.get("linenum")
+        content = self._extract_line(linenum, content_input)
+
+        match = self.get_match(regex, content)
+        if match:
+            try:
+                self.metadata["metrics"][key] = match.group(regex.get("item", 0))
+            except IndexError:
+                self.logger.error(
+                    f"Unable to fetch match group: {regex.get('item', 0)} for metric: {key}."
+                )
+
+    def handle_file_regex_metric(self, key, file_regex):
+        """Handle metrics based on file regular expressions."""
+
+        fname = file_regex["file"]
+        if fname:
+            resolved_fname = resolve_path(fname)
+            if not is_file(resolved_fname):
+                msg = f"[blue]{self}[/]: Unable to resolve file path: {fname} for metric: {key}"
+                self.logger.error(msg)
+                console.print(msg, style="red")
+                return
+
+            linenum = file_regex.get("linenum")
+            content_input = read_file(resolved_fname)
+            content = self._extract_line(linenum, content_input)
+
+            match = (
+                re.search(file_regex["exp"], content, re.MULTILINE) if content else None
+            )
+            if match:
+                try:
+                    self.metadata["metrics"][key] = match.group(
+                        file_regex.get("item", 0)
+                    )
+                except IndexError:
+                    self.logger.error(
+                        f"Unable to fetch match group: {file_regex.get('item', 0)} for metric: {key}."
                     )
 
-                    if match:
-                        try:
-                            self.metadata["metrics"][key] = match.group(
-                                file_regex.get("item", 0)
-                            )
-                        except IndexError:
-                            self.logger.error(
-                                f"Unable to fetch match group: {file_regex.get('item', 0)} for metric: {key}."
-                            )
-                            continue
+    def get_match(self, regex, content):
+        """Get the match based on the regular expression."""
 
-            self.metadata["metrics"][key] = str(self.metadata["metrics"][key])
+        if regex.get("re") == "re.match":
+            return re.match(regex["exp"], content, re.MULTILINE)
+        elif regex.get("re") == "re.fullmatch":
+            return re.fullmatch(regex["exp"], content, re.MULTILINE)
+        else:
+            return re.search(regex["exp"], content, re.MULTILINE)
 
     def output(self):
         """Return output content"""

From 00c63acc65b642fae40e5c7caba4bb36ceb41d83 Mon Sep 17 00:00:00 2001
From: Shahzeb Siddiqui <shahzebmsiddiqui@gmail.com>
Date: Wed, 15 May 2024 12:43:49 -0400
Subject: [PATCH 13/13] fix some bugs reported in regression test related to
 how running test and handling of command object in method handle_run_result

---
 buildtest/builders/base.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/buildtest/builders/base.py b/buildtest/builders/base.py
index f565cb1fd..13f9da2ff 100644
--- a/buildtest/builders/base.py
+++ b/buildtest/builders/base.py
@@ -341,7 +341,7 @@ def run(self, cmd, timeout=None):
         """
         self.prepare_run(cmd)
         command_result = self.execute_run(cmd, timeout)
-        run_result = self.handle_run_result(cmd, timeout, command_result)
+        run_result = self.handle_run_result(command_result, timeout)
         return run_result
 
     def prepare_run(self, cmd):
@@ -369,20 +369,20 @@ def execute_run(self, cmd, timeout):
         command.execute(timeout=timeout)
         return command
 
-    def handle_run_result(self, cmd, timeout, command_result):
+    def handle_run_result(self, command_result, timeout):
         """This method will handle the result of running test. If the test is successful we will record endtime,
         copy output and error file to test directory and set state to complete. If the test fails we will retry the test based on retry count.
         If the test fails after retry we will mark test as failed.
         """
-
-        self.logger.debug(f"Running Test via command: {cmd}")
+        launch_command = command_result.get_command()
+        self.logger.debug(f"Running Test via command: {launch_command}")
         ret = command_result.returncode()
         err_msg = command_result.get_error()
 
         if len(err_msg) >= 60:
             err_msg = err_msg[-60:]
         if not self._retry or ret == 0:
-            return cmd
+            return command_result
 
         console.print(f"[red]{self}: failed to submit job with returncode: {ret}")
         console.rule(f"[red]Error Message for {self}")
@@ -392,12 +392,12 @@ def handle_run_result(self, cmd, timeout, command_result):
         )
         for run in range(1, self._retry + 1):
             print(f"{self}: Run - {run}/{self._retry}")
-            command = BuildTestCommand(cmd)
+            command = self.execute_run(launch_command, timeout)
+
             console.print(
-                f"[blue]{self}[/]: Running Test via command: [cyan]{cmd}[/cyan]"
+                f"[blue]{self}[/]: Running Test via command: [cyan]{launch_command}[/cyan]"
             )
-            command.execute(timeout=timeout)
-            self.logger.debug(f"Running Test via command: {cmd}")
+            self.logger.debug(f"Running Test via command: {launch_command}")
             ret = command.returncode()
             if ret == 0:
                 return command