From 9b38273b537b5e41b37a720d5826db880b480098 Mon Sep 17 00:00:00 2001
From: Sean Bryan <sean.bryan@anu.edu.au>
Date: Wed, 11 Oct 2023 16:51:28 +1100
Subject: [PATCH] Implement remaining files

---
 tests/test_fs.py         |  77 +++++++--------
 tests/test_pbs.py        | 132 ++++++++++++-------------
 tests/test_subprocess.py | 203 ++++++++++++++++++++++-----------------
 tests/test_workdir.py    |  59 ++++++------
 4 files changed, 244 insertions(+), 227 deletions(-)

diff --git a/tests/test_fs.py b/tests/test_fs.py
index dd77015b..be55cf19 100644
--- a/tests/test_fs.py
+++ b/tests/test_fs.py
@@ -13,52 +13,47 @@
 
 from benchcab.utils.fs import mkdir, next_path
 
-pytest.skip(allow_module_level=True)
 
-
-from .common import MOCK_CWD
-
-
-def test_next_path():
+class TestNextPath:
     """Tests for `next_path()`."""
-    pattern = "rev_number-*.log"
 
-    # Success case: get next path in 'empty' CWD
-    assert len(list(MOCK_CWD.glob(pattern))) == 0
-    ret = next_path(MOCK_CWD, pattern)
-    assert ret == "rev_number-1.log"
+    @pytest.fixture()
+    def pattern(self):
+        """Return a file pattern for testing against."""
+        return "rev_number-*.log"
 
-    # Success case: get next path in 'non-empty' CWD
-    ret_path = MOCK_CWD / ret
-    ret_path.touch()
-    assert len(list(MOCK_CWD.glob(pattern))) == 1
-    ret = next_path(MOCK_CWD, pattern)
-    assert ret == "rev_number-2.log"
+    def test_next_path_in_empty_cwd(self, pattern, mock_cwd):
+        """Success case: get next path in 'empty' CWD."""
+        assert next_path(mock_cwd, pattern) == "rev_number-1.log"
 
+    def test_next_path_in_non_empty_cwd(self, pattern, mock_cwd):
+        """Success case: get next path in 'non-empty' CWD."""
+        (mock_cwd / next_path(mock_cwd, pattern)).touch()
+        assert next_path(mock_cwd, pattern) == "rev_number-2.log"
 
-@pytest.mark.parametrize(
-    "test_path,kwargs",
-    [
-        (Path(MOCK_CWD, "test1"), {}),
-        (Path(MOCK_CWD, "test1/test2"), dict(parents=True)),
-        (Path(MOCK_CWD, "test1/test2"), dict(parents=True, exist_ok=True)),
-    ],
-)
-def test_mkdir(test_path, kwargs):
-    """Tests for `mkdir()`."""
 
-    # Success case: create a test directory
-    mkdir(test_path, **kwargs)
-    assert test_path.exists()
-    test_path.rmdir()
-
-
-def test_mkdir_verbose():
-    """Tests for verbose output of `mkdir()`."""
+class TestMkdir:
+    """Tests for `mkdir()`."""
 
-    # Success case: verbose output
-    test_path = Path(MOCK_CWD, "test1")
-    with contextlib.redirect_stdout(io.StringIO()) as buf:
-        mkdir(test_path, verbose=True)
-    assert buf.getvalue() == (f"Creating {test_path} directory\n")
-    test_path.rmdir()
+    @pytest.mark.parametrize(
+        ("test_path", "kwargs"),
+        [
+            (Path("test1"), {}),
+            (Path("test1/test2"), dict(parents=True)),
+            (Path("test1/test2"), dict(parents=True, exist_ok=True)),
+        ],
+    )
+    def test_mkdir(self, test_path, kwargs):
+        """Success case: create a test directory."""
+        mkdir(test_path, **kwargs)
+        assert test_path.exists()
+        test_path.rmdir()
+
+    @pytest.mark.parametrize(
+        ("verbosity", "expected"), [(False, ""), (True, "Creating test1 directory\n")]
+    )
+    def test_standard_output(self, verbosity, expected):
+        """Success case: test standard output."""
+        with contextlib.redirect_stdout(io.StringIO()) as buf:
+            mkdir(Path("test1"), verbose=verbosity)
+        assert buf.getvalue() == expected
diff --git a/tests/test_pbs.py b/tests/test_pbs.py
index 522de476..c12e3e0d 100644
--- a/tests/test_pbs.py
+++ b/tests/test_pbs.py
@@ -1,23 +1,21 @@
 """`pytest` tests for `utils/pbs.py`."""
 
-import pytest
-
-pytest.skip(allow_module_level=True)
-
 from benchcab import internal
 from benchcab.utils.pbs import render_job_script
 
 
-def test_render_job_script():
+class TestRenderJobScript:
     """Tests for `render_job_script()`."""
-    # Success case: test default job script generated is correct
-    assert render_job_script(
-        project="tm70",
-        config_path="/path/to/config.yaml",
-        modules=["foo", "bar", "baz"],
-        benchcab_path="/absolute/path/to/benchcab",
-    ) == (
-        f"""#!/bin/bash
+
+    def test_default_job_script(self):
+        """Success case: test default job script generated is correct."""
+        assert render_job_script(
+            project="tm70",
+            config_path="/path/to/config.yaml",
+            modules=["foo", "bar", "baz"],
+            benchcab_path="/absolute/path/to/benchcab",
+        ) == (
+            f"""#!/bin/bash
 #PBS -l wd
 #PBS -l ncpus={internal.FLUXSITE_DEFAULT_PBS["ncpus"]}
 #PBS -l mem={internal.FLUXSITE_DEFAULT_PBS["mem"]}
@@ -40,17 +38,18 @@ def test_render_job_script():
 /absolute/path/to/benchcab fluxsite-bitwise-cmp --config=/path/to/config.yaml 
 
 """
-    )
-
-    # Success case: test verbose flag is added to command line arguments
-    assert render_job_script(
-        project="tm70",
-        config_path="/path/to/config.yaml",
-        modules=["foo", "bar", "baz"],
-        verbose=True,
-        benchcab_path="/absolute/path/to/benchcab",
-    ) == (
-        f"""#!/bin/bash
+        )
+
+    def test_verbose_flag_added_to_command_line_arguments(self):
+        """Success case: test verbose flag is added to command line arguments."""
+        assert render_job_script(
+            project="tm70",
+            config_path="/path/to/config.yaml",
+            modules=["foo", "bar", "baz"],
+            verbose=True,
+            benchcab_path="/absolute/path/to/benchcab",
+        ) == (
+            f"""#!/bin/bash
 #PBS -l wd
 #PBS -l ncpus={internal.FLUXSITE_DEFAULT_PBS["ncpus"]}
 #PBS -l mem={internal.FLUXSITE_DEFAULT_PBS["mem"]}
@@ -73,17 +72,18 @@ def test_render_job_script():
 /absolute/path/to/benchcab fluxsite-bitwise-cmp --config=/path/to/config.yaml -v
 
 """
-    )
-
-    # Success case: skip fluxsite-bitwise-cmp step
-    assert render_job_script(
-        project="tm70",
-        config_path="/path/to/config.yaml",
-        modules=["foo", "bar", "baz"],
-        skip_bitwise_cmp=True,
-        benchcab_path="/absolute/path/to/benchcab",
-    ) == (
-        f"""#!/bin/bash
+        )
+
+    def test_skip_bitwise_comparison_step(self):
+        """Success case: skip fluxsite-bitwise-cmp step."""
+        assert render_job_script(
+            project="tm70",
+            config_path="/path/to/config.yaml",
+            modules=["foo", "bar", "baz"],
+            skip_bitwise_cmp=True,
+            benchcab_path="/absolute/path/to/benchcab",
+        ) == (
+            f"""#!/bin/bash
 #PBS -l wd
 #PBS -l ncpus={internal.FLUXSITE_DEFAULT_PBS["ncpus"]}
 #PBS -l mem={internal.FLUXSITE_DEFAULT_PBS["mem"]}
@@ -104,23 +104,24 @@ def test_render_job_script():
 /absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml 
 
 """
-    )
-
-    # Success case: specify parameters in pbs_config
-    assert render_job_script(
-        project="tm70",
-        config_path="/path/to/config.yaml",
-        modules=["foo", "bar", "baz"],
-        skip_bitwise_cmp=True,
-        benchcab_path="/absolute/path/to/benchcab",
-        pbs_config={
-            "ncpus": 4,
-            "mem": "16GB",
-            "walltime": "00:00:30",
-            "storage": ["gdata/foo"],
-        },
-    ) == (
-        """#!/bin/bash
+        )
+
+    def test_pbs_config_parameters(self):
+        """Success case: specify parameters in pbs_config."""
+        assert render_job_script(
+            project="tm70",
+            config_path="/path/to/config.yaml",
+            modules=["foo", "bar", "baz"],
+            skip_bitwise_cmp=True,
+            benchcab_path="/absolute/path/to/benchcab",
+            pbs_config={
+                "ncpus": 4,
+                "mem": "16GB",
+                "walltime": "00:00:30",
+                "storage": ["gdata/foo"],
+            },
+        ) == (
+            """#!/bin/bash
 #PBS -l wd
 #PBS -l ncpus=4
 #PBS -l mem=16GB
@@ -141,18 +142,19 @@ def test_render_job_script():
 /absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml 
 
 """
-    )
-
-    # Success case: if the pbs_config is empty, use the default values
-    assert render_job_script(
-        project="tm70",
-        config_path="/path/to/config.yaml",
-        modules=["foo", "bar", "baz"],
-        skip_bitwise_cmp=True,
-        benchcab_path="/absolute/path/to/benchcab",
-        pbs_config={},
-    ) == (
-        f"""#!/bin/bash
+        )
+
+    def test_default_pbs_config(self):
+        """Success case: if the pbs_config is empty, use the default values."""
+        assert render_job_script(
+            project="tm70",
+            config_path="/path/to/config.yaml",
+            modules=["foo", "bar", "baz"],
+            skip_bitwise_cmp=True,
+            benchcab_path="/absolute/path/to/benchcab",
+            pbs_config={},
+        ) == (
+            f"""#!/bin/bash
 #PBS -l wd
 #PBS -l ncpus={internal.FLUXSITE_DEFAULT_PBS["ncpus"]}
 #PBS -l mem={internal.FLUXSITE_DEFAULT_PBS["mem"]}
@@ -173,4 +175,4 @@ def test_render_job_script():
 /absolute/path/to/benchcab fluxsite-run-tasks --config=/path/to/config.yaml 
 
 """
-    )
+        )
diff --git a/tests/test_subprocess.py b/tests/test_subprocess.py
index d5dd25de..868d12d2 100644
--- a/tests/test_subprocess.py
+++ b/tests/test_subprocess.py
@@ -2,100 +2,123 @@
 
 import os
 import subprocess
+from pathlib import Path
 
 import pytest
 
-pytest.skip(allow_module_level=True)
-
 from benchcab.utils.subprocess import SubprocessWrapper
 
-from .common import TMP_DIR
-
 
-def test_run_cmd(capfd):
+class TestRunCmd:
     """Tests for `run_cmd()`."""
-    subprocess_handler = SubprocessWrapper()
-
-    # Success case: test stdout is suppressed in non-verbose mode
-    subprocess_handler.run_cmd("echo foo")
-    captured = capfd.readouterr()
-    assert not captured.out
-    assert not captured.err
-
-    # Success case: test stderr is suppressed in non-verbose mode
-    subprocess_handler.run_cmd("echo foo 1>&2")
-    captured = capfd.readouterr()
-    assert not captured.out
-    assert not captured.err
-
-    # Success case: test command and stdout is printed in verbose mode
-    subprocess_handler.run_cmd("echo foo", verbose=True)
-    captured = capfd.readouterr()
-    assert captured.out == "echo foo\nfoo\n"
-    assert not captured.err
-
-    # Success case: test command and stderr is redirected to stdout in verbose mode
-    subprocess_handler.run_cmd("echo foo 1>&2", verbose=True)
-    captured = capfd.readouterr()
-    assert captured.out == "echo foo 1>&2\nfoo\n"
-    assert not captured.err
-
-    # Success case: test output is captured with capture_output enabled
-    proc = subprocess_handler.run_cmd("echo foo", capture_output=True)
-    captured = capfd.readouterr()
-    assert not captured.out
-    assert not captured.err
-    assert proc.stdout == "foo\n"
-    assert not proc.stderr
-
-    # Success case: test stderr is captured and redirected to stdout with
-    # capture_output enabled
-    proc = subprocess_handler.run_cmd("echo foo 1>&2", capture_output=True)
-    captured = capfd.readouterr()
-    assert not captured.out
-    assert not captured.err
-    assert proc.stdout == "foo\n"
-    assert not proc.stderr
-
-    # Success case: test command is printed and stdout is captured in verbose mode
-    proc = subprocess_handler.run_cmd("echo foo", capture_output=True, verbose=True)
-    captured = capfd.readouterr()
-    assert captured.out == "echo foo\n"
-    assert not captured.err
-    assert proc.stdout == "foo\n"
-    assert not proc.stderr
-
-    # Success case: test stdout is redirected to file
-    file_path = TMP_DIR / "out.txt"
-    subprocess_handler.run_cmd("echo foo", output_file=file_path)
-    with file_path.open("r", encoding="utf-8") as file:
-        assert file.read() == "foo\n"
-    captured = capfd.readouterr()
-    assert not captured.out
-    assert not captured.err
-
-    # Success case: test command is printed and stdout is redirected to file in verbose mode
-    file_path = TMP_DIR / "out.txt"
-    subprocess_handler.run_cmd("echo foo", output_file=file_path, verbose=True)
-    with file_path.open("r", encoding="utf-8") as file:
-        assert file.read() == "foo\n"
-    captured = capfd.readouterr()
-    assert captured.out == "echo foo\n"
-    assert not captured.err
-
-    # Success case: test command is run with environment
-    proc = subprocess_handler.run_cmd(
-        "echo $FOO", capture_output=True, env={"FOO": "bar", **os.environ}
-    )
-    assert proc.stdout == "bar\n"
-
-    # Failure case: check non-zero return code throws an exception
-    with pytest.raises(subprocess.CalledProcessError):
-        subprocess_handler.run_cmd("exit 1")
-
-    # Failure case: check stderr is redirected to stdout on non-zero
-    # return code
-    with pytest.raises(subprocess.CalledProcessError) as exc:
-        subprocess_handler.run_cmd("echo foo 1>&2; exit 1", capture_output=True)
-    assert exc.value.stdout == "foo\n"
-    assert not exc.value.stderr
+
+    @pytest.fixture()
+    def subprocess_handler(self):
+        """Return an instance of `SubprocessWrapper` for testing."""
+        return SubprocessWrapper()
+
+    def test_stdout_is_suppressed_in_non_verbose_mode(self, subprocess_handler, capfd):
+        """Success case: test stdout is suppressed in non-verbose mode."""
+        subprocess_handler.run_cmd("echo foo")
+        captured = capfd.readouterr()
+        assert not captured.out
+        assert not captured.err
+
+    def test_stderr_is_suppressed_in_non_verbose_mode(self, subprocess_handler, capfd):
+        """Success case: test stderr is suppressed in non-verbose mode."""
+        subprocess_handler.run_cmd("echo foo 1>&2")
+        captured = capfd.readouterr()
+        assert not captured.out
+        assert not captured.err
+
+    def test_command_and_stdout_is_printed_in_verbose_mode(
+        self, subprocess_handler, capfd
+    ):
+        """Success case: test command and stdout is printed in verbose mode."""
+        subprocess_handler.run_cmd("echo foo", verbose=True)
+        captured = capfd.readouterr()
+        assert captured.out == "echo foo\nfoo\n"
+        assert not captured.err
+
+    def test_command_and_stderr_is_redirected_to_stdout_in_verbose_mode(
+        self, subprocess_handler, capfd
+    ):
+        """Success case: test command and stderr is redirected to stdout in verbose mode."""
+        subprocess_handler.run_cmd("echo foo 1>&2", verbose=True)
+        captured = capfd.readouterr()
+        assert captured.out == "echo foo 1>&2\nfoo\n"
+        assert not captured.err
+
+    def test_output_is_captured_with_capture_output_enabled(
+        self, subprocess_handler, capfd
+    ):
+        """Success case: test output is captured with capture_output enabled."""
+        proc = subprocess_handler.run_cmd("echo foo", capture_output=True)
+        captured = capfd.readouterr()
+        assert not captured.out
+        assert not captured.err
+        assert proc.stdout == "foo\n"
+        assert not proc.stderr
+
+    def test_stderr_captured_to_stdout(self, subprocess_handler, capfd):
+        """Success case: test stderr is captured to stdout with capture_output enabled."""
+        proc = subprocess_handler.run_cmd("echo foo 1>&2", capture_output=True)
+        captured = capfd.readouterr()
+        assert not captured.out
+        assert not captured.err
+        assert proc.stdout == "foo\n"
+        assert not proc.stderr
+
+    def test_command_is_printed_and_stdout_is_captured_in_verbose_mode(
+        self, subprocess_handler, capfd
+    ):
+        """Success case: test command is printed and stdout is captured in verbose mode."""
+        proc = subprocess_handler.run_cmd("echo foo", capture_output=True, verbose=True)
+        captured = capfd.readouterr()
+        assert captured.out == "echo foo\n"
+        assert not captured.err
+        assert proc.stdout == "foo\n"
+        assert not proc.stderr
+
+    def test_stdout_is_redirected_to_file(self, subprocess_handler, capfd):
+        """Success case: test stdout is redirected to file."""
+        file_path = Path("out.txt")
+        subprocess_handler.run_cmd("echo foo", output_file=file_path)
+        with file_path.open("r", encoding="utf-8") as file:
+            assert file.read() == "foo\n"
+        captured = capfd.readouterr()
+        assert not captured.out
+        assert not captured.err
+
+    def test_command_is_printed_and_stdout_is_redirected_to_file_in_verbose_mode(
+        self, subprocess_handler, capfd
+    ):
+        """Success case: test command is printed and stdout is redirected to file in verbose mode."""
+        file_path = Path("out.txt")
+        subprocess_handler.run_cmd("echo foo", output_file=file_path, verbose=True)
+        with file_path.open("r", encoding="utf-8") as file:
+            assert file.read() == "foo\n"
+        captured = capfd.readouterr()
+        assert captured.out == "echo foo\n"
+        assert not captured.err
+
+    def test_command_is_run_with_environment(self, subprocess_handler):
+        """Success case: test command is run with environment."""
+        proc = subprocess_handler.run_cmd(
+            "echo $FOO", capture_output=True, env={"FOO": "bar", **os.environ}
+        )
+        assert proc.stdout == "bar\n"
+
+    def test_check_non_zero_return_code_throws_an_exception(self, subprocess_handler):
+        """Failure case: check non-zero return code throws an exception."""
+        with pytest.raises(subprocess.CalledProcessError):
+            subprocess_handler.run_cmd("exit 1")
+
+    def test_stderr_is_redirected_to_stdout_on_non_zero_return_code(
+        self, subprocess_handler
+    ):
+        """Failure case: check stderr is redirected to stdout on non-zero return code."""
+        with pytest.raises(subprocess.CalledProcessError) as exc:
+            subprocess_handler.run_cmd("echo foo 1>&2; exit 1", capture_output=True)
+        assert exc.value.stdout == "foo\n"
+        assert not exc.value.stderr
diff --git a/tests/test_workdir.py b/tests/test_workdir.py
index 4fb8b981..62dd710c 100644
--- a/tests/test_workdir.py
+++ b/tests/test_workdir.py
@@ -9,43 +9,40 @@
 
 import pytest
 
-pytest.skip(allow_module_level=True)
-
-from benchcab.fluxsite import Task
-from benchcab.repository import CableRepository
 from benchcab.workdir import (
     clean_directory_tree,
     setup_fluxsite_directory_tree,
 )
 
 
-def setup_mock_fluxsite_directory_list():
-    """Return the list of work directories we want benchcab to create."""
-    fluxsite_directory_list = [
-        Path("runs", "fluxsite"),
-        Path("runs", "fluxsite", "logs"),
-        Path("runs", "fluxsite", "outputs"),
-        Path("runs", "fluxsite", "tasks"),
-        Path("runs", "fluxsite", "analysis"),
-        Path("runs", "fluxsite", "analysis", "bitwise-comparisons"),
-    ]
-
-    return fluxsite_directory_list
-
-
-def test_setup_directory_tree():
+class TestSetupFluxsiteDirectoryTree:
     """Tests for `setup_fluxsite_directory_tree()`."""
-    # Success case: generate the full fluxsite directory structure
-    setup_fluxsite_directory_tree()
 
-    for path in setup_mock_fluxsite_directory_list():
-        assert path.exists()
-
-
-@pytest.mark.parametrize("test_path", [Path("runs"), Path("src")])
-def test_clean_directory_tree(test_path):
+    @pytest.fixture(autouse=True)
+    def fluxsite_directory_list(self):
+        """Return the list of work directories we want benchcab to create."""
+        return [
+            Path("runs", "fluxsite"),
+            Path("runs", "fluxsite", "logs"),
+            Path("runs", "fluxsite", "outputs"),
+            Path("runs", "fluxsite", "tasks"),
+            Path("runs", "fluxsite", "analysis"),
+            Path("runs", "fluxsite", "analysis", "bitwise-comparisons"),
+        ]
+
+    def test_directory_structure_generated(self, fluxsite_directory_list):
+        """Success case: generate the full fluxsite directory structure."""
+        setup_fluxsite_directory_tree()
+        for path in fluxsite_directory_list:
+            assert path.exists()
+
+
+class TestCleanDirectoryTree:
     """Tests for `clean_directory_tree()`."""
-    # Success case: directory tree does not exist after clean
-    test_path.mkdir()
-    clean_directory_tree()
-    assert not test_path.exists()
+
+    @pytest.mark.parametrize("test_path", [Path("runs"), Path("src")])
+    def test_clean_directory_tree(self, test_path):
+        """Success case: directory tree does not exist after clean."""
+        test_path.mkdir()
+        clean_directory_tree()
+        assert not test_path.exists()