From 4ef97591bbe79fe1ae92996cd91898e823a6d317 Mon Sep 17 00:00:00 2001 From: Mathieu Dupont <108517594+mathieudpnt@users.noreply.github.com> Date: Wed, 17 Dec 2025 11:00:20 +0100 Subject: [PATCH 1/5] add job submition dependency option --- src/osekit/utils/job.py | 95 +++++++++++++- tests/test_job.py | 273 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 361 insertions(+), 7 deletions(-) diff --git a/src/osekit/utils/job.py b/src/osekit/utils/job.py index 9a22c68e..246caba6 100644 --- a/src/osekit/utils/job.py +++ b/src/osekit/utils/job.py @@ -4,6 +4,7 @@ jobs, with writting/submitting of PBS files. """ +from __future__ import annotations import subprocess from dataclasses import dataclass @@ -307,15 +308,35 @@ def write_pbs(self, path: Path) -> None: self.path = path self.progress() - def submit_pbs(self) -> None: - """Submit the PBS file of the job to a PBS queueing system.""" + def submit_pbs(self, depend_on: Job | list[Job] | str | list[str] | None = None) -> None: + """Submit the PBS file of the job to a PBS queueing system. + + Parameters + ---------- + depend_on: Job | list[Job] | str | None + Job dependency. Can be: + - A Job instance: will wait for that job to complete successfully + - A list of Job instances: will wait for all jobs to complete successfully + - A string: job ID (e.g., "12345.datarmor") or dependency specification + - None: no dependency + + """ if self.update_status() is not JobStatus.PREPARED: msg = "Job should be written before being submitted." raise ValueError(msg) + cmd = ["qsub"] + + if depend_on is not None: + dependency_str = self._build_dependency_string(depend_on) + if dependency_str: + cmd.extend(["-W", f"depend={dependency_str}"]) + + cmd.append(str(self.path)) + try: request = subprocess.run( - ["qsub", self.path], + cmd, capture_output=True, text=True, check=False, @@ -327,6 +348,52 @@ def submit_pbs(self) -> None: self.job_id = request.stdout.split(".", maxsplit=1)[0].strip() self.progress() + @staticmethod + def _build_dependency_string( + depend_on: Job | list[Job] | str | list[str], dependency_type: str = "afterok" + ) -> str: + """Build a PBS dependency string. + + Parameters + ---------- + depend_on: Job | list[Job] | str | list[str] + Job(s) or job ID(s) to depend on. + dependency_type: str + Type of dependency (afterok, afterany, afternotok, after). + + Returns + ------- + str: + PBS dependency string (e.g., "afterok:12345:12346"). + + """ + job_ids = [] + + if isinstance(depend_on, str): + if ":" in depend_on: + return depend_on + else: + job_ids.append(depend_on) + elif isinstance(depend_on, Job): + if depend_on.job_id is None: + msg = f"Job '{depend_on.name}' has not been submitted yet." + raise ValueError(msg) + job_ids.append(depend_on.job_id) + elif isinstance(depend_on, list): + for item in depend_on: + if isinstance(item, str): + job_ids.append(item) + elif isinstance(item, Job): + if item.job_id is None: + msg = f"Job '{item.name}' has not been submitted yet." + raise ValueError(msg) + job_ids.append(item.job_id) + + if not job_ids: + return "" + + return f"{dependency_type}:{':'.join(job_ids)}" + def update_info(self) -> None: """Request info about the job and update it.""" if self.job_id is None: @@ -443,9 +510,25 @@ def create_job( job.write_pbs(output_folder / f"{name}.pbs") self.jobs.append(job) - def submit_pbs(self) -> None: - """Submit all repared jobs to the PBS queueing system.""" + def submit_pbs( + self, dependencies: dict[str, "Job | list[Job]"] | None = None + ) -> None: + """Submit all prepared jobs to the PBS queueing system. + + Parameters + ---------- + dependencies: dict[str, Job | list[Job]] | None + Optional dictionary mapping job names to their dependencies. + Example: {"job2": job1, "job3": [job1, job2]} + + """ for job in self.jobs: if job.update_status() is not JobStatus.PREPARED: continue - job.submit_pbs() + + # Check if this job has dependencies + depend_on = None + if dependencies and job.name in dependencies: + depend_on = dependencies[job.name] + + job.submit_pbs(depend_on=depend_on) diff --git a/tests/test_job.py b/tests/test_job.py index f98305b3..ba857e4e 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -414,7 +414,7 @@ def __init__(self, name: str, status: JobStatus) -> None: self.name = name self.status = status - def submit_pbs(self) -> None: + def submit_pbs(self, depend_on=None) -> None: # Add depend_on parameter submitted_jobs.append(self.name) def update_status(self) -> JobStatus: @@ -436,3 +436,274 @@ def update_status(self) -> JobStatus: job_builder.submit_pbs() assert submitted_jobs == ["prepared"] + + +@pytest.mark.parametrize( + ("depend_on", "expected"), + [ + pytest.param( + "4815162342", + "afterok:4815162342", + id="basic_job_id", + ), + pytest.param( + "4815162342.datarmor", + "afterok:4815162342.datarmor", + id="job_id_with_server", + ), + pytest.param( + "afterok:12345", + "afterok:12345", + id="already_formatted_id", + ), + pytest.param( + ["123", "456", "789"], + "afterok:123:456:789", + id="already_formatted_id", + ), + ], +) +def test_build_dependency_string_with_string_input(depend_on: str, expected: str) -> None: + """Test building dependency string from string inputs.""" + dep_str = Job._build_dependency_string(depend_on) + assert dep_str == expected + + +def test_build_dependency_string_with_single_job() -> None: + """Test building dependency string from a single Job instance.""" + dummy_job = Job(Path("script.py")) + dummy_job.job_id = "12345" + + dep_str = Job._build_dependency_string(dummy_job) + assert dep_str == "afterok:12345" + + +def test_build_dependency_string_with_job_list() -> None: + """Test building dependency string from multiple Job instances.""" + job1 = Job(Path("script1.py")) + job1.job_id = "12345" + + job2 = Job(Path("script2.py")) + job2.job_id = "67890" + + job3 = Job(Path("script3.py")) + job3.job_id = "11111" + + dep_str = Job._build_dependency_string([job1, job2, job3]) + assert dep_str == "afterok:12345:67890:11111" + + +def test_build_dependency_string_with_unsubmitted_job_raises() -> None: + """Test that using an unsubmitted job raises ValueError.""" + job = Job(Path("script.py")) + job.job_id = None + job.name = "my_way" + + with pytest.raises( + ValueError, match="Job 'my_way' has not been submitted yet" + ): + Job._build_dependency_string(job) + + +def test_build_dependency_string_with_unsubmitted_job_in_list_raises() -> None: + """Test that using an unsubmitted job in a list raises ValueError.""" + job1 = Job(Path("script1.py"), name="franck") + job1.job_id = "12345" + + job2 = Job(Path("script2.py"), name="sinatra") + job2.job_id = None + + with pytest.raises(ValueError, match="Job 'sinatra' has not been submitted yet"): + Job._build_dependency_string([job1, job2]) + + +def test_build_dependency_string_with_different_dependency_types() -> None: + """Test building dependency strings with different dependency types.""" + job = Job(Path("script.py")) + job.job_id = "12345" + + assert Job._build_dependency_string(job, "afterok") == "afterok:12345" + assert Job._build_dependency_string(job, "afterany") == "afterany:12345" + assert Job._build_dependency_string(job, "afternotok") == "afternotok:12345" + assert Job._build_dependency_string(job, "after") == "after:12345" + + +def test_submit_pbs_with_string_dependency( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test submitting a job with a string job ID dependency.""" + script = tmp_path / "script.py" + script.write_text("") + outdir = tmp_path + job = Job(script, name="dependent_job", output_folder=outdir) + pbs_path = tmp_path / "dependent_job.pbs" + job.write_pbs(pbs_path) + + captured_cmd = [] + + class Dummy: + def __init__(self) -> None: + self.stdout = "99999.server\n" + self.stderr = "" + + def mock_run(cmd, *args, **kwargs): + captured_cmd.extend(cmd) + return Dummy() + + monkeypatch.setattr(subprocess, "run", mock_run) + + job.submit_pbs(depend_on="12345.datarmor") + + assert "-W" in captured_cmd + w_index = captured_cmd.index("-W") + assert captured_cmd[w_index + 1] == "depend=afterok:12345.datarmor" + assert job.job_id == "99999" + + +def test_submit_pbs_with_job_dependency( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test submitting a job that depends on another Job instance.""" + script = tmp_path / "script.py" + script.write_text("") + outdir = tmp_path + + job1 = Job(script, name="job1", output_folder=outdir) + job1.job_id = "12345" + + job2 = Job(script, name="job2", output_folder=outdir) + pbs_path = tmp_path / "job2.pbs" + job2.write_pbs(pbs_path) + + captured_cmd = [] + + class Dummy: + def __init__(self) -> None: + self.stdout = "67890.server\n" + self.stderr = "" + + def mock_run(cmd, *args, **kwargs): + captured_cmd.extend(cmd) + return Dummy() + + monkeypatch.setattr(subprocess, "run", mock_run) + + job2.submit_pbs(depend_on=job1) + + assert "-W" in captured_cmd + w_index = captured_cmd.index("-W") + assert captured_cmd[w_index + 1] == "depend=afterok:12345" + assert job2.job_id == "67890" + + +def test_submit_pbs_with_multiple_job_dependencies( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test submitting a job that depends on multiple Job instances.""" + script = tmp_path / "script.py" + script.write_text("") + outdir = tmp_path + + job1 = Job(script, name="my", output_folder=outdir) + job1.job_id = "11111" + + job2 = Job(script, name="kind", output_folder=outdir) + job2.job_id = "22222" + + job3 = Job(script, name="of_town", output_folder=outdir) + job3.job_id = "33333" + + job4 = Job(script, name="chicago", output_folder=outdir) + pbs_path = tmp_path / "chicago.pbs" + job4.write_pbs(pbs_path) + + captured_cmd = [] + + class Dummy: + def __init__(self) -> None: + self.stdout = "44444.server\n" + self.stderr = "" + + def mock_run(cmd, *args, **kwargs): + captured_cmd.extend(cmd) + return Dummy() + + monkeypatch.setattr(subprocess, "run", mock_run) + + job4.submit_pbs(depend_on=[job1, job2, job3]) + + assert "-W" in captured_cmd + w_index = captured_cmd.index("-W") + assert captured_cmd[w_index + 1] == "depend=afterok:11111:22222:33333" + assert job4.job_id == "44444" + + +def test_submit_pbs_with_no_dependency( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Test that submitting without dependency works as before.""" + script = tmp_path / "script.py" + script.write_text("") + outdir = tmp_path + job = Job(script, name="independent_job", output_folder=outdir) + pbs_path = tmp_path / "independent_job.pbs" + job.write_pbs(pbs_path) + + captured_cmd = [] + + class Dummy: + def __init__(self) -> None: + self.stdout = "55555.server\n" + self.stderr = "" + + def mock_run(cmd, *args, **kwargs): + captured_cmd.extend(cmd) + return Dummy() + + monkeypatch.setattr(subprocess, "run", mock_run) + + job.submit_pbs() + + # Should not have -W flag + assert "-W" not in captured_cmd + assert job.job_id == "55555" + + +def test_job_builder_submit_with_dependencies(monkeypatch: pytest.MonkeyPatch) -> None: + """Test JobBuilder.submit_pbs with dependencies.""" + submitted_jobs = [] + + class DummyJob: + def __init__(self, name: str, status: JobStatus) -> None: + self.name = name + self.status = status + self.job_id = f"{name}_id" + self.dependency = None + + def submit_pbs(self, depend_on=None) -> None: + self.dependency = depend_on + submitted_jobs.append({"name": self.name, "depends_on": depend_on}) + + def update_status(self) -> JobStatus: + return self.status + + monkeypatch.setattr(job_module, "Job", DummyJob) + + job1 = DummyJob(name="job1", status=JobStatus.PREPARED) + job2 = DummyJob(name="job2", status=JobStatus.PREPARED) + job3 = DummyJob(name="job3", status=JobStatus.PREPARED) + + job_builder = JobBuilder() + job_builder.jobs = [job1, job2, job3] + + dependencies = { + "job2": job1, + "job3": [job1, job2] + } + + job_builder.submit_pbs(dependencies=dependencies) + + assert len(submitted_jobs) == 3 + assert submitted_jobs[0] == {"name": "job1", "depends_on": None} + assert submitted_jobs[1] == {"name": "job2", "depends_on": job1} + assert submitted_jobs[2] == {"name": "job3", "depends_on": [job1, job2]} \ No newline at end of file From af76a3b01afa9127a69c018fb0154c165a33531d Mon Sep 17 00:00:00 2001 From: Mathieu Dupont <108517594+mathieudpnt@users.noreply.github.com> Date: Wed, 17 Dec 2025 17:05:21 +0100 Subject: [PATCH 2/5] lighter build dep str func and tests refacto --- src/osekit/utils/job.py | 89 ++++++++++---- tests/test_job.py | 260 +++++++++++++--------------------------- 2 files changed, 144 insertions(+), 205 deletions(-) diff --git a/src/osekit/utils/job.py b/src/osekit/utils/job.py index 246caba6..5f575425 100644 --- a/src/osekit/utils/job.py +++ b/src/osekit/utils/job.py @@ -348,9 +348,41 @@ def submit_pbs(self, depend_on: Job | list[Job] | str | list[str] | None = None) self.job_id = request.stdout.split(".", maxsplit=1)[0].strip() self.progress() + @staticmethod + def _normalize_dependencies( + depend_on: Job | list[Job] | str | list[str], + dependency_type: str, + ) -> list[Job | str]: + """Normalize dependency input to a list and strip prefixes from strings. + + Parameters + ---------- + depend_on: Job | list[Job] | str | list[str] + Raw dependency input. + dependency_type: str + Type of dependency (used to strip prefixes like "afterok:"). + + Returns + ------- + list[Job | str] + Normalized list of dependencies. + + """ + # Ensure we have a list + depend_on_list = depend_on if isinstance(depend_on, list) else [depend_on] + + # Strip dependency type prefix from strings (e.g., "afterok:12345" -> "12345") + normalized = [ + dep.lstrip(f"{dependency_type}:") if isinstance(dep, str) else dep + for dep in depend_on_list + ] + + return normalized + @staticmethod def _build_dependency_string( - depend_on: Job | list[Job] | str | list[str], dependency_type: str = "afterok" + dependency: Job | list[Job] | str | list[str], + dependency_type: str = "afterok", ) -> str: """Build a PBS dependency string. @@ -363,34 +395,39 @@ def _build_dependency_string( Returns ------- - str: - PBS dependency string (e.g., "afterok:12345:12346"). + str + PBS dependency string. + + Examples + -------- + >>> Job._build_dependency_string("12345") + 'afterok:12345' + >>> Job._build_dependency_string(["12345", "67890"]) + 'afterok:12345:67890' + >>> Job._build_dependency_string("12345", dependency_type="afterany") + 'afterany:12345' """ - job_ids = [] + # Normalize input + depend_on_list = Job._normalize_dependencies(dependency, dependency_type) + + # Check for unsubmitted jobs + if unsubmitted_job := next( + ( + j + for j in depend_on_list + if isinstance(j, Job) and j.status.value < JobStatus.QUEUED.value + ), + None, + ): + msg = f"Job '{unsubmitted_job.name}' has not been submitted yet." + raise ValueError(msg) - if isinstance(depend_on, str): - if ":" in depend_on: - return depend_on - else: - job_ids.append(depend_on) - elif isinstance(depend_on, Job): - if depend_on.job_id is None: - msg = f"Job '{depend_on.name}' has not been submitted yet." - raise ValueError(msg) - job_ids.append(depend_on.job_id) - elif isinstance(depend_on, list): - for item in depend_on: - if isinstance(item, str): - job_ids.append(item) - elif isinstance(item, Job): - if item.job_id is None: - msg = f"Job '{item.name}' has not been submitted yet." - raise ValueError(msg) - job_ids.append(item.job_id) - - if not job_ids: - return "" + # Build dependency string + job_ids = [ + dep.job_id if isinstance(dep, Job) else dep + for dep in depend_on_list + ] return f"{dependency_type}:{':'.join(job_ids)}" diff --git a/tests/test_job.py b/tests/test_job.py index ba857e4e..8cdcff2f 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import subprocess from pathlib import Path @@ -439,12 +441,12 @@ def update_status(self) -> JobStatus: @pytest.mark.parametrize( - ("depend_on", "expected"), + ("dependency", "expected"), [ pytest.param( "4815162342", "afterok:4815162342", - id="basic_job_id", + id="single_job_id", ), pytest.param( "4815162342.datarmor", @@ -452,99 +454,112 @@ def update_status(self) -> JobStatus: id="job_id_with_server", ), pytest.param( - "afterok:12345", - "afterok:12345", - id="already_formatted_id", - ), - pytest.param( - ["123", "456", "789"], - "afterok:123:456:789", - id="already_formatted_id", + ["11111", "22222", "33333"], + "afterok:11111:22222:33333", + id="multiple_job_ids", ), ], ) -def test_build_dependency_string_with_string_input(depend_on: str, expected: str) -> None: +def test_build_dependency_string_with_string_input( + dependency: str | list[str], expected: str +) -> None: """Test building dependency string from string inputs.""" - dep_str = Job._build_dependency_string(depend_on) + dep_str = Job._build_dependency_string(dependency) assert dep_str == expected def test_build_dependency_string_with_single_job() -> None: """Test building dependency string from a single Job instance.""" - dummy_job = Job(Path("script.py")) - dummy_job.job_id = "12345" + job = Job(Path("script.py")) + job.job_id = "12345" + job.status = JobStatus.QUEUED - dep_str = Job._build_dependency_string(dummy_job) + dep_str = Job._build_dependency_string(job) assert dep_str == "afterok:12345" -def test_build_dependency_string_with_job_list() -> None: +def test_build_dependency_string_with_multiple_jobs() -> None: """Test building dependency string from multiple Job instances.""" - job1 = Job(Path("script1.py")) - job1.job_id = "12345" + jobs = [] + for i, job_id in enumerate(["12345", "67890", "11111"], 1): + job = Job(Path(f"script{i}.py")) + job.job_id = job_id + job.status = JobStatus.QUEUED + jobs.append(job) + + dep_str = Job._build_dependency_string(jobs) + assert dep_str == "afterok:12345:67890:11111" - job2 = Job(Path("script2.py")) - job2.job_id = "67890" - job3 = Job(Path("script3.py")) - job3.job_id = "11111" +@pytest.mark.parametrize( + "dependency_type", + ["afterok", "afterany", "afternotok", "after"], +) +def test_build_dependency_string_with_different_types(dependency_type: str) -> None: + """Test building dependency strings with different dependency types.""" + job = Job(Path("script.py")) + job.job_id = "12345" + job.status = JobStatus.QUEUED - dep_str = Job._build_dependency_string([job1, job2, job3]) - assert dep_str == "afterok:12345:67890:11111" + dep_str = Job._build_dependency_string(job, dependency_type) + assert dep_str == f"{dependency_type}:12345" def test_build_dependency_string_with_unsubmitted_job_raises() -> None: """Test that using an unsubmitted job raises ValueError.""" - job = Job(Path("script.py")) - job.job_id = None - job.name = "my_way" + job = Job(Path("script.py"), name="my_way") + job.status = JobStatus.PREPARED - with pytest.raises( - ValueError, match="Job 'my_way' has not been submitted yet" - ): + with pytest.raises(ValueError, match="Job 'my_way' has not been submitted yet"): Job._build_dependency_string(job) def test_build_dependency_string_with_unsubmitted_job_in_list_raises() -> None: """Test that using an unsubmitted job in a list raises ValueError.""" - job1 = Job(Path("script1.py"), name="franck") + job1 = Job(Path("script1.py"), name="job1") job1.job_id = "12345" + job1.status = JobStatus.QUEUED - job2 = Job(Path("script2.py"), name="sinatra") - job2.job_id = None + job2 = Job(Path("script2.py"), name="job2") + job2.status = JobStatus.PREPARED # Not yet submitted - with pytest.raises(ValueError, match="Job 'sinatra' has not been submitted yet"): + with pytest.raises(ValueError, match="Job 'job2' has not been submitted yet"): Job._build_dependency_string([job1, job2]) -def test_build_dependency_string_with_different_dependency_types() -> None: - """Test building dependency strings with different dependency types.""" - job = Job(Path("script.py")) - job.job_id = "12345" - - assert Job._build_dependency_string(job, "afterok") == "afterok:12345" - assert Job._build_dependency_string(job, "afterany") == "afterany:12345" - assert Job._build_dependency_string(job, "afternotok") == "afternotok:12345" - assert Job._build_dependency_string(job, "after") == "after:12345" - - -def test_submit_pbs_with_string_dependency( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch +@pytest.mark.parametrize( + ("dependency", "expected_depend_flag"), + [ + pytest.param( + "12345.datarmor", + "depend=afterok:12345.datarmor", + id="string_dependency", + ), + pytest.param( + ["11111", "22222", "33333"], + "depend=afterok:11111:22222:33333", + id="multiple_string_dependencies", + ), + ], +) +def test_submit_pbs_with_dependencies( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + dependency: str | list[str], + expected_depend_flag: str, ) -> None: - """Test submitting a job with a string job ID dependency.""" + """Test submitting jobs with various dependency configurations.""" script = tmp_path / "script.py" script.write_text("") - outdir = tmp_path - job = Job(script, name="dependent_job", output_folder=outdir) + job = Job(script, name="dependent_job", output_folder=tmp_path) pbs_path = tmp_path / "dependent_job.pbs" job.write_pbs(pbs_path) captured_cmd = [] class Dummy: - def __init__(self) -> None: - self.stdout = "99999.server\n" - self.stderr = "" + stdout = "99999.server\n" + stderr = "" def mock_run(cmd, *args, **kwargs): captured_cmd.extend(cmd) @@ -552,35 +567,35 @@ def mock_run(cmd, *args, **kwargs): monkeypatch.setattr(subprocess, "run", mock_run) - job.submit_pbs(depend_on="12345.datarmor") + job.submit_pbs(depend_on=dependency) assert "-W" in captured_cmd w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == "depend=afterok:12345.datarmor" - assert job.job_id == "99999" + assert captured_cmd[w_index + 1] == expected_depend_flag -def test_submit_pbs_with_job_dependency( +def test_submit_pbs_with_job_instance_dependency( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Test submitting a job that depends on another Job instance.""" + """Test submitting a job that depends on Job instances.""" script = tmp_path / "script.py" script.write_text("") - outdir = tmp_path - job1 = Job(script, name="job1", output_folder=outdir) + # Create a dependency job + job1 = Job(script, name="job1", output_folder=tmp_path) job1.job_id = "12345" + job1.status = JobStatus.QUEUED - job2 = Job(script, name="job2", output_folder=outdir) + # Create a dependent job + job2 = Job(script, name="job2", output_folder=tmp_path) pbs_path = tmp_path / "job2.pbs" job2.write_pbs(pbs_path) captured_cmd = [] class Dummy: - def __init__(self) -> None: - self.stdout = "67890.server\n" - self.stderr = "" + stdout = "67890.server\n" + stderr = "" def mock_run(cmd, *args, **kwargs): captured_cmd.extend(cmd) @@ -588,122 +603,9 @@ def mock_run(cmd, *args, **kwargs): monkeypatch.setattr(subprocess, "run", mock_run) + # Test single Job dependency job2.submit_pbs(depend_on=job1) assert "-W" in captured_cmd w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == "depend=afterok:12345" - assert job2.job_id == "67890" - - -def test_submit_pbs_with_multiple_job_dependencies( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Test submitting a job that depends on multiple Job instances.""" - script = tmp_path / "script.py" - script.write_text("") - outdir = tmp_path - - job1 = Job(script, name="my", output_folder=outdir) - job1.job_id = "11111" - - job2 = Job(script, name="kind", output_folder=outdir) - job2.job_id = "22222" - - job3 = Job(script, name="of_town", output_folder=outdir) - job3.job_id = "33333" - - job4 = Job(script, name="chicago", output_folder=outdir) - pbs_path = tmp_path / "chicago.pbs" - job4.write_pbs(pbs_path) - - captured_cmd = [] - - class Dummy: - def __init__(self) -> None: - self.stdout = "44444.server\n" - self.stderr = "" - - def mock_run(cmd, *args, **kwargs): - captured_cmd.extend(cmd) - return Dummy() - - monkeypatch.setattr(subprocess, "run", mock_run) - - job4.submit_pbs(depend_on=[job1, job2, job3]) - - assert "-W" in captured_cmd - w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == "depend=afterok:11111:22222:33333" - assert job4.job_id == "44444" - - -def test_submit_pbs_with_no_dependency( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Test that submitting without dependency works as before.""" - script = tmp_path / "script.py" - script.write_text("") - outdir = tmp_path - job = Job(script, name="independent_job", output_folder=outdir) - pbs_path = tmp_path / "independent_job.pbs" - job.write_pbs(pbs_path) - - captured_cmd = [] - - class Dummy: - def __init__(self) -> None: - self.stdout = "55555.server\n" - self.stderr = "" - - def mock_run(cmd, *args, **kwargs): - captured_cmd.extend(cmd) - return Dummy() - - monkeypatch.setattr(subprocess, "run", mock_run) - - job.submit_pbs() - - # Should not have -W flag - assert "-W" not in captured_cmd - assert job.job_id == "55555" - - -def test_job_builder_submit_with_dependencies(monkeypatch: pytest.MonkeyPatch) -> None: - """Test JobBuilder.submit_pbs with dependencies.""" - submitted_jobs = [] - - class DummyJob: - def __init__(self, name: str, status: JobStatus) -> None: - self.name = name - self.status = status - self.job_id = f"{name}_id" - self.dependency = None - - def submit_pbs(self, depend_on=None) -> None: - self.dependency = depend_on - submitted_jobs.append({"name": self.name, "depends_on": depend_on}) - - def update_status(self) -> JobStatus: - return self.status - - monkeypatch.setattr(job_module, "Job", DummyJob) - - job1 = DummyJob(name="job1", status=JobStatus.PREPARED) - job2 = DummyJob(name="job2", status=JobStatus.PREPARED) - job3 = DummyJob(name="job3", status=JobStatus.PREPARED) - - job_builder = JobBuilder() - job_builder.jobs = [job1, job2, job3] - - dependencies = { - "job2": job1, - "job3": [job1, job2] - } - - job_builder.submit_pbs(dependencies=dependencies) - - assert len(submitted_jobs) == 3 - assert submitted_jobs[0] == {"name": "job1", "depends_on": None} - assert submitted_jobs[1] == {"name": "job2", "depends_on": job1} - assert submitted_jobs[2] == {"name": "job3", "depends_on": [job1, job2]} \ No newline at end of file + assert captured_cmd[w_index + 1] == "depend=afterok:12345" \ No newline at end of file From 966a37e4b8de426ea80f6cd478909d72993ac461 Mon Sep 17 00:00:00 2001 From: Mathieu Dupont <108517594+mathieudpnt@users.noreply.github.com> Date: Wed, 17 Dec 2025 17:17:33 +0100 Subject: [PATCH 3/5] lighter tests --- src/osekit/utils/job.py | 19 +++----- tests/test_job.py | 99 ++++++++++++++++------------------------- 2 files changed, 45 insertions(+), 73 deletions(-) diff --git a/src/osekit/utils/job.py b/src/osekit/utils/job.py index 5f575425..d50a187f 100644 --- a/src/osekit/utils/job.py +++ b/src/osekit/utils/job.py @@ -308,12 +308,12 @@ def write_pbs(self, path: Path) -> None: self.path = path self.progress() - def submit_pbs(self, depend_on: Job | list[Job] | str | list[str] | None = None) -> None: + def submit_pbs(self, dependency: Job | list[Job] | str | list[str] | None = None) -> None: """Submit the PBS file of the job to a PBS queueing system. Parameters ---------- - depend_on: Job | list[Job] | str | None + dependency: Job | list[Job] | str | None Job dependency. Can be: - A Job instance: will wait for that job to complete successfully - A list of Job instances: will wait for all jobs to complete successfully @@ -327,8 +327,8 @@ def submit_pbs(self, depend_on: Job | list[Job] | str | list[str] | None = None) cmd = ["qsub"] - if depend_on is not None: - dependency_str = self._build_dependency_string(depend_on) + if dependency is not None: + dependency_str = self._build_dependency_string(dependency) if dependency_str: cmd.extend(["-W", f"depend={dependency_str}"]) @@ -368,17 +368,13 @@ def _normalize_dependencies( Normalized list of dependencies. """ - # Ensure we have a list depend_on_list = depend_on if isinstance(depend_on, list) else [depend_on] - # Strip dependency type prefix from strings (e.g., "afterok:12345" -> "12345") - normalized = [ + return [ dep.lstrip(f"{dependency_type}:") if isinstance(dep, str) else dep for dep in depend_on_list ] - return normalized - @staticmethod def _build_dependency_string( dependency: Job | list[Job] | str | list[str], @@ -408,10 +404,8 @@ def _build_dependency_string( 'afterany:12345' """ - # Normalize input depend_on_list = Job._normalize_dependencies(dependency, dependency_type) - # Check for unsubmitted jobs if unsubmitted_job := next( ( j @@ -423,7 +417,6 @@ def _build_dependency_string( msg = f"Job '{unsubmitted_job.name}' has not been submitted yet." raise ValueError(msg) - # Build dependency string job_ids = [ dep.job_id if isinstance(dep, Job) else dep for dep in depend_on_list @@ -568,4 +561,4 @@ def submit_pbs( if dependencies and job.name in dependencies: depend_on = dependencies[job.name] - job.submit_pbs(depend_on=depend_on) + job.submit_pbs(dependency=depend_on) diff --git a/tests/test_job.py b/tests/test_job.py index 8cdcff2f..8ea79366 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -416,7 +416,7 @@ def __init__(self, name: str, status: JobStatus) -> None: self.name = name self.status = status - def submit_pbs(self, depend_on=None) -> None: # Add depend_on parameter + def submit_pbs(self, dependency=None) -> None: submitted_jobs.append(self.name) def update_status(self) -> JobStatus: @@ -468,27 +468,29 @@ def test_build_dependency_string_with_string_input( assert dep_str == expected -def test_build_dependency_string_with_single_job() -> None: - """Test building dependency string from a single Job instance.""" - job = Job(Path("script.py")) - job.job_id = "12345" - job.status = JobStatus.QUEUED - - dep_str = Job._build_dependency_string(job) - assert dep_str == "afterok:12345" - - -def test_build_dependency_string_with_multiple_jobs() -> None: - """Test building dependency string from multiple Job instances.""" +@pytest.mark.parametrize( + ("job_ids", "expected"), + [ + pytest.param(["12345"], "afterok:12345", id="single_job"), + pytest.param( + ["12345", "67890", "11111"], "afterok:12345:67890:11111", id="multiple_jobs" + ), + ], +) +def test_build_dependency_string_with_job_instances( + job_ids: list[str], expected: str +) -> None: + """Test building dependency string from Job instance(s).""" jobs = [] - for i, job_id in enumerate(["12345", "67890", "11111"], 1): + for i, job_id in enumerate(job_ids, 1): job = Job(Path(f"script{i}.py")) job.job_id = job_id job.status = JobStatus.QUEUED jobs.append(job) - dep_str = Job._build_dependency_string(jobs) - assert dep_str == "afterok:12345:67890:11111" + dependency = jobs[0] if len(jobs) == 1 else jobs + dep_str = Job._build_dependency_string(dependency) + assert dep_str == expected @pytest.mark.parametrize( @@ -528,29 +530,43 @@ def test_build_dependency_string_with_unsubmitted_job_in_list_raises() -> None: @pytest.mark.parametrize( - ("dependency", "expected_depend_flag"), + ("dependency_setup", "expected_depend_flag"), [ pytest.param( - "12345.datarmor", + lambda: "12345.datarmor", "depend=afterok:12345.datarmor", id="string_dependency", ), pytest.param( - ["11111", "22222", "33333"], + lambda: ["11111", "22222", "33333"], "depend=afterok:11111:22222:33333", id="multiple_string_dependencies", ), + pytest.param( + lambda: "job_instance", # Special marker + "depend=afterok:12345", + id="job_instance_dependency", + ), ], ) -def test_submit_pbs_with_dependencies( +def test_submit_pbs_with_various_dependencies( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, - dependency: str | list[str], + dependency_setup: callable, expected_depend_flag: str, ) -> None: """Test submitting jobs with various dependency configurations.""" script = tmp_path / "script.py" script.write_text("") + + # Setup dependency + dependency = dependency_setup() + if dependency == "job_instance": + job1 = Job(script, name="job1", output_folder=tmp_path) + job1.job_id = "12345" + job1.status = JobStatus.QUEUED + dependency = job1 + job = Job(script, name="dependent_job", output_folder=tmp_path) pbs_path = tmp_path / "dependent_job.pbs" job.write_pbs(pbs_path) @@ -567,45 +583,8 @@ def mock_run(cmd, *args, **kwargs): monkeypatch.setattr(subprocess, "run", mock_run) - job.submit_pbs(depend_on=dependency) - - assert "-W" in captured_cmd - w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == expected_depend_flag - - -def test_submit_pbs_with_job_instance_dependency( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Test submitting a job that depends on Job instances.""" - script = tmp_path / "script.py" - script.write_text("") - - # Create a dependency job - job1 = Job(script, name="job1", output_folder=tmp_path) - job1.job_id = "12345" - job1.status = JobStatus.QUEUED - - # Create a dependent job - job2 = Job(script, name="job2", output_folder=tmp_path) - pbs_path = tmp_path / "job2.pbs" - job2.write_pbs(pbs_path) - - captured_cmd = [] - - class Dummy: - stdout = "67890.server\n" - stderr = "" - - def mock_run(cmd, *args, **kwargs): - captured_cmd.extend(cmd) - return Dummy() - - monkeypatch.setattr(subprocess, "run", mock_run) - - # Test single Job dependency - job2.submit_pbs(depend_on=job1) + job.submit_pbs(dependency=dependency) assert "-W" in captured_cmd w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == "depend=afterok:12345" \ No newline at end of file + assert captured_cmd[w_index + 1] == expected_depend_flag \ No newline at end of file From 1cc0b1ce12755274ef82430cf3df63821966272d Mon Sep 17 00:00:00 2001 From: Mathieu Dupont <108517594+mathieudpnt@users.noreply.github.com> Date: Thu, 18 Dec 2025 12:17:02 +0100 Subject: [PATCH 4/5] better tests --- src/osekit/utils/job.py | 84 ++++++------- tests/test_job.py | 273 ++++++++++++++++++++++------------------ 2 files changed, 189 insertions(+), 168 deletions(-) diff --git a/src/osekit/utils/job.py b/src/osekit/utils/job.py index d50a187f..940c099c 100644 --- a/src/osekit/utils/job.py +++ b/src/osekit/utils/job.py @@ -6,6 +6,7 @@ """ from __future__ import annotations +import re import subprocess from dataclasses import dataclass from enum import Enum @@ -348,44 +349,40 @@ def submit_pbs(self, dependency: Job | list[Job] | str | list[str] | None = None self.job_id = request.stdout.split(".", maxsplit=1)[0].strip() self.progress() - @staticmethod - def _normalize_dependencies( - depend_on: Job | list[Job] | str | list[str], - dependency_type: str, - ) -> list[Job | str]: - """Normalize dependency input to a list and strip prefixes from strings. - - Parameters - ---------- - depend_on: Job | list[Job] | str | list[str] - Raw dependency input. - dependency_type: str - Type of dependency (used to strip prefixes like "afterok:"). + _VALID_DEPENDENCY_TYPES = {"afterok", "afterany", "afternotok", "after"} - Returns - ------- - list[Job | str] - Normalized list of dependencies. + @staticmethod + def _validate_dependency_type(dependency_type: str) -> None: + if dependency_type not in Job._VALID_DEPENDENCY_TYPES: + raise ValueError( + f"Unsupported dependency type '{dependency_type}'. " + f"Expected one of {sorted(Job._VALID_DEPENDENCY_TYPES)}." + ) - """ - depend_on_list = depend_on if isinstance(depend_on, list) else [depend_on] + _JOB_ID_PATTERN = re.compile(r"\d{7}") - return [ - dep.lstrip(f"{dependency_type}:") if isinstance(dep, str) else dep - for dep in depend_on_list - ] + @staticmethod + def _validate_dependency(dependency: str | Job | list[str] | list[Job]) -> list[str]: + deps = dependency if isinstance(dependency, list) else [dependency] + job_ids = [dep.job_id if isinstance(dep, Job) else dep for dep in deps] + for job_id in job_ids: + if not Job._JOB_ID_PATTERN.fullmatch(job_id): + raise ValueError( + f"Invalid job ID '{job_id}'. Job IDs must be 7 digits long." + ) + return job_ids @staticmethod def _build_dependency_string( - dependency: Job | list[Job] | str | list[str], + dependency: str | Job | list[str] | list[Job], dependency_type: str = "afterok", ) -> str: """Build a PBS dependency string. Parameters ---------- - depend_on: Job | list[Job] | str | list[str] - Job(s) or job ID(s) to depend on. + dependency: Job | str + Job or job ID to depend on. dependency_type: str Type of dependency (afterok, afterany, afternotok, after). @@ -396,33 +393,30 @@ def _build_dependency_string( Examples -------- - >>> Job._build_dependency_string("12345") - 'afterok:12345' - >>> Job._build_dependency_string(["12345", "67890"]) - 'afterok:12345:67890' - >>> Job._build_dependency_string("12345", dependency_type="afterany") - 'afterany:12345' + >>> Job._build_dependency_string("1234567") + 'afterok:1234567' + >>> Job._build_dependency_string(["1234567", "4567891"]) + 'afterok:1234567:4567891' + >>> Job._build_dependency_string("7894561", dependency_type="afterany") + 'afterany:7894651' """ - depend_on_list = Job._normalize_dependencies(dependency, dependency_type) + dependency = dependency if isinstance(dependency, list) else [dependency] + id_str = Job._validate_dependency(dependency) + Job._validate_dependency_type(dependency_type) if unsubmitted_job := next( - ( - j - for j in depend_on_list - if isinstance(j, Job) and j.status.value < JobStatus.QUEUED.value - ), - None, + ( + j + for j in dependency + if isinstance(j, Job) and j.status.value < JobStatus.QUEUED.value + ), + None, ): msg = f"Job '{unsubmitted_job.name}' has not been submitted yet." raise ValueError(msg) - job_ids = [ - dep.job_id if isinstance(dep, Job) else dep - for dep in depend_on_list - ] - - return f"{dependency_type}:{':'.join(job_ids)}" + return f"{dependency_type}:{':'.join(id_str)}" def update_info(self) -> None: """Request info about the job and update it.""" diff --git a/tests/test_job.py b/tests/test_job.py index 8ea79366..77081c84 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -1,6 +1,7 @@ from __future__ import annotations import subprocess +from contextlib import nullcontext from pathlib import Path import pytest @@ -441,150 +442,176 @@ def update_status(self) -> JobStatus: @pytest.mark.parametrize( - ("dependency", "expected"), + ("dependency", "job_id", "job_status", "expected", "expected_exception"), [ pytest.param( - "4815162342", - "afterok:4815162342", + "1234567", + None, + None, + "afterok:1234567", + nullcontext(), id="single_job_id", ), pytest.param( - "4815162342.datarmor", - "afterok:4815162342.datarmor", - id="job_id_with_server", + ["1234567", "4567891", "7891234"], + [None] * 3, + [None] * 3, + "afterok:1234567:4567891:7891234", + nullcontext(), + id="multiple_job_ids", ), pytest.param( - ["11111", "22222", "33333"], - "afterok:11111:22222:33333", - id="multiple_job_ids", + "123", + None, + None, + None, + pytest.raises( + ValueError, + match=r"Invalid job ID '123'\. Job IDs must be 7 digits long\.", + ), + id="invalid_job_id_too_short", ), - ], -) -def test_build_dependency_string_with_string_input( - dependency: str | list[str], expected: str -) -> None: - """Test building dependency string from string inputs.""" - dep_str = Job._build_dependency_string(dependency) - assert dep_str == expected - - -@pytest.mark.parametrize( - ("job_ids", "expected"), - [ - pytest.param(["12345"], "afterok:12345", id="single_job"), pytest.param( - ["12345", "67890", "11111"], "afterok:12345:67890:11111", id="multiple_jobs" + "abcdefg", + None, + None, + None, + pytest.raises( + ValueError, + match=r"Invalid job ID 'abcdefg'\. Job IDs must be 7 digits long\.", + ), + id="invalid_job_id_non_numeric", + ), + pytest.param( + ["1234567", "not_a_job_id"], + [None] * 2, + [None] * 2, + None, + pytest.raises( + ValueError, + match=r"Invalid job ID 'not_a_job_id'\. Job IDs must be 7 digits long\.", + ), + id="multiple_job_id_one_invalid", ), - ], -) -def test_build_dependency_string_with_job_instances( - job_ids: list[str], expected: str -) -> None: - """Test building dependency string from Job instance(s).""" - jobs = [] - for i, job_id in enumerate(job_ids, 1): - job = Job(Path(f"script{i}.py")) - job.job_id = job_id - job.status = JobStatus.QUEUED - jobs.append(job) - - dependency = jobs[0] if len(jobs) == 1 else jobs - dep_str = Job._build_dependency_string(dependency) - assert dep_str == expected - - -@pytest.mark.parametrize( - "dependency_type", - ["afterok", "afterany", "afternotok", "after"], -) -def test_build_dependency_string_with_different_types(dependency_type: str) -> None: - """Test building dependency strings with different dependency types.""" - job = Job(Path("script.py")) - job.job_id = "12345" - job.status = JobStatus.QUEUED - - dep_str = Job._build_dependency_string(job, dependency_type) - assert dep_str == f"{dependency_type}:12345" - - -def test_build_dependency_string_with_unsubmitted_job_raises() -> None: - """Test that using an unsubmitted job raises ValueError.""" - job = Job(Path("script.py"), name="my_way") - job.status = JobStatus.PREPARED - - with pytest.raises(ValueError, match="Job 'my_way' has not been submitted yet"): - Job._build_dependency_string(job) - - -def test_build_dependency_string_with_unsubmitted_job_in_list_raises() -> None: - """Test that using an unsubmitted job in a list raises ValueError.""" - job1 = Job(Path("script1.py"), name="job1") - job1.job_id = "12345" - job1.status = JobStatus.QUEUED - - job2 = Job(Path("script2.py"), name="job2") - job2.status = JobStatus.PREPARED # Not yet submitted - - with pytest.raises(ValueError, match="Job 'job2' has not been submitted yet"): - Job._build_dependency_string([job1, job2]) - - -@pytest.mark.parametrize( - ("dependency_setup", "expected_depend_flag"), - [ pytest.param( - lambda: "12345.datarmor", - "depend=afterok:12345.datarmor", - id="string_dependency", + Job(script_path=Path("test.py"), name="job_1"), + "1234567", + JobStatus.QUEUED, + "afterok:1234567", + nullcontext(), + id="single_job_instance", + ), + pytest.param( + [ + Job(script_path=Path("horse_with.py"), name="job_1"), + Job(script_path=Path("no_name.py"), name="job_2"), + ], + ["1234567", "4567891"], + [JobStatus.QUEUED, JobStatus.QUEUED], + "afterok:1234567:4567891", + nullcontext(), + id="multiple_job_instance", ), pytest.param( - lambda: ["11111", "22222", "33333"], - "depend=afterok:11111:22222:33333", - id="multiple_string_dependencies", + [ + Job(script_path=Path("king_crimson.py"), name="job_1"), + Job(script_path=Path("crimson_king.py"), name="job_2"), + ], + ["1234567", "not_an_id"], + [JobStatus.QUEUED, JobStatus.QUEUED], + None, + pytest.raises( + ValueError, + match=r"Invalid job ID 'not_an_id'\. Job IDs must be 7 digits long\.", + ), + id="multiple_job_instance_invalid_one", ), pytest.param( - lambda: "job_instance", # Special marker - "depend=afterok:12345", - id="job_instance_dependency", + [ + Job(script_path=Path("king_crimson.py"), name="job_1"), + "9876543", + ], + ["1234567", None], + [JobStatus.QUEUED, None], + "afterok:1234567:9876543", + nullcontext(), + id="job_and_string_input", + ), + pytest.param( + Job(script_path=Path("test.py"), name="tornero"), + "1234567", + JobStatus.UNPREPARED, + None, + pytest.raises( + ValueError, + match="Job 'tornero' has not been submitted yet.", + ), + id="unprepared_job_instance", + ), + pytest.param( + [ + Job(script_path=Path("script.py"), name="dalida"), + Job(script_path=Path("script.py"), name="mourir_sur_scene"), + ], + ["1234567", "4567896"], + [JobStatus.QUEUED, JobStatus.PREPARED], + None, + pytest.raises( + ValueError, + match="Job 'mourir_sur_scene' has not been submitted yet.", + ), + id="multiple_job_instance_one_not_submitted", ), ], ) -def test_submit_pbs_with_various_dependencies( - tmp_path: Path, - monkeypatch: pytest.MonkeyPatch, - dependency_setup: callable, - expected_depend_flag: str, -) -> None: - """Test submitting jobs with various dependency configurations.""" - script = tmp_path / "script.py" - script.write_text("") - # Setup dependency - dependency = dependency_setup() - if dependency == "job_instance": - job1 = Job(script, name="job1", output_folder=tmp_path) - job1.job_id = "12345" - job1.status = JobStatus.QUEUED - dependency = job1 - - job = Job(script, name="dependent_job", output_folder=tmp_path) - pbs_path = tmp_path / "dependent_job.pbs" - job.write_pbs(pbs_path) - - captured_cmd = [] +def test_build_dependency_string_with_string_input( + dependency: str | list[str] | Job | list[Job], + job_id: str | list[str] | None, + job_status: JobStatus, + expected: str | None, + expected_exception: type[Exception], +) -> None: + """Test building dependency string from string and Job inputs.""" + deps = dependency if isinstance(dependency, list) else [dependency] + ids = job_id if isinstance(job_id, list) else [job_id] + status = job_status if isinstance(job_id, list) else [job_status] - class Dummy: - stdout = "99999.server\n" - stderr = "" + for dep, job_id, job_status in zip(deps, ids, status, strict=True): + if isinstance(dep, Job): + dep.status = job_status + dep.job_id = job_id - def mock_run(cmd, *args, **kwargs): - captured_cmd.extend(cmd) - return Dummy() + with expected_exception: + dep_str = Job._build_dependency_string(dependency) + if expected is not None: + assert dep_str == expected - monkeypatch.setattr(subprocess, "run", mock_run) - job.submit_pbs(dependency=dependency) +@pytest.mark.parametrize( + ("dependency_type", "expected_exception"), + [ + pytest.param("afterok", nullcontext(), id="afterok"), + pytest.param("afterany", nullcontext(), id="afterany"), + pytest.param("afternotok", nullcontext(), id="afternotok"), + pytest.param("after", nullcontext(), id="after"), + pytest.param( + "not_a_supported_type", + pytest.raises( + ValueError, + match=r"Unsupported dependency type 'not_a_supported_type'\. Expected one of \['after', 'afterany', 'afternotok', 'afterok'\]\.", + ), + id="invalid_dependency_type", + ), + ], +) - assert "-W" in captured_cmd - w_index = captured_cmd.index("-W") - assert captured_cmd[w_index + 1] == expected_depend_flag \ No newline at end of file +# For now, only "afterok" is used in code +def test_build_dependency_string_with_different_types( + dependency_type: str, + expected_exception: type[Exception], +) -> None: + """Test building dependency strings with different dependency types.""" + with expected_exception: + dep_str = Job._build_dependency_string("1234567", dependency_type) + assert dep_str == f"{dependency_type}:1234567" \ No newline at end of file From 5ab803c2c170fd96c53e920f47b9ebba3d331af9 Mon Sep 17 00:00:00 2001 From: Mathieu Dupont <108517594+mathieudpnt@users.noreply.github.com> Date: Fri, 19 Dec 2025 09:34:31 +0100 Subject: [PATCH 5/5] test_refacto_yet_again --- src/osekit/utils/job.py | 10 ++-- tests/test_job.py | 104 ++++++++++++++++++---------------------- 2 files changed, 50 insertions(+), 64 deletions(-) diff --git a/src/osekit/utils/job.py b/src/osekit/utils/job.py index 940c099c..0d7962e9 100644 --- a/src/osekit/utils/job.py +++ b/src/osekit/utils/job.py @@ -6,7 +6,6 @@ """ from __future__ import annotations -import re import subprocess from dataclasses import dataclass from enum import Enum @@ -359,14 +358,11 @@ def _validate_dependency_type(dependency_type: str) -> None: f"Expected one of {sorted(Job._VALID_DEPENDENCY_TYPES)}." ) - _JOB_ID_PATTERN = re.compile(r"\d{7}") - @staticmethod - def _validate_dependency(dependency: str | Job | list[str] | list[Job]) -> list[str]: - deps = dependency if isinstance(dependency, list) else [dependency] - job_ids = [dep.job_id if isinstance(dep, Job) else dep for dep in deps] + def _validate_dependency(dependency: list[str] | list[Job]) -> list[str]: + job_ids = [dep.job_id if isinstance(dep, Job) else dep for dep in dependency] for job_id in job_ids: - if not Job._JOB_ID_PATTERN.fullmatch(job_id): + if not job_id.isdigit() or len(job_id)!=7: raise ValueError( f"Invalid job ID '{job_id}'. Job IDs must be 7 digits long." ) diff --git a/tests/test_job.py b/tests/test_job.py index 77081c84..34dd0cc3 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -442,29 +442,26 @@ def update_status(self) -> JobStatus: @pytest.mark.parametrize( - ("dependency", "job_id", "job_status", "expected", "expected_exception"), + ("dependency", "ids", "status", "expected"), [ pytest.param( - "1234567", - None, - None, - "afterok:1234567", - nullcontext(), + ["1234567"], + [None], + [None], + nullcontext("afterok:1234567"), id="single_job_id", ), pytest.param( ["1234567", "4567891", "7891234"], [None] * 3, [None] * 3, - "afterok:1234567:4567891:7891234", - nullcontext(), + nullcontext("afterok:1234567:4567891:7891234"), id="multiple_job_ids", ), pytest.param( - "123", - None, - None, - None, + ["123"], + [None], + [None], pytest.raises( ValueError, match=r"Invalid job ID '123'\. Job IDs must be 7 digits long\.", @@ -472,10 +469,19 @@ def update_status(self) -> JobStatus: id="invalid_job_id_too_short", ), pytest.param( - "abcdefg", - None, - None, - None, + [Job(script_path=Path("test.py"), name="job_1")], + ["12345678"], + [JobStatus.QUEUED], + pytest.raises( + ValueError, + match=r"Invalid job ID '12345678'\. Job IDs must be 7 digits long\.", + ), + id="invalid_job_id_too_long", + ), + pytest.param( + ["abcdefg"], + [None], + [None], pytest.raises( ValueError, match=r"Invalid job ID 'abcdefg'\. Job IDs must be 7 digits long\.", @@ -486,7 +492,6 @@ def update_status(self) -> JobStatus: ["1234567", "not_a_job_id"], [None] * 2, [None] * 2, - None, pytest.raises( ValueError, match=r"Invalid job ID 'not_a_job_id'\. Job IDs must be 7 digits long\.", @@ -494,11 +499,10 @@ def update_status(self) -> JobStatus: id="multiple_job_id_one_invalid", ), pytest.param( - Job(script_path=Path("test.py"), name="job_1"), - "1234567", - JobStatus.QUEUED, - "afterok:1234567", - nullcontext(), + [Job(script_path=Path("test.py"), name="job_1")], + ["1234567"], + [JobStatus.QUEUED], + nullcontext("afterok:1234567"), id="single_job_instance", ), pytest.param( @@ -508,8 +512,7 @@ def update_status(self) -> JobStatus: ], ["1234567", "4567891"], [JobStatus.QUEUED, JobStatus.QUEUED], - "afterok:1234567:4567891", - nullcontext(), + nullcontext("afterok:1234567:4567891"), id="multiple_job_instance", ), pytest.param( @@ -519,7 +522,6 @@ def update_status(self) -> JobStatus: ], ["1234567", "not_an_id"], [JobStatus.QUEUED, JobStatus.QUEUED], - None, pytest.raises( ValueError, match=r"Invalid job ID 'not_an_id'\. Job IDs must be 7 digits long\.", @@ -533,15 +535,13 @@ def update_status(self) -> JobStatus: ], ["1234567", None], [JobStatus.QUEUED, None], - "afterok:1234567:9876543", - nullcontext(), + nullcontext("afterok:1234567:9876543"), id="job_and_string_input", ), pytest.param( - Job(script_path=Path("test.py"), name="tornero"), - "1234567", - JobStatus.UNPREPARED, - None, + [Job(script_path=Path("test.py"), name="tornero")], + ["1234567"], + [JobStatus.UNPREPARED], pytest.raises( ValueError, match="Job 'tornero' has not been submitted yet.", @@ -555,7 +555,6 @@ def update_status(self) -> JobStatus: ], ["1234567", "4567896"], [JobStatus.QUEUED, JobStatus.PREPARED], - None, pytest.raises( ValueError, match="Job 'mourir_sur_scene' has not been submitted yet.", @@ -566,35 +565,28 @@ def update_status(self) -> JobStatus: ) def test_build_dependency_string_with_string_input( - dependency: str | list[str] | Job | list[Job], - job_id: str | list[str] | None, - job_status: JobStatus, + dependency: list[str] | list[Job], + ids: list[str] | None, + status: list[JobStatus], expected: str | None, - expected_exception: type[Exception], ) -> None: """Test building dependency string from string and Job inputs.""" - deps = dependency if isinstance(dependency, list) else [dependency] - ids = job_id if isinstance(job_id, list) else [job_id] - status = job_status if isinstance(job_id, list) else [job_status] - - for dep, job_id, job_status in zip(deps, ids, status, strict=True): + for dep, id, st in zip(dependency, ids, status, strict=True): if isinstance(dep, Job): - dep.status = job_status - dep.job_id = job_id + dep.status = st + dep.job_id = id - with expected_exception: - dep_str = Job._build_dependency_string(dependency) - if expected is not None: - assert dep_str == expected + with expected as e: + assert Job._build_dependency_string(dependency) == e @pytest.mark.parametrize( - ("dependency_type", "expected_exception"), + ("dependency_type", "expected"), [ - pytest.param("afterok", nullcontext(), id="afterok"), - pytest.param("afterany", nullcontext(), id="afterany"), - pytest.param("afternotok", nullcontext(), id="afternotok"), - pytest.param("after", nullcontext(), id="after"), + pytest.param("afterok", nullcontext("afterok:1234567"), id="afterok"), + pytest.param("afterany", nullcontext("afterany:1234567"), id="afterany"), + pytest.param("afternotok", nullcontext("afternotok:1234567"), id="afternotok"), + pytest.param("after", nullcontext("after:1234567"), id="after"), pytest.param( "not_a_supported_type", pytest.raises( @@ -606,12 +598,10 @@ def test_build_dependency_string_with_string_input( ], ) -# For now, only "afterok" is used in code def test_build_dependency_string_with_different_types( dependency_type: str, - expected_exception: type[Exception], + expected: type[Exception], ) -> None: """Test building dependency strings with different dependency types.""" - with expected_exception: - dep_str = Job._build_dependency_string("1234567", dependency_type) - assert dep_str == f"{dependency_type}:1234567" \ No newline at end of file + with expected as e: + assert Job._build_dependency_string("1234567", dependency_type) == e \ No newline at end of file