Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors unit-test generation to use pathlib, add config handling, support deployment_method and rundir, render conftest.py, and update templates to run Snakemake via "python -m snakemake" with conda-prefix handling and improved output comparison tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Workflow
participant UnitTests as unit_tests.generate
participant Generator as GeneratedFiles
participant Pytest
participant Test as GeneratedTest
participant Snakemake
User->>Workflow: snakemake --generate-unit-tests
Workflow->>Workflow: determine rundir = Path.cwd()
Workflow->>UnitTests: generate(dag, path, deployment_method, snakefile, configfiles, rundir)
UnitTests->>UnitTests: copy config*/data/expected (copy_files), mkdirs, set perms
UnitTests->>Generator: render conftest.py and ruletest.py (paths relative to rundir)
User->>Pytest: pytest .tests/unit/ [--conda-prefix ...]
Pytest->>Test: load conftest, provide conda_prefix fixture
Pytest->>Test: run test_<rule>(conda_prefix)
Test->>Test: prepare workdir (copy config & data)
Test->>Snakemake: python -m snakemake [targets] [--snakefile ...] [--configfile ...] --directory workdir --software-deployment-method ...
Snakemake-->>Test: finish run
Test->>Test: common.OutputChecker.check() (file comparisons)
Test-->>Pytest: assertions/pass or fail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/unit_tests/__init__.py(2 hunks)src/snakemake/unit_tests/templates/ruletest.py.jinja2(1 hunks)src/snakemake/workflow.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/unit_tests/__init__.pysrc/snakemake/workflow.py
🧬 Code Graph Analysis (2)
src/snakemake/unit_tests/__init__.py (1)
src/snakemake/workflow.py (1)
snakefile(1543-1548)
src/snakemake/workflow.py (1)
src/snakemake/rules.py (1)
snakefile(142-143)
🪛 Ruff (0.12.2)
src/snakemake/unit_tests/__init__.py
32-32: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (5)
src/snakemake/unit_tests/templates/ruletest.py.jinja2 (2)
30-33: LGTM! Conditional snakefile argument implementation is correct.The conditional block properly adds the
--snakefileargument to the generated test command only when a snakefile is provided, which aligns with the PR objectives.
40-40: Critical fix: Config file path is now properly quoted.This change prevents syntax errors when config file paths contain spaces or special characters, directly addressing the PR's objective of adding missing quotes.
src/snakemake/unit_tests/__init__.py (2)
31-33: LGTM! Function signature properly extended with snakefile parameter.The addition of the optional
snakefile=Noneparameter is well-placed and maintains backward compatibility while enabling the new functionality.
110-110: Correct integration with template context.The snakefile parameter is properly passed to the Jinja2 template, enabling the conditional snakefile argument functionality in the generated test scripts.
src/snakemake/workflow.py (1)
894-899: Perfect integration completing the snakefile support feature.The addition of
snakefile=self.main_snakefileproperly connects the workflow's main snakefile to the unit test generation, completing the implementation across all modified files.
johanneskoester
requested changes
Jul 25, 2025
Contributor
johanneskoester
left a comment
There was a problem hiding this comment.
Do you think it is feasible to check whether all this works via a testcase?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py (2)
1-4: Documentation comment needs correction.The docstring references "Snakemake 9.8.2.dev35" which appears to be a development version. For expected test results, this should either be a stable version or use a placeholder.
-Rule test code for unit testing of rules generated with Snakemake 9.8.2.dev35. +Rule test code for unit testing of rules generated with Snakemake.
53-58: Output verification implementation is well-designed.The new
OutputCheckerapproach provides flexible file comparison with support for different compression formats (cmp/zcmp/bzcmp). The comments clearly explain how to customize the behavior.Note: There's a minor typo in the comment on line 55 - "zmp" should be "zcmp":
- # To modify this behavior, you can inherit from common.OutputChecker in here - # and overwrite the method `compare_files(generated_file, expected_file), - # also see common.py. + # To modify this behavior, you can inherit from common.OutputChecker in here + # and overwrite the method `compare_files(generated_file, expected_file)`, + # also see common.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/a/data/input.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
- tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-08-13T11:05:23.821Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
📚 Learning: changes made within test cases, such as in `tests/test_wrapper/snakefile`, are for testing purposes ...
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: new test dependencies for snakemake should be introduced in separate prs rather than being added as ...
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: in the snakemake repository, avoid suggesting return type annotations for test functions....
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: in `snakemake/rules.py`, within the `rule` class, `self.basedir` is a custom class (not a `pathlib.p...
Learnt from: johanneskoester
PR: snakemake/snakemake#3132
File: snakemake/rules.py:1105-1105
Timestamp: 2024-10-11T12:49:08.705Z
Learning: In `snakemake/rules.py`, within the `Rule` class, `self.basedir` is a custom class (not a `pathlib.Path` object) that has a `join()` method. Therefore, using `self.basedir.join(conda_env)` is appropriate in this context.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: in the snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing,...
Learnt from: johanneskoester
PR: snakemake/snakemake#3204
File: tests/test_cores_cluster/qsub:1-6
Timestamp: 2024-11-12T20:22:54.184Z
Learning: In the Snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing, and input validation and error handling are not required in such scripts.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: in test scripts within the snakemake project, concise code is preferred over verbose error handling ...
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
📚 Learning: in the snakemake test suite, tests for configuration keys like 'group-components' may already be cov...
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (56)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (3)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py (3)
16-16: LGTM! Function signature correctly fixed.The function now properly accepts
conda_prefixas a required parameter, addressing the previous mutable default argument issue.
32-51: Configfile parameter quoting is handled by unit test templates
The Jinja2 template insnakemake/unit_tests/templates/ruletest.py.jinja2already wraps eachconfigfilevalue in quotes:
- It emits
"--configfile",followed by"<configfile>"for each entry.
This ensures the path is correctly quoted at execution time. No changes required.
18-30: Directory structure verified.The
.tests/unit/a/dataand.tests/unit/a/expecteddirectories are present undertests/test_generate_unit_tests/expected-results, and there is no top-levelconfig/folder (so the conditional copy will simply be skipped). No changes required.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py (1)
47-48: Add error handling for subprocess call.The
subprocess.check_outputcall can raiseCalledProcessErrorif the file comparison fails, but this exception should be caught and converted to a more informative error message.- def compare_files(self, generated_file, expected_file): - sp.check_output(["cmp", generated_file, expected_file]) + def compare_files(self, generated_file, expected_file): + try: + sp.check_output(["cmp", generated_file, expected_file]) + except sp.CalledProcessError as e: + raise ValueError(f"File comparison failed: {expected_file} vs {generated_file}") from e
🧹 Nitpick comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py (1)
16-45: Fix unused loop variables.The static analysis correctly identifies unused
subdirsvariables in theos.walkcalls.Apply this diff to fix the unused variables:
- input_files = set( - (Path(path) / f).relative_to(self.data_path) - for path, subdirs, files in os.walk(self.data_path) - for f in files - ) - expected_files = set( - (Path(path) / f).relative_to(self.expected_path) - for path, subdirs, files in os.walk(self.expected_path) - for f in files - ) + input_files = set( + (Path(path) / f).relative_to(self.data_path) + for path, _, files in os.walk(self.data_path) + for f in files + ) + expected_files = set( + (Path(path) / f).relative_to(self.expected_path) + for path, _, files in os.walk(self.expected_path) + for f in files + ) unexpected_files = set() - for path, subdirs, files in os.walk(self.workdir): + for path, _, files in os.walk(self.workdir):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/snakemake/unit_tests/templates/common.py.jinja2(2 hunks)src/snakemake/unit_tests/templates/ruletest.py.jinja2(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/common.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py(2 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
- src/snakemake/unit_tests/templates/common.py.jinja2
- tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
- src/snakemake/unit_tests/templates/ruletest.py.jinja2
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-08-13T11:05:23.821Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
📚 Learning: changes made within test cases, such as in `tests/test_wrapper/snakefile`, are for testing purposes ...
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
📚 Learning: new test dependencies for snakemake should be introduced in separate prs rather than being added as ...
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
📚 Learning: in the snakemake test suite, tests for configuration keys like 'group-components' may already be cov...
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
📚 Learning: in test scripts within the snakemake project, concise code is preferred over verbose error handling ...
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
📚 Learning: avoid adding input validation or error handling that unnecessarily complicates the code in the `snak...
Learnt from: johanneskoester
PR: snakemake/snakemake#3171
File: snakemake/cli.py:106-145
Timestamp: 2024-10-29T09:26:26.636Z
Learning: Avoid adding input validation or error handling that unnecessarily complicates the code in the `snakemake` codebase, especially when the cases handled don't make sense.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
📚 Learning: in the snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing,...
Learnt from: johanneskoester
PR: snakemake/snakemake#3204
File: tests/test_cores_cluster/qsub:1-6
Timestamp: 2024-11-12T20:22:54.184Z
Learning: In the Snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing, and input validation and error handling are not required in such scripts.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
🪛 Ruff (0.12.2)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
28-28: Loop control variable subdirs not used within loop body
Rename unused subdirs to _subdirs
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (56)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (2)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py (2)
1-8: LGTM!The imports and module structure are appropriate for the test utility functionality.
10-14: LGTM!Simple and clean initialization for the test utility class.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (1)
16-16: Good fix for the mutable default argument issue.The function signature change removes the mutable default argument, which fixes the potential for unexpected behavior across multiple test runs. The
conda_prefixparameter is now properly supplied by the pytest fixture system.
🧹 Nitpick comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (1)
52-57: Great improvement to output verification system.The new
OutputCheckerapproach is much better:
- Centralizes comparison logic for reusability
- Supports compressed files with appropriate tools
- Provides clear customization guidance
- More maintainable than inline comparison
Minor formatting issue:
- # To modify this behavior, you can inherit from common.OutputChecker in here - # and overwrite the method `compare_files(generated_file, expected_file), + # To modify this behavior, you can inherit from common.OutputChecker in here + # and overwrite the method `compare_files(generated_file, expected_file),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py
- tests/test_generate_unit_tests/expected-results/.tests/unit/common.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-08-13T11:05:23.821Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: johanneskoester
PR: snakemake/snakemake#2960
File: CHANGELOG.md:6-6
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Do not review pull requests generated by Release Please for the Snakemake project.
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: snakemake/snakemake#2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
📚 Learning: changes made within test cases, such as in `tests/test_wrapper/snakefile`, are for testing purposes ...
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: new test dependencies for snakemake should be introduced in separate prs rather than being added as ...
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: in the snakemake repository, avoid suggesting return type annotations for test functions....
Learnt from: johanneskoester
PR: snakemake/snakemake#2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: in the snakemake test suite, tests for configuration keys like 'group-components' may already be cov...
Learnt from: johanneskoester
PR: snakemake/snakemake#3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: the file "test.jsonl" in tests/test_nonstr_params is automatically created by snakemake, rather than...
Learnt from: johanneskoester
PR: snakemake/snakemake#2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: in `snakemake/rules.py`, within the `rule` class, `self.basedir` is a custom class (not a `pathlib.p...
Learnt from: johanneskoester
PR: snakemake/snakemake#3132
File: snakemake/rules.py:1105-1105
Timestamp: 2024-10-11T12:49:08.705Z
Learning: In `snakemake/rules.py`, within the `Rule` class, `self.basedir` is a custom class (not a `pathlib.Path` object) that has a `join()` method. Therefore, using `self.basedir.join(conda_env)` is appropriate in this context.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: in the snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing,...
Learnt from: johanneskoester
PR: snakemake/snakemake#3204
File: tests/test_cores_cluster/qsub:1-6
Timestamp: 2024-11-12T20:22:54.184Z
Learning: In the Snakemake codebase, the `tests/test_cores_cluster/qsub` script is a dummy script for testing, and input validation and error handling are not required in such scripts.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
📚 Learning: in test scripts within the snakemake project, concise code is preferred over verbose error handling ...
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (57)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: formatting
- GitHub Check: apidocs
🔇 Additional comments (4)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (4)
1-4: LGTM! Version information is helpful for generated tests.Adding the Snakemake version to the generated test docstring is a good practice for tracking which generator version created the test.
9-11: LGTM! Imports align with new functionality.The added
tempfileand explicitcheck_outputimports are necessary for the updated test implementation using temporary directories and subprocess execution.
18-30: Excellent improvements to directory and path handling.The changes enhance the test robustness:
- Using
TemporaryDirectorycontext manager ensures proper cleanup- Converting to
Pathobjects improves path manipulation- Conditional config directory copying supports workflows with configuration files
- The
dirs_exist_ok=Trueparameter properly handles existing directoriesThese changes align well with the PR objective to support config files in unit tests.
32-50: Excellent fix for the config file path issue!The rewritten command invocation addresses the core issue from #843:
- Using
python -m snakemakeis more reliable than depending on PATH- Explicit
--configfile config/config.jsonargument properly handles config files- The command structure is clear and reproducible
- Appending
conda_prefixprovides flexible environment managementThis directly fixes the syntax error issue mentioned in the PR objectives where config file paths weren't properly quoted.
Contributor
Mhm, indeed, I think it makes most sense to have the unit tests being put relative to the original workdir, not the one that is set via -d. |
Contributor
Author
|
Should we then change the default? Or should we just explain it the docs that the user needs to specify the unit tests path in these cases? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/snakemake/unit_tests/templates/common.py.jinja2 (1)
23-23: Remove leftover debug printsThese prints add noise to test output and should not ship in generated tests.
- print(f"input: {input_files}") # DEBUG + # input_files collected ... - print(f"workdir: {workdir_files}") # DEBUG + # workdir_files collected ... - print(f"expected: {expected_files}") # DEBUG + # expected_files collectedAlso applies to: 30-30, 37-37
src/snakemake/unit_tests/__init__.py (1)
34-36: Fix mutable default argumentUsing a mutable list as a default argument can cause unexpected behavior if the list is modified during function execution.
Apply this fix:
def generate( - dag, path: Path, deploy=None, snakefile=None, configfiles=None, rundir=None + dag, path: Path, deploy=None, snakefile=None, configfiles=None, rundir=None ): + if deploy is None: + deploy = ["conda", "singularity"]Then add the initialization at the beginning of the function body:
if deploy is None: deploy = ["conda", "singularity"]
🧹 Nitpick comments (6)
tests/test_generate_unit_tests/expected-results/.tests/units/conftest.py (1)
1-3: Avoid embedding dev version in generated conftest to reduce churnEmbedding a dev version string in generated files causes noisy diffs every bump. Consider making the header version-agnostic.
Apply this diff to the docstring:
-""" -conftest.py for unit testing of rules generated with Snakemake 9.8.2.dev50. -""" +""" +conftest.py for unit testing of rules generated with Snakemake. +"""docs/snakefiles/testing.rst (1)
30-31: Document platform fallback for comparisonsGiven the template now supports multiple compressors and may need to run on Windows, clarify that a Python fallback is used if external tools are unavailable.
-By default, the comparison happens byte by byte (using ``cmp/zcmp/bzcmp/xzcmp``). This behavior can be overwritten by modifying the test file. +By default, the comparison happens byte by byte (using ``cmp/zcmp/bzcmp/xzcmp`` when available, otherwise a built-in Python fallback). This behavior can be overwritten by modifying the test file.tests/common.py (1)
140-146: Normalize path type for print_tree to avoid Path/str mixingWith dpath() and other call sites moving to Path objects, root.replace(path, "") can raise a TypeError if path is a Path. Normalize to a string early.
-def print_tree(path, exclude=None): - for root, _dirs, files in os.walk(path): +def print_tree(path, exclude=None): + path = os.fspath(path) + for root, _dirs, files in os.walk(path): if exclude and root.startswith(os.path.join(path, exclude)): continue level = root.replace(path, "").count(os.sep)src/snakemake/unit_tests/__init__.py (1)
108-110: Consider more robust config file discoveryUsing
Path().glob("config*")will search for config files in the current working directory, which may not be the intended behavior when the workflow is executed from a different directory.Consider using a more explicit path:
- copy_files(list(Path().glob("config*")), path / rulename / "config") + copy_files(list(Path(rundir).glob("config*")), path / rulename / "config")tests/test_generate_unit_tests/expected-results/.tests/units/test_a.py (1)
20-22: Verify that test paths exist before test executionThe test assumes these paths exist but doesn't validate them. If the paths don't exist,
shutil.copytreewill fail with a less helpful error message.Consider adding path validation:
config_path = Path(".tests/units/a/config") data_path = Path(".tests/units/a/data") expected_path = Path(".tests/units/a/expected") + + # Validate paths exist + assert config_path.exists(), f"Config path does not exist: {config_path}" + assert data_path.exists(), f"Data path does not exist: {data_path}" + assert expected_path.exists(), f"Expected path does not exist: {expected_path}"tests/test_generate_unit_tests/expected-results/.tests/units/common.py (1)
23-23: Remove or guard DEBUG print statementsThe DEBUG print statements on lines 23, 30, and 37 should either be removed or guarded with a debug flag for production use.
Consider using a debug flag:
+ debug = os.environ.get('SNAKEMAKE_UNIT_TEST_DEBUG', '').lower() == 'true' # ... existing code ... - print(f"input: {input_files}") # DEBUG + if debug: + print(f"input: {input_files}") # ... existing code ... - print(f"workdir: {workdir_files}") # DEBUG + if debug: + print(f"workdir: {workdir_files}") # ... existing code ... - print(f"expected: {expected_files}") # DEBUG + if debug: + print(f"expected: {expected_files}")Also applies to: 30-30, 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
tests/test_generate_unit_tests/.tests/integration/input.csvis excluded by!**/*.csvtests/test_generate_unit_tests/.tests/integration/test/0.tsvis excluded by!**/*.tsvtests/test_generate_unit_tests/.tests/integration/test/1.tsvis excluded by!**/*.tsvtests/test_generate_unit_tests/.tests/integration/test/2.tsvis excluded by!**/*.tsvtests/test_generate_unit_tests/expected-results/.tests/units/a/data/input.csvis excluded by!**/*.csvtests/test_generate_unit_tests/expected-results/.tests/units/b/expected/test/0.tsvis excluded by!**/*.tsv
📒 Files selected for processing (14)
docs/snakefiles/testing.rst(1 hunks)src/snakemake/unit_tests/__init__.py(3 hunks)src/snakemake/unit_tests/templates/common.py.jinja2(2 hunks)src/snakemake/unit_tests/templates/ruletest.py.jinja2(1 hunks)src/snakemake/workflow.py(5 hunks)tests/common.py(3 hunks)tests/test_generate_unit_tests/.tests/integration/config/config.json(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/a/config/config/config.json(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/b/config/config/config.json(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/common.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/conftest.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/test_a.py(1 hunks)tests/test_generate_unit_tests/expected-results/.tests/units/test_b.py(1 hunks)tests/tests.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/test_generate_unit_tests/.tests/integration/config/config.json
- tests/test_generate_unit_tests/expected-results/.tests/units/b/config/config/config.json
- tests/test_generate_unit_tests/expected-results/.tests/units/a/config/config/config.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake/workflow.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
tests/common.pytests/tests.pysrc/snakemake/unit_tests/__init__.py
🧠 Learnings (1)
📚 Learning: 2024-10-06T14:09:54.370Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Applied to files:
tests/test_generate_unit_tests/expected-results/.tests/units/test_b.pytests/common.pydocs/snakefiles/testing.rsttests/tests.pysrc/snakemake/unit_tests/templates/ruletest.py.jinja2
🧬 Code graph analysis (1)
tests/test_generate_unit_tests/expected-results/.tests/units/test_a.py (1)
src/snakemake/shell.py (1)
check_output(57-65)
🔇 Additional comments (3)
src/snakemake/unit_tests/templates/ruletest.py.jinja2 (1)
16-71: LGTM! Well-structured unit test generation templateThe template effectively generates comprehensive unit tests with proper:
- Conda prefix handling via pytest fixture
- Temporary workspace isolation
- Flexible target specification supporting multiple output files
- Software deployment method configuration
- Proper file comparison with support for compressed formats
The overall structure follows testing best practices with clear separation of setup, execution, and validation phases.
src/snakemake/unit_tests/__init__.py (2)
40-58: Validate path handling logic and ensure test coverage forcopy_filesA search across the test suite did not reveal any existing tests targeting the
copy_filesfunction insrc/snakemake/unit_tests/__init__.py, so the original concerns still stand:
- No unit tests found for
copy_files(absolute or relative paths)- The slice
str(parent)[len(root):]may yield incorrect results ifrootisn’t actually a prefix ofparent- There’s no handling for cases where the destination already contains a file or directory
Please review and address the following:
• Add targeted unit tests for
copy_filescovering:
– Absolute paths (e.g./foo/bar/baz.txt)
– Relative paths
– Nested directory structures
– Empty input (files = [])• Refactor the absolute‐path truncation logic to use
Path.relative_to()(or equivalent) to ensure correctness, rather than manual string slicing.• Define and document the behavior when the target path already exists (e.g. overwrite, skip, or error).
116-122: I’ve initiated a search to confirm whether theoverwrite_workdirfield is ever used when computingrundir. This will clarify ifrundircan be overridden or if it’s always set to the current working directory, ensuring our recommendations focus only on the remaining risk aroundPath.relative_to.
tests/test_generate_unit_tests/expected-results/.tests/units/test_a.py
Outdated
Show resolved
Hide resolved
tests/test_generate_unit_tests/expected-results/.tests/units/test_b.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
johanneskoester
approved these changes
Sep 4, 2025
johanneskoester
pushed a commit
that referenced
this pull request
Sep 5, 2025
🤖 I have created a release *beep* *boop* --- ## [9.11.0](v9.10.1...v9.11.0) (2025-09-05) ### Features * add landing page to the report containing custom metadata defined with a YTE yaml template ([#3452](#3452)) ([c754d80](c754d80)) * Issue a warning when unsupported characters are used in wildcard names ([#1587](#1587)) ([fa57355](fa57355)) ### Bug Fixes * avoid checking output files in immediate-submit mode ([#3569](#3569)) ([58c42cf](58c42cf)) * clarify --keep-going flag help text to distinguish runtime vs DAG construction errors ([#3705](#3705)) ([a3a8ef4](a3a8ef4)) * enable docstring assignment in `use rule ... with:` block ([#3710](#3710)) ([2191962](2191962)) * fix missing attribute error in greedy scheduler settings when using touch, dryrun or immediate submit. ([6471004](6471004)) * only trigger script with CODE label ([#3707](#3707)) ([2d01f8c](2d01f8c)) * parser.py incorrectly parsing multiline f-strings ([#3701](#3701)) ([06ece76](06ece76)) * parsing ordinary yaml strings ([#3506](#3506)) ([ddf334e](ddf334e)) * remove temp files when using checkpoints ([#3716](#3716)) ([5ff3e20](5ff3e20)) * Restore python rules changes triggering reruns. (GH: [#3213](#3213)) ([#3485](#3485)) ([2f663be](2f663be)) * unit testing ([#3680](#3680)) ([06ba7e6](06ba7e6)) * use queue_input_wait_time when updating queue input jobs ([#3712](#3712)) ([a945a19](a945a19)) ### Documentation * output files within output directories is an error ([#2848](#2848)) ([#2913](#2913)) ([37272e5](37272e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 tasks
kjohnsen
pushed a commit
to kjohnsen/snakemake
that referenced
this pull request
Dec 15, 2025
<!--Add a description of your PR here--> Fixes snakemake#843 - Add missing quotes to config file. - Include `snakefile` in the test. - Make config files realtive, so tests are portable - Support comparison for `gz` and `bz2` files - Changed the logic, in that it now compares expected files to the corresponding output (instead of the other way around). This allows to disable/delete specific expected files that will not be compared. - Fixed tests for Windows - Added optional `--conda-prefix` option to `pytest` to avoid installing all environments again if they are already available. - Also copy the `config/` folder (if it exists) to the temp workdir, since it is necessary in some workflows ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Enhanced unit-test generation: per-rule test directories, optional Snakefile/config support, multi-target test runs, copied inputs made read-only, and generated pytest conftest with conda-prefix fixture. - Workflow now exposes a runtime directory for consistent test context. - Tests - Tests migrated to pytest, generated in-place under .tests/unit and executed via python -m snakemake; removed one broad integration test and Windows skip. - Documentation - Testing guide updated with pytest examples, multi-tool comparison guidance (cmp/zcmp/bzcmp/xzcmp), and best practices for small, versioned test data. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kjohnsen
pushed a commit
to kjohnsen/snakemake
that referenced
this pull request
Dec 15, 2025
🤖 I have created a release *beep* *boop* --- ## [9.11.0](snakemake/snakemake@v9.10.1...v9.11.0) (2025-09-05) ### Features * add landing page to the report containing custom metadata defined with a YTE yaml template ([snakemake#3452](snakemake#3452)) ([c754d80](snakemake@c754d80)) * Issue a warning when unsupported characters are used in wildcard names ([snakemake#1587](snakemake#1587)) ([fa57355](snakemake@fa57355)) ### Bug Fixes * avoid checking output files in immediate-submit mode ([snakemake#3569](snakemake#3569)) ([58c42cf](snakemake@58c42cf)) * clarify --keep-going flag help text to distinguish runtime vs DAG construction errors ([snakemake#3705](snakemake#3705)) ([a3a8ef4](snakemake@a3a8ef4)) * enable docstring assignment in `use rule ... with:` block ([snakemake#3710](snakemake#3710)) ([2191962](snakemake@2191962)) * fix missing attribute error in greedy scheduler settings when using touch, dryrun or immediate submit. ([6471004](snakemake@6471004)) * only trigger script with CODE label ([snakemake#3707](snakemake#3707)) ([2d01f8c](snakemake@2d01f8c)) * parser.py incorrectly parsing multiline f-strings ([snakemake#3701](snakemake#3701)) ([06ece76](snakemake@06ece76)) * parsing ordinary yaml strings ([snakemake#3506](snakemake#3506)) ([ddf334e](snakemake@ddf334e)) * remove temp files when using checkpoints ([snakemake#3716](snakemake#3716)) ([5ff3e20](snakemake@5ff3e20)) * Restore python rules changes triggering reruns. (GH: [snakemake#3213](snakemake#3213)) ([snakemake#3485](snakemake#3485)) ([2f663be](snakemake@2f663be)) * unit testing ([snakemake#3680](snakemake#3680)) ([06ba7e6](snakemake@06ba7e6)) * use queue_input_wait_time when updating queue input jobs ([snakemake#3712](snakemake#3712)) ([a945a19](snakemake@a945a19)) ### Documentation * output files within output directories is an error ([snakemake#2848](snakemake#2848)) ([snakemake#2913](snakemake#2913)) ([37272e5](snakemake@37272e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #843
snakefilein the test.gzandbz2files--conda-prefixoption topytestto avoid installing all environments again if they are already available.config/folder (if it exists) to the temp workdir, since it is necessary in some workflowsQC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit
New Features
Tests
Documentation