Reviewer's comments

Author: al-rigazzi

tests/test_output_files.py

@@ -106,12 +106,12 @@ def test_mutated_model_output(test_dir):
 def test_get_output_files_with_create_job_step(test_dir):
     """Testing output files through _create_job_step"""
     exp_dir = pathlib.Path(test_dir)
-    model.path = test_dir
+    local_model = Model("test_model", params={}, path=test_dir, run_settings=rs)
     # Create metadata_dir to simulate consistent metadata structure
     metadata_dir = exp_dir / CONFIG.metadata_subdir
-    step = controller._create_job_step(model, metadata_dir)
-    expected_out_path = metadata_dir / (model.name + ".out")
-    expected_err_path = metadata_dir / (model.name + ".err")
+    step = controller._create_job_step(local_model, metadata_dir)
+    expected_out_path = metadata_dir / (local_model.name + ".out")
+    expected_err_path = metadata_dir / (local_model.name + ".err")
     assert step.get_output_files() == (str(expected_out_path), str(expected_err_path))
 
 

tests/test_symlinking.py

@@ -48,21 +48,44 @@
 bs = SbatchSettings()
 batch_rs = SrunSettings("echo", ["spam", "eggs"])
 
-ens = Ensemble("ens", params={}, run_settings=rs, batch_settings=bs, replicas=3)
-orc = Orchestrator(db_nodes=3, batch=True, launcher="slurm", run_command="srun")
-model = Model("test_model", params={}, path="", run_settings=rs)
-batch_model = Model(
-    "batch_test_model", params={}, path="", run_settings=batch_rs, batch_settings=bs
-)
-anon_batch_model = _AnonymousBatchJob(batch_model)
+
+@pytest.fixture
+def model_entity():
+    return Model("test_model", params={}, path="", run_settings=rs)
+
+
+@pytest.fixture
+def ensemble_entity():
+    return Ensemble("ens", params={}, run_settings=rs, batch_settings=bs, replicas=3)
+
+
+@pytest.fixture
+def orchestrator_entity():
+    return Orchestrator(
+        db_nodes=3, batch=True, launcher="slurm", run_command="srun"
+    )
+
+
+@pytest.fixture
+def anon_batch_model_entity():
+    batch_model = Model(
+        "batch_test_model",
+        params={},
+        path="",
+        run_settings=batch_rs,
+        batch_settings=bs,
+    )
+    return _AnonymousBatchJob(batch_model)
 
 
 @pytest.mark.parametrize(
-    "entity",
-    [pytest.param(ens, id="ensemble"), pytest.param(model, id="model")],
+    "entity_fixture",
+    ["ensemble_entity", "model_entity"],
+    ids=["ensemble", "model"],
 )
-def test_symlink(test_dir, entity):
+def test_symlink(test_dir, request, entity_fixture):
     """Test symlinking historical output files"""
+    entity = request.getfixturevalue(entity_fixture)
     entity.path = test_dir
     if entity.type == Ensemble:
         for member in entity.models:
@@ -93,15 +116,17 @@ def symlink_with_create_job_step(test_dir, entity):
 
 
 @pytest.mark.parametrize(
-    "entity",
+    "entity_fixture",
     [
-        pytest.param(ens, id="ensemble"),
-        pytest.param(orc, id="orchestrator"),
-        pytest.param(anon_batch_model, id="model"),
+        "ensemble_entity",
+        "orchestrator_entity",
+        "anon_batch_model_entity",
     ],
+    ids=["ensemble", "orchestrator", "model"],
 )
-def test_batch_symlink(entity, test_dir):
+def test_batch_symlink(request, entity_fixture, test_dir):
     """Test symlinking historical output files"""
+    entity = request.getfixturevalue(entity_fixture)
     exp_dir = pathlib.Path(test_dir)
     entity.path = test_dir
     # For entities with sub-entities (like Orchestrator), set their paths too