feat: added new 'qos' resource #241
Conversation
WalkthroughThe changes update the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
snakemake_executor_plugin_slurm/__init__.py
262-262: Undefined name jog
(F821)
🪛 GitHub Actions: CI
snakemake_executor_plugin_slurm/__init__.py
[error] 262-262: Flake8: F821 undefined name 'jog'
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
262-263: LGTM for the QoS feature addition!The implementation to add QoS support follows the same pattern as other SLURM resources like "constraint". This is a valuable addition that will make it easier for users to specify QoS settings without having to use the more complex
slurm_extraparameter.🧰 Tools
🪛 Ruff (0.8.2)
262-262: Undefined name
jog(F821)
🪛 GitHub Actions: CI
[error] 262-262: Flake8: F821 undefined name 'jog'
…ecutor-plugin-slurm into feat/add-qos-flag
|
@coderabbitai please generate tests for the resources |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 32502 🏁 Script executed: Length of output: 533 🏁 Script executed: Length of output: 6680 @cmeesters I've created comprehensive tests for the import pytest
from unittest.mock import MagicMock, patch
from snakemake_interface_common.exceptions import WorkflowError
class TestSlrmResources:
"""Test cases for the constraint and qos resources in the Executor.run_job method."""
@pytest.fixture
def mock_job(self):
"""Create a mock job with configurable resources."""
def _create_job(**resources):
mock_resources = MagicMock()
# Configure get method to return values from resources dict
mock_resources.get.side_effect = lambda key, default=None: resources.get(
key, default
)
# Add direct attribute access for certain resources
for key, value in resources.items():
setattr(mock_resources, key, value)
mock_job = MagicMock()
mock_job.resources = mock_resources
mock_job.name = "test_job"
mock_job.wildcards = {}
mock_job.is_group.return_value = False
mock_job.jobid = 1
return mock_job
return _create_job
@pytest.fixture
def mock_executor(self):
"""Create a mock executor for testing the run_job method."""
from snakemake_executor_plugin_slurm import Executor
# Create a mock workflow
mock_workflow = MagicMock()
mock_settings = MagicMock()
mock_settings.requeue = False
mock_settings.no_account = True
mock_settings.logdir = None
mock_workflow.executor_settings = mock_settings
mock_workflow.workdir_init = "/test/workdir"
# Create an executor with the mock workflow
executor = Executor(mock_workflow, None)
# Mock some executor methods to avoid external calls
executor.get_account_arg = MagicMock(return_value="")
executor.get_partition_arg = MagicMock(return_value="")
executor.report_job_submission = MagicMock()
# Return the mocked executor
return executor
def test_constraint_resource(self, mock_job, mock_executor):
"""Test that the constraint resource is correctly added to the sbatch command."""
# Create a job with a constraint resource
job = mock_job(constraint="haswell")
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert the constraint is correctly included
assert "-C 'haswell'" in call_args
def test_qos_resource(self, mock_job, mock_executor):
"""Test that the qos resource is correctly added to the sbatch command."""
# Create a job with a qos resource
job = mock_job(qos="normal")
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert the qos is correctly included
assert "--qos 'normal'" in call_args
def test_both_constraint_and_qos(self, mock_job, mock_executor):
"""Test that both constraint and qos resources can be used together."""
# Create a job with both constraint and qos resources
job = mock_job(constraint="haswell", qos="high")
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert both resources are correctly included
assert "-C 'haswell'" in call_args
assert "--qos 'high'" in call_args
def test_no_resources(self, mock_job, mock_executor):
"""Test that no constraint or qos flags are added when resources are not specified."""
# Create a job without constraint or qos resources
job = mock_job()
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert neither resource is included
assert "-C " not in call_args
assert "--qos " not in call_args
def test_empty_constraint(self, mock_job, mock_executor):
"""Test that an empty constraint is still included in the command."""
# Create a job with an empty constraint
job = mock_job(constraint="")
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert the constraint is included (even if empty)
assert "-C ''" in call_args
def test_empty_qos(self, mock_job, mock_executor):
"""Test that an empty qos is still included in the command."""
# Create a job with an empty qos
job = mock_job(qos="")
# Patch subprocess.Popen to capture the sbatch command
with patch("subprocess.Popen") as mock_popen:
# Configure the mock to return successful submission
process_mock = MagicMock()
process_mock.communicate.return_value = ("123", "")
process_mock.returncode = 0
mock_popen.return_value = process_mock
# Run the job
mock_executor.run_job(job)
# Get the sbatch command from the call
call_args = mock_popen.call_args[0][0]
# Assert the qos is included (even if empty)
assert "--qos ''" in call_argsThese tests follow the existing patterns in the codebase and thoroughly test both the
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/tests.py (1)
156-343: Consider extracting common test setup code to reduce duplication.Each test method contains similar setup code for mocking subprocess.Popen. This could be extracted to a helper method or fixture to reduce duplication.
+ @pytest.fixture + def mock_process(self): + """Create a mock process that returns a successful submission.""" + process_mock = MagicMock() + process_mock.communicate.return_value = ("123", "") + process_mock.returncode = 0 + return process_mock + + @pytest.fixture + def setup_job_submission(self, mock_process): + """Setup mock for job submission and capture sbatch command.""" + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value = mock_process + yield mock_popenThen in each test:
def test_constraint_resource(self, mock_job, mock_executor, setup_job_submission): job = mock_job(constraint="haswell") mock_executor.run_job(job) call_args = setup_job_submission.call_args[0][0] assert "-C 'haswell'" in call_argsThis would make the tests more concise and maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (10)
tests/tests.py (10)
4-4: Clean import addition for the test requirements.The addition of
patchfromunittest.mockis appropriate as it's used later in the test cases to mock thesubprocess.Popencalls.
150-154: Good test class organization.The new test class follows the existing test file structure and clearly documents its purpose - testing the constraint and qos resources in the Executor.run_job method.
156-179: Well-structured mock_job fixture.This fixture correctly replicates the pattern from the existing test classes while adding necessary job attributes for the specific tests. The configurable resources approach allows for flexible test case creation.
180-204: Comprehensive mock_executor fixture.The fixture properly initializes and mocks the Executor class with all necessary methods to isolate the tests from external dependencies.
205-229: Test correctly validates constraint resource handling.The test verifies that the
-Cflag with the appropriate constraint value is correctly included in the sbatch command.
230-251: Proper verification of the new qos resource functionality.This test correctly validates that when a job has the qos resource set, the
--qosflag with the appropriate value is included in the sbatch command.
252-274: Good test for combined resource usage.Testing both resources together ensures they don't interfere with each other and validates the core functionality of this PR.
275-300: Appropriate test for absence of resources.This test ensures no flag is added when resources aren't specified, which is an important negative test case.
301-322: Edge case coverage for empty constraint values.Good test to verify that an empty constraint string still results in the flag being included, matching expected SLURM behavior.
323-343: Edge case coverage for empty qos values.Similarly, this test verifies that an empty qos string still results in the flag being included in the command.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
23-25: Ensure PyTest Upgrade is Appropriate
The new step guarantees that the latest version of PyTest is installed, which can help benefit from recent fixes and features. However, please verify that this upgrade doesn’t inadvertently introduce breaking changes for tests or other dependencies. Consider pinning a version range (e.g.,pytest>=7.0,<8.0) after confirming compatibility if stability becomes a concern.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
|
please parse the results of the action run, @coderabbitai, https://github.com/snakemake/snakemake-executor-plugin-slurm/actions/runs/13901260630/job/38893362329?pr=241. The test apparently succeeds, the pytest library is in trouble. What is causing this side effect? How to avoid it? |
|
Tip For best results, initiate chat on the files or code changes. @cmeesters: One common culprit is a naming or path-shadowing conflict that disrupts how pytest loads. For instance, a file or directory named |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/tests.py (3)
107-114: Validate necessity ofpatch_sys_streamsfixture.
It's useful for capturing or suppressing console output but ensure it’s strictly needed. If these tests pass without it, you might simplify by removing the fixture to reduce complexity. Otherwise, it’s fine as-is.
130-153: Consider consolidating duplicate job fixtures.
This new fixture largely parallels the one inTestGresString. If you plan to reuse them extensively, refactoring both into a shared fixture can help reduce code duplication and ensure consistency.
297-318: Confirm expected behavior for empty qos.
Similarly, sending--qos ''might yield unexpected results with certain SLURM setups. If the intent is truly to set an empty string, the test is correct. Otherwise, consider omitting this flag when the resource is empty.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (4) (4)
Executor(137:761)get_account_arg(611:637)get_partition_arg(639:654)run_job(192:354)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (2)
tests/tests.py (2)
4-5: No concerns on these imports.
They are standard and cohesive with testing needs.
275-296: Confirm expected behavior for empty constraint.
Currently, the test confirms that-C ''is inserted in the sbatch command. Ensure this is truly desired, as an empty constraint might cause unexpected handling in certain cluster configurations.Could you check your cluster’s SLURM documentation or run a test submission of “sbatch -C '' …” to confirm that it yields the intended result?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/tests.py (2)
4-5: Remove unused import.The
sysmodule is imported but not used anywhere in the code, as confirmed by the static analyzer.from unittest.mock import MagicMock, patch -import sys import pytest🧰 Tools
🪛 Ruff (0.8.2)
5-5:
sysimported but unusedRemove unused import:
sys(F401)
122-145: Consider refactoring duplicate fixtures.The
mock_jobfixture here is very similar to the one inTestGresString. You could consider refactoring this into a shared fixture to reduce code duplication.# Create a shared fixture at the module level @pytest.fixture def base_mock_job(): """Create a configurable mock job with resources.""" def _create_job(**resources): mock_resources = MagicMock() mock_resources.get.side_effect = lambda key, default=None: resources.get( key, default ) for key, value in resources.items(): setattr(mock_resources, key, value) mock_job = MagicMock() mock_job.resources = mock_resources return mock_job return _create_job # Then update TestGresString and TestSLURMResources to use it class TestSLURMResources: @pytest.fixture def mock_job(self, base_mock_job): """Create a mock job with SLURM-specific attributes.""" def _create_job(**resources): mock_job = base_mock_job(**resources) mock_job.name = "test_job" mock_job.wildcards = {} mock_job.is_group.return_value = False mock_job.jobid = 1 return mock_job return _create_job
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (4) (4)
Executor(137:761)get_account_arg(611:637)get_partition_arg(639:654)run_job(192:354)
🪛 Ruff (0.8.2)
tests/tests.py
5-5: sys imported but unused
Remove unused import: sys
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (8)
tests/tests.py (8)
116-120: The new test class covers the requirements well.This test class effectively tests both the new
qosresource and the existingconstraintresource as requested in the PR comments.
146-170: Well-structured executor fixture.The
mock_executorfixture is comprehensive and properly mocks the necessary components to test the run_job method without making external calls.
171-195: Good test for constraint resource.The test correctly verifies that the constraint resource is properly added to the sbatch command with the right flag.
196-217: Good test for the new qos resource.This test verifies the core functionality added by this PR, ensuring that the qos resource is correctly added to the sbatch command with the appropriate flag.
218-240: Good combined test case.Testing both resources together ensures that they don't interfere with each other and can be used simultaneously, which is an important edge case to cover.
241-266: Good negative test case.Testing the scenario where no resources are specified ensures that no flags are added unnecessarily, which is important for correctness.
267-288: Good edge case handling for empty constraint.Testing that an empty constraint is still included in the command is important for ensuring consistent behavior with edge cases.
289-310: Good edge case handling for empty qos.Similarly, testing that an empty qos is still included in the command ensures consistent behavior with edge cases for the new feature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tests.py (1)
121-144: Consider refactoring duplicate mock_job fixture.This fixture is very similar to the one in
TestGresString. Consider extracting this into a common fixture or utility function to reduce code duplication.- @pytest.fixture - def mock_job(self): - """Create a mock job with configurable resources.""" - - def _create_job(**resources): - mock_resources = MagicMock() - # Configure get method to return values from resources dict - mock_resources.get.side_effect = lambda key, default=None: resources.get( - key, default - ) - # Add direct attribute access for certain resources - for key, value in resources.items(): - setattr(mock_resources, key, value) - - mock_job = MagicMock() - mock_job.resources = mock_resources - mock_job.name = "test_job" - mock_job.wildcards = {} - mock_job.is_group.return_value = False - mock_job.jobid = 1 - return mock_job - - return _create_jobYou could create a common fixture in a conftest.py file or extend the existing one:
@pytest.fixture def slurm_mock_job(): """Create a mock job with configurable resources for SLURM tests.""" def _create_job(**resources): mock_resources = MagicMock() mock_resources.get.side_effect = lambda key, default=None: resources.get( key, default ) for key, value in resources.items(): setattr(mock_resources, key, value) mock_job = MagicMock() mock_job.resources = mock_resources # Add job attributes that are common to all tests mock_job.name = "test_job" mock_job.wildcards = {} mock_job.is_group.return_value = False mock_job.jobid = 1 return mock_job return _create_job
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (4) (4)
Executor(137:761)get_account_arg(611:637)get_partition_arg(639:654)run_job(192:354)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (10)
tests/tests.py (10)
4-4: Good addition of the patch import for mocking.The addition of
patchfromunittest.mockis appropriate for mockingsubprocess.Popenin the new tests.
115-120: Well-structured test class for SLURM resources.The new test class has a clear purpose, good documentation, and appropriately focuses on testing both the new
qosresource and existingconstraintresource.
145-169: Well-implemented mock_executor fixture.The fixture properly mocks the necessary components to isolate test execution from external dependencies and provides a clean testing environment for the SLURM executor.
170-194: Test for constraint resource is well-structured.This test correctly verifies that the constraint resource is properly passed to the sbatch command with the
-Cflag.
195-216: Test for qos resource is well-implemented.This test correctly verifies that the qos resource (which is the main feature of this PR) is properly passed to the sbatch command with the
--qosflag.
217-239: Good test for combined resource usage.Testing both resources together ensures they work correctly in combination, which is an important use case.
240-265: Important negative test case.Testing the absence of flags when no resources are specified is a good practice to ensure the command isn't modified unnecessarily.
1-344: Overall assessment: Well-implemented tests for the new qos resource.The test suite is comprehensive, covering both the new qos resource and its interaction with the existing constraint resource. The tests follow a consistent pattern, are well-documented, and include important edge cases.
Two areas for consideration:
- The commented-out test case should be addressed
- Consider refactoring the duplicate mock_job fixture
Otherwise, the implementation is solid and aligns well with the PR objectives.
288-308:❓ Verification inconclusive
Verify the handling of empty qos values.
Similar to the empty constraint test, this test confirms that empty qos values are included in the command. Consider whether this is the intended behavior or if the flag should be omitted for empty values.
Run the following script to check how SLURM handles empty qos values:
🏁 Script executed:
#!/bin/bash # Description: Verify how SLURM handles empty qos values # Test empty qos echo "Testing empty qos (--qos '')" sbatch --help | grep -A 2 "\-\-qos" || echo "QOS flag not found in help" # Try a test submission with empty qos (dry run mode) echo "Attempting mock submission with empty qos" echo '#!/bin/bash\necho "Test"' > test_script.sh chmod +x test_script.sh sbatch --wrap="echo test" --qos '' --test-only || echo "Empty qos flag may cause issues" rm test_script.shLength of output: 663
Action: Verify the intended behavior for handling empty qos values.
- The test in
tests/tests.pyasserts that an empty qos value results in including the flag ("--qos ''") in the sbatch command.- Our verification script couldn’t retrieve SLURM behavior due to the unavailability of the
sbatchcommand in the test environment. This means we haven’t confirmed whether SLURM ignores an empty qos flag or might produce issues.- Please verify in a proper SLURM environment (or by consulting SLURM documentation) if including an empty qos flag is intended. If SLURM expects the flag to be omitted when empty, consider updating both the code implementation and the test accordingly.
266-287:❓ Verification inconclusive
Verify the handling of empty constraint values.
The test confirms that empty constraint values are still included in the command. While this is consistent with the implementation, consider whether passing empty strings to SLURM is the intended behavior or if it would be better to skip adding the flag entirely.
Run the following script to check how SLURM handles empty constraint values:
🏁 Script executed:
#!/bin/bash # Description: Verify how SLURM handles empty constraint values # Test empty constraint echo "Testing empty constraint (-C '')" sbatch --help | grep -A 2 "\-C" || echo "Constraint flag not found in help" # Try a test submission with empty constraint (dry run mode) echo "Attempting mock submission with empty constraint" echo '#!/bin/bash\necho "Test"' > test_script.sh chmod +x test_script.sh sbatch --wrap="echo test" -C '' --test-only || echo "Empty constraint flag may cause issues" rm test_script.shLength of output: 706
Review Update: Confirming Intended Behavior for Empty Constraint Values
The current test in
tests/tests.pyproperly confirms that an empty constraint (-C '') is still being passed to the command. However, our additional dry-run script indicated that in our test environment (wheresbatchisn’t available) we couldn't validate how SLURM processes such an empty flag. This leaves the question open: Is passing an empty constraint intended, or should the implementation omit the flag when its value is empty?
- Action Required: Please verify on an actual SLURM system whether including an empty constraint flag causes any unintended issues. If SLURM treats the empty flag undesirably, consider updating the implementation to skip adding the
-Cflag when the constraint is empty.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/tests.py (5)
54-87: Consider relocatingmock_executorfixture to theTestSLURMResourcesclass.The
mock_executorfixture seems to be defined in theTestGresStringclass but isn't used there. It's only used in theTestSLURMResourcesclass. For better organization and clarity, consider removing this fixture from here and just keep the one defined inTestSLURMResources.
159-182: Reduce code duplication by sharing fixtures between test classes.The
mock_jobandmock_executorfixtures inTestSLURMResourcesare nearly identical to those defined inTestGresString. Consider refactoring these into shared fixtures at the module level to avoid duplication and make future maintenance easier.- @pytest.fixture - def mock_job(self): - """Create a mock job with configurable resources.""" - - def _create_job(**resources): - mock_resources = MagicMock() - # Configure get method to return values from resources dict - mock_resources.get.side_effect = lambda key, default=None: resources.get( - key, default - ) - # Add direct attribute access for certain resources - for key, value in resources.items(): - setattr(mock_resources, key, value) - - mock_job = MagicMock() - mock_job.resources = mock_resources - mock_job.name = "test_job" - mock_job.wildcards = {} - mock_job.is_group.return_value = False - mock_job.jobid = 1 - return mock_job - - return _create_job - - @pytest.fixture - def mock_executor(self): - """Create a mock executor for testing the run_job method.""" - from snakemake_executor_plugin_slurm import Executor - - # Create a mock workflow - mock_workflow = MagicMock() - mock_settings = MagicMock() - mock_settings.requeue = False - mock_settings.no_account = True - mock_settings.logdir = None - mock_workflow.executor_settings = mock_settings - mock_workflow.workdir_init = "/test/workdir" - - # Create an executor with the mock workflow - executor = Executor(mock_workflow, None) - - # Mock some executor methods to avoid external calls - executor.get_account_arg = MagicMock(return_value="") - executor.get_partition_arg = MagicMock(return_value="") - executor.report_job_submission = MagicMock() - - # Return the mocked executor - return executorInstead, use the existing fixtures from the
TestGresStringclass:# TestSLURMResources can just use the fixtures from TestGresStringAlso applies to: 183-207
217-232: Simplify test boilerplate by extracting common mock setup patterns.Each test method contains nearly identical boilerplate code for setting up the subprocess.Popen mock and extracting the command arguments. This could be refactored into a helper method or fixture to reduce duplication and make the tests more concise and maintainable.
@pytest.fixture def mock_subprocess_popen(self): """Create a mock for subprocess.Popen that succeeds and returns job ID 123.""" with patch("subprocess.Popen") as mock_popen: # Configure the mock to return successful submission process_mock = MagicMock() process_mock.communicate.return_value = ("123", "") process_mock.returncode = 0 mock_popen.return_value = process_mock yield mock_popen def _assert_command_contains(self, command, expected_parts): """Helper to assert that a command contains all expected parts.""" for part in expected_parts: assert part in command def _run_job_and_get_command(self, mock_popen, mock_executor, job): """Run a job and extract the command args.""" mock_executor.run_job(job) return mock_popen.call_args[0][0]Then use these in your test methods to simplify them:
def test_constraint_resource(self, mock_job, mock_executor, mock_subprocess_popen): """Test that the constraint resource is correctly added to the sbatch command.""" job = mock_job(constraint="haswell") command = self._run_job_and_get_command(mock_subprocess_popen, mock_executor, job) self._assert_command_contains(command, ["-C 'haswell'"])Also applies to: 239-254, 261-277, 287-303, 310-325, 332-347
220-220: Use byte strings for communicate() return values for consistency.In the mock setup,
process_mock.communicate.return_valueis set to string tuples like("123", "")instead of byte tuples like(b"123", b""). This contrasts with theTestGresString.mock_executorfixture which correctly uses byte tuples. While Python will implicitly convert these in many cases, it's better to be consistent with the actual behavior ofsubprocess.Popen.communicate()which returns bytes.- process_mock.communicate.return_value = ("123", "") + process_mock.communicate.return_value = (b"123", b"")Also applies to: 242-242, 264-264, 290-290, 313-313, 335-335
222-222: Add stdout and stderr attributes to process_mock for full compatibility.In the
TestGresString.mock_executorfixture,process_mockhasstdoutandstderrattributes set, but these are missing in theTestSLURMResourcestest methods. While they might not be used in the current tests, adding them would make the mocks more complete and consistent.process_mock.communicate.return_value = ("123", "") process_mock.returncode = 0 +process_mock.stdout = MagicMock() +process_mock.stderr = MagicMock() mock_popen.return_value = process_mockAlso applies to: 244-244, 266-266, 292-292, 315-315, 337-337
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (1)
tests/tests.py (1)
153-347: Well-structured test implementation for QoS and constraint resources.The test implementation is comprehensive and follows good testing practices:
- Each test case targets a specific functionality
- Proper use of mocking to isolate the tested components
- Good coverage of different scenarios (single resource, combined resources, empty values)
- Thorough assertions to verify the command arguments
…sting and future tests
…ecutor-plugin-slurm into feat/add-qos-flag
|
@coderabbitai - the previous tests with your mock_executor all failed, because a) the Executor class needed to submit jobs, but the sbatch command was not extractable and b) some bug in pytest caused to read(!) from stdout, which ought not to be possible and - of course - always failed. Please withdraw your earlier suggestion to create test suite. |
|
Tip For best results, initiate chat on the files or code changes. @cmeesters I understand the issues you've encountered with my suggested test suite. The approach I proposed was fundamentally flawed due to the inability to extract the sbatch command and the unexpected pytest behavior with stdout. I hereby withdraw my earlier suggestion for creating the test suite. I apologize for the inconvenience and thank you for providing this feedback. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
snakemake_executor_plugin_slurm/submit_string.py (3)
33-37: Unusual conditional logic forconstraintresource.The condition
job.resources.get("constraint") or isinstance(job.resources.get("constraint"), str)will evaluate toTrueeven for empty strings. This appears to be intentional based on the test cases, but the logic could be simplified.Consider simplifying the conditional logic:
-if job.resources.get("constraint") or isinstance( - job.resources.get("constraint"), str -): +if job.resources.get("constraint") is not None: call += f" -C '{job.resources.get('constraint')}'"
38-39: Similar unusual conditional logic forqosresource.The same unusual conditional pattern is used for the
qosresource as for theconstraintresource.Consider simplifying this condition as well:
-if job.resources.get("qos") or isinstance(job.resources.get("qos"), str): +if job.resources.get("qos") is not None: call += f" --qos='{job.resources.get('qos')}'"
5-7: Function docstring could be more descriptive.The current docstring is minimal. Adding more details about the parameters and return value would improve code documentation.
Consider expanding the docstring:
def get_submit_command(job, params): """ - Return the submit command for the job. + Return the sbatch command string for submitting a job to SLURM. + + Parameters: + ----------- + job : object + The job object containing resources and requirements. + params : dict + Dictionary with submission parameters like run_uuid, slurm_logfile, + comment_str, account, partition, and workdir. + + Returns: + -------- + str + The complete sbatch command string. """tests/tests.py (2)
226-248: Code duplication in test fixtures.The
mock_jobfixture inTestSLURMResourcesduplicates the one inTestGresString. This violates the DRY principle.Consider moving this fixture to a common base class or utility function to avoid duplication.
# Example of a shared fixture at module level @pytest.fixture def create_mock_job(): """Create a mock job with configurable resources.""" def _create_job(**resources): mock_resources = MagicMock() mock_resources.get.side_effect = lambda key, default=None: resources.get( key, default ) for key, value in resources.items(): setattr(mock_resources, key, value) mock_job = MagicMock() mock_job.resources = mock_resources mock_job.name = "test_job" mock_job.wildcards = {} mock_job.is_group.return_value = False mock_job.jobid = 1 return mock_job return _create_job
324-327: Tests have unnecessary mocking.The mocks for
subprocess.Popenare set up but not used in the test execution. In this test, you're only checking the command string returned byget_submit_command, not actually executing any subprocess calls.Remove the unnecessary mocking to simplify the test:
- # Patch subprocess.Popen to capture the sbatch command - with patch("subprocess.Popen") as mock_popen: - # Configure the mock to return successful submission - process_mock = MagicMock() - process_mock.communicate.return_value = ("123", "") - process_mock.returncode = 0 - mock_popen.return_value = process_mock - - # Assert both resources are correctly included - sbatch_command = get_submit_command(job, params) - assert " --qos='high'" in sbatch_command - assert " -C 'haswell'" in sbatch_command + # Assert both resources are correctly included + sbatch_command = get_submit_command(job, params) + assert " --qos='high'" in sbatch_command + assert " -C 'haswell'" in sbatch_commandThis applies to all other test methods in
TestSLURMResourcesas well.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snakemake_executor_plugin_slurm/__init__.py(3 hunks)snakemake_executor_plugin_slurm/submit_string.py(1 hunks)tests/tests.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_slurm/init.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (8)
tests/tests.py (8)
285-294: Similar unused parameters in test setup.Same issue as in the previous test - the
qoskey inparamsappears to be unused.
312-313: Unnecessary parameters in test setup.Similar to previous comments,
constraintandqoskeys inparamsappear to be unused.
255-275: LGTM! Comprehensive test coverage for theconstraintresource.The test correctly verifies that the constraint resource is properly added to the sbatch command with the correct format.
277-299: LGTM! Comprehensive test coverage for theqosresource.The test correctly verifies that the qos resource is properly added to the sbatch command with the correct format.
358-381: LGTM! Good edge case testing for empty constraint values.The test correctly verifies that an empty constraint string is still included in the command, which is important for correct SLURM behavior.
383-405: LGTM! Good edge case testing for empty qos values.The test correctly verifies that an empty qos string is still included in the command.
329-357: LGTM! Proper negative test case for when no resources are specified.The test correctly verifies that no constraint or qos flags are added when these resources aren't specified.
219-224: LGTM! Well-structured test class with clear purpose.The new
TestSLURMResourcesclass is well documented and has a clear purpose of testing the job resources passed to the SLURM executor.
Co-authored-by: Filipe G. Vieira <1151762+fgvieira@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/submit_string.py (1)
4-7: Consider adding docstring details for parameters and return value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/submit_string.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (14)
snakemake_executor_plugin_slurm/submit_string.py (14)
1-2: No issues found with the import statement.
8-16: Multi-line string construction for the basesbatchcall is clear and concise.
24-25: Usage of--clustershere appears correct for specifying clusters.
27-28: Specifying-tfor runtime is correctly aligned with SLURM usage.
30-31: Duplicated--clusterscode.Removing the second instance ensures
--clustersis not added twice:- if job.resources.get("clusters"): - call += f" --clusters {job.resources.clusters}"
33-37: Constraint parameter with-Cis properly formed for SLURM.
38-39:--qosparameter is correctly implemented.
40-43: Memory specification logic aligns with SLURM's--memor--mem-per-cpuoptions.
45-46: Node count usage with--nodesis correct.
50-55: GPU-based logic for tasks per GPU versus tasks is suitably handled.
56-60: Appending CPU settings and extra SLURM arguments is well-structured.
62-65: Work directory setting with-Dis standard SLURM practice.
67-67: Return statement is straightforward and finalizes the submission command.
68-68: No code present on this line.
…ecutor-plugin-slurm into feat/add-qos-flag
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Filipe G. Vieira <1151762+fgvieira@users.noreply.github.com>
…ecutor-plugin-slurm into feat/add-qos-flag
|
@fgvieira now, please take a look ;-). PS the tinkering after coderabbit's remarks left a mark on this PR, sorry for that. Actually, only the latest bits do matter. A small change in code, triggered quite some refactoring, here. |
🤖 I have created a release *beep* *boop* --- ## [1.2.0](v1.1.0...v1.2.0) (2025-04-04) ### Features * added new 'qos' resource ([#241](#241)) ([c8cdfc4](c8cdfc4)) ### Bug Fixes * account and partition settings corrected ([#249](#249)) ([e7a248f](e7a248f)) * case insensitive matching of declared slurm account ([#244](#244)) ([dc9a4fd](dc9a4fd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new QoS resource. - **Bug Fixes** - Resolved issues with account and partition settings. - Improved case insensitive matching for account names. - **Chores** - Updated the package version to 1.2.0. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
depsite the branch name, this PR introduces a new resource:
qos. Asqosseems to be frequently used and similar toconstraint, adding this resource, rather than requiring the somewhat convoluted use ofslurm_extraseems a more service oriented approach.Summary by CodeRabbit
New Features
Bug Fixes
constraintandqosparameters.Tests
constraintandqos.Chores
pytestdependency to a newer version for potential improvements and new features.