feat: Allow to specify comparison command per-unit test#3956
feat: Allow to specify comparison command per-unit test#3956fgvieira merged 17 commits intosnakemake:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a public mapping for compression comparison commands and threads it through OutputChecker APIs; updates generated rule-test invocation to include an --allowed-rules filter and --show-failed-logs; updates several test template version strings and test-paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/snakemake/unit_tests/templates/common.py.jinja2`:
- Around line 51-52: The loop calling compare_files misses the new required
cmp_cmds argument and will raise a TypeError; update the call in the for f in
expected_files loop to pass the comparator commands (e.g., self.cmp_cmds) — for
example call self.compare_files(self.expected_path / f, self.workdir / f,
self.cmp_cmds) so it matches the updated compare_files signature.
🧹 Nitpick comments (1)
src/snakemake/unit_tests/templates/common.py.jinja2 (1)
54-58: Consider makingcmp_cmdsoptional with a default value.If the intent is to allow per-test customization while having sensible defaults, consider defaulting to the global
cmp_cmdsmapping. This prevents breakage if call sites omit the argument and aligns with the PR objective of allowing but not requiring custom comparison commands.♻️ Proposed refactor
- def compare_files(self, expected_file, generated_file, cmp_cmds): + def compare_files(self, expected_file, generated_file, cmp_cmds=cmp_cmds): check_output( cmp_cmds.get(expected_file.suffix, ["cmp"]) + [expected_file, generated_file] )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/snakemake/unit_tests/templates/common.py.jinja2`:
- Around line 52-58: The call to compare_files is missing the third required
argument cmp_cmds, causing a TypeError; update the call at the loop that uses
self.compare_files(self.expected_path / f, self.workdir / f) to pass the
appropriate cmp_cmds value (e.g., an available variable or an attribute like
self.cmp_cmds) so it matches the signature def compare_files(self,
expected_file, generated_file, cmp_cmds), or alternatively add a safe default to
the method signature (e.g., cmp_cmds=None and handle default inside
compare_files) so compare_files, expected_path, and workdir remain consistent.
🧹 Nitpick comments (1)
src/snakemake/unit_tests/templates/common.py.jinja2 (1)
23-23: Minor: PEP 8 style for default parameter.PEP 8 recommends no spaces around
=in keyword argument defaults.✏️ Suggested fix
- def check(self, cmp_cmds = cmp_cmds): + def check(self, cmp_cmds=cmp_cmds):
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/snakemake/unit_tests/templates/ruletest.py.jinja2`:
- Line 9: Remove the unused import by deleting the top-level "import pytest"
statement from the ruletest.py.jinja2 template; since pytest fixtures like
conda_prefix are provided automatically, there is no reference to pytest in the
template code, so remove the import to avoid an unused import warning.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/common.py (1)
47-56:⚠️ Potential issue | 🔴 CriticalUpdate expected output to match refactored template.
The template at
src/snakemake/unit_tests/templates/common.py.jinja2now uses acmp_cmdsdictionary mapping file extensions to comparison commands, but the expected-results file attests/test_generate_unit_tests/expected-results/.tests/unit/common.pystill contains the old hardcoded suffix checks. The expected output file must be updated to include thecmp_cmdsdictionary and refactored method signatures to match what the template generates:
- Add
cmp_cmdsdictionary (lines 9-13)- Update
check()signature to includecmp_cmds = cmp_cmdsparameter- Update
compare_files()to acceptcmp_cmdsparameter and usecmp_cmds.get()instead of if/elif chains- Update the call to
compare_files()at line 45 to passcmp_cmds
🧹 Nitpick comments (2)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (1)
20-22: Consider simplifying the path references.The path pattern
.tests/integration/../../.tests/unit/b/...effectively resolves to.tests/unit/b/...since going into.tests/integration/and then../..returns to the starting directory. If there's a specific reason for this indirection (e.g., test execution context), a brief comment explaining it would help maintainability.tests/test_generate_unit_tests/expected-results/.tests/unit/test_a.py (1)
20-22: Same path pattern observation as in test_b.py.The path pattern
.tests/integration/../../.tests/unit/a/...has the same unnecessary indirection noted in test_b.py. If this is intentional, consider adding a comment explaining the rationale to aid future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (1)
31-49:⚠️ Potential issue | 🟠 MajorMissing
--allowed-rulesargument — expected output doesn't match updated template.The template in
ruletest.py.jinja2now unconditionally emits--allowed-rulesand{{ ruletest.name }}(lines 48–49), but this expected output file does not include those arguments. This will cause the unit test generation test to fail when comparing the generated output against this expected result.The fix should add the two lines after
--target-files-omit-workdir-adjustment(line 43):Proposed fix
"-j1", "--target-files-omit-workdir-adjustment", + "--allowed-rules", + "b", "--configfile",
🤖 Fix all issues with AI agents
In `@src/snakemake/unit_tests/templates/ruletest.py.jinja2`:
- Around line 48-49: The template now always emits the "--allowed-rules" flag
and the rule name token "{{ ruletest.name }}", so update the expected output for
the unit test that verifies generation of test_b to include those two arguments
in the same position as the template (i.e., add --allowed-rules followed by the
rule name to the expected command line in the expected-results file for test_b);
ensure spacing and quoting match the template output so the test comparison
succeeds.
🧹 Nitpick comments (1)
tests/test_generate_unit_tests/expected-results/.tests/unit/test_b.py (1)
20-22: Roundabout paths resolve correctly but are unnecessarily verbose.The paths like
".tests/integration/../../.tests/unit/b/config"resolve to".tests/unit/b/config". While functionally correct, the intermediateintegration/../..traversal is confusing to read. If the path generation can be simplified upstream (in theRuleTestpath properties), that would improve readability of the generated tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/snakefiles/testing.rst`:
- Around line 30-31: Replace the inline code token `dict` in the sentence "the
user can pass a `dict` with the extensions and command to use" with
reStructuredText literal markup using double backticks (i.e., change `dict` →
``dict``) so the term is rendered as inline code in docs/snakefiles/testing.rst;
keep the surrounding sentence unchanged.
🤖 I have created a release *beep* *boop* --- ## [9.17.0](v9.16.3...v9.17.0) (2026-03-13) ### Features * Allow storing snakemake metadata in files or databases ([#4012](#4012)) ([dd75f31](dd75f31)) * Allow to specify comparison command per-unit test ([#3956](#3956)) ([b88171c](b88171c)) * job table orderd topological when run is started ([#4018](#4018)) ([75cf506](75cf506)) * lambda functions for priority in rules ([#3253](#3253)) ([d2aa226](d2aa226)) * Make on... directive of modules accessible ([#4050](#4050)) ([e9f2e1c](e9f2e1c)) ### Bug Fixes * adjust conda tests to not fail on apple silicon; fix [#4040](#4040) ([#4049](#4049)) ([f5b0142](f5b0142)) * allow "--containerize apptainer" to output apptainer format instead of dockerfile ([#4030](#4030)) ([f5cac30](f5cac30)) * apptainer command not recognized when singularity is absent ([#4010](#4010)) ([b8162e2](b8162e2)) * capture stderr when tests fail ([#3995](#3995)) ([97d74ba](97d74ba)) * **docs:** make Data-dependent conditional execution a complete example ([#4043](#4043)) ([3a1d7f2](3a1d7f2)) * don't build the DAG when running unlock. Fixes [#4000](#4000) and [#198](#198) ([#4007](#4007)) ([acf79fd](acf79fd)) * Ensure pixi tasks may be run as advertised ([#4046](#4046)) ([88253c2](88253c2)) * fix checkpoint handling corner cases ([#3870](#3870) and [#3559](#3559)) ([#4015](#4015)) ([63f4257](63f4257)) * issue 3642 ([#4054](#4054)) ([76e6fc2](76e6fc2)) * issue 3815 ([#4026](#4026)) ([b0eec96](b0eec96)) * logging None in shellcmd context causes error ([#4064](#4064)) ([d0652cd](d0652cd)) * lookup function returns default value for empty DataFrame queries ([#4056](#4056)) ([f71de97](f71de97)) * make `cache: omit-software` a rule specific property ([#4085](#4085)) ([034a9e7](034a9e7)) * reduce number of tests leaving temporary files behind ([#4033](#4033)) ([a3a1c97](a3a1c97)) * regression in dynamic resource handling ([#4038](#4038)) ([f2c554a](f2c554a)) * somewhat shorter announce message ([#4080](#4080)) ([57efc71](57efc71)) ### Performance Improvements * switch reretry with tenacity; decouple container classes (with Python 3.7 compat for old scripts) from rest of the codebase (enabling moving to newer python versions) ([#4032](#4032)) ([ffb19e7](ffb19e7)) ### Documentation * Add AI-assisted contributions policy to contributing guidelines ([#4051](#4051)) ([dd70526](dd70526)) * **codebase:** Update & simplify plugin architecture section ([#4052](#4052)) ([176cf63](176cf63)) * Correct workflow.source_path() description in documentation ([#4036](#4036)) ([45883c5](45883c5)) * fixed wrong code example for collect() function ([#4037](#4037)) ([5c85ed8](5c85ed8)) * Minor docs improvements ([#4089](#4089)) ([29ea226](29ea226)) * switch to sphinx_design for tabs ([#3976](#3976)) ([9674614](9674614)) * typo in the migration table breaking a pip install command ([#4024](#4024)) ([66f9dda](66f9dda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
QC
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
Improvements
Documentation
Updates