test: Improve logging tests #3848
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughAdded precise StrPath type and widespread type annotations in tests/common.py; introduced a JSONL-emitting Snakemake test logger plugin; added log-event counting/assertion helpers and refactored logging tests to use them. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Snakemake as Snakemake (emitter)
participant Handler as LogHandler
participant Stream as Output Stream (stdout/stderr/file)
Note right of Handler: initialized with settings (outfile, has_formatter, has_filter, needs_rulegraph)
Snakemake->>Handler: emit(record) [first call]
Handler->>Handler: detect first_emit -> build logger_info payload
Handler->>Stream: _emit({"event":"logger_info", "formatter":..., "filter":...})
Stream-->>Handler: write OK
Snakemake->>Handler: emit(record) [subsequent]
Handler->>Handler: build payload (event, level, formatted message)
Handler->>Stream: _emit(payload)
Stream-->>Handler: write OK
Handler->>Handler: close() (optional)
Handler->>Stream: close file if opened
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🧹 Nitpick comments (1)
tests/common.py (1)
70-75: Function names suggest boolean return but returnstr | None.
has_azbatch_account_urlandhas_zenodo_tokenreturn the actual values rather than booleans, which is inconsistent with thehas_*naming convention used by other functions in this file (e.g.,has_apptainer,has_conda). Consider renaming toget_*or keeping thehas_*pattern with boolean returns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (3)
tests/common.py(10 hunks)tests/logging/plugins/snakemake_logger_plugin_test/__init__.py(1 hunks)tests/test_logging.py(6 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:
tests/test_logging.pytests/logging/plugins/snakemake_logger_plugin_test/__init__.pytests/common.py
🧠 Learnings (3)
📚 Learning: 2025-01-13T18:50:40.791Z
Learnt from: cademirch
Repo: snakemake/snakemake PR: 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.
Applied to files:
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py
📚 Learning: 2024-10-08T17:41:54.542Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 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/common.py
📚 Learning: 2024-10-13T14:10:37.796Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the `snakemake/cli.py` file, `ExecutorPluginRegistry()` is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
Applied to files:
tests/common.py
🧬 Code graph analysis (1)
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py (3)
src/snakemake/persistence.py (1)
metadata(467-468)src/snakemake/gui.py (1)
record(102-106)src/snakemake/common/__init__.py (1)
write(421-422)
🪛 Ruff (0.14.6)
tests/test_logging.py
111-111: subprocess call with shell=True identified, security issue
(S602)
224-224: Unused function argument: capfd
(ARG001)
311-311: subprocess call: check for execution of untrusted input
(S603)
tests/common.py
57-57: Consider moving this statement to an else block
(TRY300)
142-142: Uses of tarfile.extractall()
(S202)
169-169: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
170-170: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
171-171: Do not perform function call os.environ.get in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ 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). (46)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (9, macos-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (9)
tests/test_logging.py (2)
27-37: LGTM!The helper function cleanly extracts and counts events from captured log records using Counter.
248-283: Well-designed parameterized test for the logger plugin.Good coverage of the plugin configuration options (stream output, formatter, filter, rulegraph). The test validates both the logger configuration metadata and the expected event counts for each scenario.
Also applies to: 338-354
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py (3)
23-48: LGTM!Clean dataclass-based settings definition with appropriate metadata for CLI help text.
71-96: LGTM!The emit/close lifecycle is properly implemented: logger info emitted on first record, proper JSON serialization, and file handle cleanup in
close().
98-120: LGTM!Properties correctly delegate to settings for plugin configuration.
tests/common.py (4)
34-41: LGTM!Good addition of the
StrPathtype alias for flexible path handling. The docstring clearly explains its purpose.
157-261: Comprehensive documentation and type annotations forrun().The added docstring with parameter descriptions and return type significantly improves usability. The explicit
Path | Nonereturn type correctly reflects the cleanup behavior.
298-299: LGTM!Using
os.fsdecode()correctly handles the case wheretmpdiris passed as aPathLikeobject, ensuring compatibility with the rest of the function that expects a string.
539-543: LGTM!Explicit return values (
Path(tmpdir)vsNone) clearly communicate the cleanup behavior to callers.
|
While waiting on @cademirch's thorough review, may I ask you to format your code with |
|
@johanneskoester Formatted the logger plugin file, which was being missed by the Pixi task: [tool.pixi.feature.style.tasks]
format = { cmd = "black docs/ apidocs/ src/ tests/*.py", description = "Format the code" }Maybe this should be changed from |
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 (1)
tests/logging/plugins/snakemake_logger_plugin_test/__init__.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:
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: Hocnonsense
Repo: snakemake/snakemake PR: 3714
File: tests/tests.py:1637-1644
Timestamp: 2025-09-08T12:39:09.146Z
Learning: In Snakemake module tests, targeting log-only files like "aaalog" can be an intentional test pattern to validate module behavior and error handling, even though log files aren't typically producible targets in normal workflows.
📚 Learning: 2025-01-13T18:50:40.791Z
Learnt from: cademirch
Repo: snakemake/snakemake PR: 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.
Applied to files:
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py
📚 Learning: 2025-01-17T12:00:09.368Z
Learnt from: leoschwarz
Repo: snakemake/snakemake PR: 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/logging/plugins/snakemake_logger_plugin_test/__init__.py
📚 Learning: 2024-08-13T16:22:58.430Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 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.
Applied to files:
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py
🔇 Additional comments (1)
tests/logging/plugins/snakemake_logger_plugin_test/__init__.py (1)
84-91: Verify the design choice to ignoreself.formatter.The code always uses
DEFAULT_FORMATTERto format records (line 88), even whenself.formatteris set by the logging manager. While the module docstring mentions this behavior ("formatter not used if set"), it's potentially confusing—why exposehas_formatterproperty and allow the formatter to be set if it's never actually used for formatting?Please confirm this is the intended design for the test plugin. If the formatter is truly not needed, consider documenting why this plugin differs from typical handlers.
| self.stream = sys.stderr | ||
| self.needs_close = False | ||
| else: | ||
| self.stream = open(self.settings.outfile, "w") |
There was a problem hiding this comment.
Specify UTF-8 encoding when opening the output file.
For cross-platform compatibility and to ensure consistent JSONL output, explicitly specify encoding='utf-8' when opening the file.
Apply this diff:
- self.stream = open(self.settings.outfile, "w")
+ self.stream = open(self.settings.outfile, "w", encoding="utf-8")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.stream = open(self.settings.outfile, "w") | |
| self.stream = open(self.settings.outfile, "w", encoding="utf-8") |
🤖 Prompt for AI Agents
In tests/logging/plugins/snakemake_logger_plugin_test/__init__.py around line 67
the test opens the output file without specifying encoding which can produce
inconsistent cross-platform JSONL; update the open call to include
encoding='utf-8' so the file is opened with explicit UTF-8 encoding to ensure
consistent output across platforms.
f0efc9d to
7df5205
Compare
|
Also rebased to fix merge conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_logging.py (1)
223-245: Remove unusedcapfdparameter fromtest_rule_failure.
capfdis injected but never used in this test, and Ruff still flags it (ARG001). Since you only inspectcaplogand structured events,capfdcan be dropped:-def test_rule_failure( - caplog: pytest.LogCaptureFixture, capfd: pytest.CaptureFixture[str] -): +def test_rule_failure(caplog: pytest.LogCaptureFixture):
🧹 Nitpick comments (2)
tests/test_logging.py (2)
27-75: Event counting helpers look solid; consider tightening theeventcheck.
count_events()/check_event_counts()nicely centralize log-event assertions and provide good diagnostics on failure. One minor robustness tweak: incount_events(),if event:will skip falsy-but-valid event values; usingif event is not None:would avoid surprises if a futureLogEventmember ever evaluates toFalse.
105-117: Avoidshell=Trueand unquoted Snakefile path intest_logging_config.Building a shell command via f-string (
f"snakemake -s {snakefile}") withshell=Trueis an anti-pattern that breaks if the path contains spaces or special characters and introduces unnecessary shell interpretation. Prefer passing an argument list without a shell:- snakefile = dpath("logging/test_logging_config") / "Snakefile" - p = sp.Popen( - f"snakemake -s {snakefile}", - shell=True, - stdout=sp.PIPE, - stderr=sp.PIPE, - cwd=tmp_path, - ) + snakefile = dpath("logging/test_logging_config") / "Snakefile" + p = sp.Popen( + ["snakemake", "-s", str(snakefile)], + stdout=sp.PIPE, + stderr=sp.PIPE, + cwd=tmp_path, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (3)
tests/common.py(11 hunks)tests/logging/plugins/snakemake_logger_plugin_test/__init__.py(1 hunks)tests/test_logging.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/logging/plugins/snakemake_logger_plugin_test/init.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/test_logging.pytests/common.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: Hocnonsense
Repo: snakemake/snakemake PR: 3714
File: tests/tests.py:1637-1644
Timestamp: 2025-09-08T12:39:09.146Z
Learning: In Snakemake module tests, targeting log-only files like "aaalog" can be an intentional test pattern to validate module behavior and error handling, even though log files aren't typically producible targets in normal workflows.
📚 Learning: 2024-10-08T17:41:54.542Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 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/common.py
📚 Learning: 2024-10-13T14:10:37.796Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the `snakemake/cli.py` file, `ExecutorPluginRegistry()` is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
Applied to files:
tests/common.py
🪛 Ruff (0.14.6)
tests/test_logging.py
111-111: subprocess call with shell=True identified, security issue
(S602)
224-224: Unused function argument: capfd
(ARG001)
314-314: subprocess call: check for execution of untrusted input
(S603)
tests/common.py
145-145: Uses of tarfile.extractall()
(S202)
172-172: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
173-173: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
174-174: Do not perform function call os.environ.get in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ 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). (46)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (6)
tests/test_logging.py (3)
165-181: Dry-run event assertions look well-factored.Using
count_events(caplog)andcheck_event_countshere makes the expectations explicit and reusable; the mapping of expected events for the dry run looks consistent with the intent of the test.
184-221: Good combination of structured and textual logging checks.This test nicely validates both structured
LogEventcounts viacheck_event_countsand human-readable stderr output viacapfd, which should catch regressions in either layer without duplicating logic.
248-357: Plugin test is comprehensive; small note on event mapping clarity.The parametrized plugin test covers the key handler configuration knobs and checks both the initial
logger_inforecord and subsequentLogEventcounts viacheck_event_counts, which is excellent. The conditional expectations forWORKFLOW_STARTED,DEBUG_DAG, andRULEGRAPHcorrectly encode the interaction between the default filter and plugin options, making this a strong regression test.tests/common.py (3)
21-89: StrPath alias and added annotations improve clarity without changing behavior.Introducing
StrPath = str | os.PathLikeand using it across helpers likedpath,md5sum,copy,get_expected_files,untar_folder, andprint_treealigns signatures with actual usage and keeps call sites flexible. The implementations already worked withos.PathLike, so these are type-level improvements only.
125-158: Path-like handling in filesystem helpers is coherent.Allowing
StrPathforcopy,get_expected_files,untar_folder, andexcludeinprint_treefits how these functions are used elsewhere (e.g., withPathinputs) and leveragesos.fspathin stdlib APIs, so this should be safe while improving typing.
160-229: I'll help you verify the concerns in this review comment. Let me start by searching for call sites of therun()function and how its return value is being used.
<function_calls>
#!/bin/bashFind usages of run() return value in tests
Look for patterns where run() is called and the result is assigned or used
rg -n "=\s*run(" tests -A 5 -B 2
</function_calls>
#!/bin/bashAlso search for direct usage of run() return value without assignment
rg -n "run([^)]*)." tests -C 3
</function_calls>
#!/bin/bashCheck the actual implementation of the run() function at the mentioned lines
sed -n '160,240p' tests/common.py | head -100
</function_calls>
#!/bin/bashLook at what the function returns and how tmpdir is used
sed -n '160,330p' tests/common.py
</function_calls>
|
Also removed unused test fixture as per CodeRabbit suggestion. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_logging.py (4)
27-42: Confirm builtin-generic annotations match supported Python versionsThe annotations
dict[LogEvent, int]anddict[LogEvent, int | None]require Python ≥3.9 unlessfrom __future__ import annotationsis enabled; on 3.8 these would raise at import time. If Snakemake still supports 3.8, consider adding postponed-annotations at the top of this module or switching toDict[...]fromtyping; otherwise, this is fine but should be consistent with the declared minimum Python version.+from __future__ import annotations + import os import shutil import sys import subprocess as sp import logging from collections import Counter from pathlib import Path import json
70-73: Make debug printing robust to non-orderableLogEventenumsIf
LogEventdoes not define ordering,sorted(observed)can raise aTypeErroron some Python versions. Sorting by a string representation avoids depending on enum ordering while keeping the output readable.- for event in sorted(observed): + for event in sorted(observed, key=str): print(f" {event}: {observed[event]}", file=sys.stderr)
105-116: Avoidshell=Trueand string command to make Snakemake invocation more robust
Popen(f"snakemake -s {snakefile}", shell=True, ...)is unnecessary here and becomes brittle if the Snakefile path ever contains spaces or shell metacharacters. Building an argument list and lettingPopenhandle it directly avoids quoting issues and sidesteps theshell=Truestatic-analysis warning.- snakefile = dpath("logging/test_logging_config") / "Snakefile" - p = sp.Popen( - f"snakemake -s {snakefile}", - shell=True, - stdout=sp.PIPE, - stderr=sp.PIPE, - cwd=tmp_path, - ) + snakefile = dpath("logging/test_logging_config") / "Snakefile" + p = sp.Popen( + ["snakemake", "-s", str(snakefile)], + stdout=sp.PIPE, + stderr=sp.PIPE, + cwd=tmp_path, + )
246-355: Consider preserving existingPYTHONPATHwhen injecting the plugin directoryIn
test_plugin,env = dict(os.environ)is good for retaining the base environment, butenv["PYTHONPATH"] = str(plugin_dir)overwrites any existingPYTHONPATH. That’s fine for isolated CI, but if tests run in environments that already rely onPYTHONPATH, this could subtly change imports. To be more robust, you could prefix the plugin directory to the existing value instead of replacing it.- env = dict(os.environ) - env["PYTHONPATH"] = str(plugin_dir) + env = dict(os.environ) + existing = env.get("PYTHONPATH") + env["PYTHONPATH"] = ( + str(plugin_dir) + if not existing + else os.pathsep.join([str(plugin_dir), existing]) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_logging.py(6 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:
tests/test_logging.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: Hocnonsense
Repo: snakemake/snakemake PR: 3714
File: tests/tests.py:1637-1644
Timestamp: 2025-09-08T12:39:09.146Z
Learning: In Snakemake module tests, targeting log-only files like "aaalog" can be an intentional test pattern to validate module behavior and error handling, even though log files aren't typically producible targets in normal workflows.
🧬 Code graph analysis (1)
tests/test_logging.py (2)
tests/common.py (1)
dpath(38-41)tests/logging/plugins/snakemake_logger_plugin_test/__init__.py (2)
has_formatter(116-117)needs_rulegraph(120-121)
🪛 Ruff (0.14.6)
tests/test_logging.py
111-111: subprocess call with shell=True identified, security issue
(S602)
312-312: subprocess call: check for execution of untrusted input
(S603)
⏰ 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). (46)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, macos-latest, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (9, macos-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: apidocs
🔇 Additional comments (3)
tests/test_logging.py (3)
165-181: Dry-run event-count test cleanly uses the new helpersThe structure of
test_log_events_dryrun—capturing atINFO, then delegating counting and assertions tocount_events/check_event_counts—looks good and keeps the expectations explicit without duplicating logic.
184-206: Workflow run event expectations and stderr checks look consistentThe combination of event-count assertions (including
JOB_STARTED: Nonefor “at least one”) with explicit stderr content checks gives good coverage of both structured logging and human-readable output; nothing stands out as problematic here.
223-243: Failure-path event coverage for rule errors is thorough
test_rule_failureexercises the failing-workflow path and asserts a detailed set ofLogEventcounts (including bothERRORandJOB_ERRORplusJOB_STARTEDas “at least one”), which should catch regressions in how failures are logged.
|
CodeRabbit keeps coming up with good suggestions. Extended |
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 (1)
tests/test_logging.py(6 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:
tests/test_logging.py
🧬 Code graph analysis (1)
tests/test_logging.py (1)
src/snakemake/shell.py (2)
shell(43-371)executable(68-79)
🪛 Ruff (0.14.6)
tests/test_logging.py
111-111: subprocess call with shell=True identified, security issue
(S602)
318-318: subprocess call: check for execution of untrusted input
(S603)
⏰ 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). (46)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (8, 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 (6, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: apidocs
🔇 Additional comments (8)
tests/test_logging.py (8)
27-37: LGTM! Clean helper implementation.The
count_eventsfunction correctly tallies log records by event type using Counter and properly handles records that may not have an event attribute.
40-76: LGTM! Well-designed validation helper.The
check_event_countsfunction provides clear validation with helpful error messages and debug output. The pattern of usingNoneto indicate "at least one occurrence" is intuitive and flexible.
105-123: LGTM! Good test isolation improvement.The modifications properly isolate the test by running in a temporary directory and add clear documentation. The
shell=Trueflag is acceptable here since the command uses known, controlled paths in a test context.
125-129: LGTM! Clear documentation added.The docstring clearly explains the test purpose and references the relevant issue.
165-182: LGTM! Clean refactoring using new helpers.The test has been successfully refactored to use the new helper functions, reducing code duplication while maintaining clear assertions for dry run event counts.
184-221: LGTM! Comprehensive test with good coverage.The test properly validates both event counts and stderr output. The use of
NoneforJOB_STARTEDcorrectly handles variable job grouping behavior.
223-244: LGTM! Complete failure scenario test.The test properly validates event counts for a failing workflow, including appropriate expectations for ERROR and JOB_ERROR events.
246-281: Well-designed comprehensive plugin test.The parametrized test thoroughly covers different plugin configurations. The logic for conditional event count expectations based on plugin properties (
has_filter,needs_rulegraph) is correct. The subprocess call flagged by static analysis (S603) is acceptable in this test context with controlled inputs.Also applies to: 296-361
5b08bb0 to
790058a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_logging.py (1)
111-117: Consider avoidingshell=Truefor better robustness.While acceptable in test code with controlled inputs, using
shell=Truewith string formatting can be fragile. Consider refactoring to use a list of arguments instead.Apply this diff to improve robustness:
- p = sp.Popen( - f"snakemake -s {snakefile}", - shell=True, - stdout=sp.PIPE, - stderr=sp.PIPE, - cwd=tmp_path, - ) + p = sp.Popen( + ["snakemake", "-s", str(snakefile)], + stdout=sp.PIPE, + stderr=sp.PIPE, + cwd=tmp_path, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_logging.py(6 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:
tests/test_logging.py
🪛 Ruff (0.14.6)
tests/test_logging.py
111-111: subprocess call with shell=True identified, security issue
(S602)
318-318: subprocess call: check for execution of untrusted input
(S603)
⏰ 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). (46)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, macos-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (1)
tests/test_logging.py (1)
318-325: Static analysis warning is a false positive.The subprocess call is safe in this test context—all command arguments are constructed from controlled values (
sys.executable, test paths fromdpath()). The S603 warning can be safely ignored here.
|
@cademirch Looks like the |
|
@cademirch looks like your edit fixed it, thanks! |
- Edits to `common` test module:
- Added type annotations to many functions
- Edits to `run()` function:
- Documented and added type annotations for many arguments.
- Allow `path` and `tmpdir` to be either string or `os.Pathlike`.
- Removed unused `subpath` argument.
- Updates to existing logging tests:
- Added file to `test-all` Pixi task, I don't think it was actually
being run before.
- Added some helper functions to deduplicate code relating to counting
log events.
- Don't count exact number of `LOG_COUNTED` events (multiple jobs can be
grouped under a single event), just assert at least one exists.
- Completed `test_rule_failure` test, add expected log counts.
- Run workflow in `test_logging_config` in temporary directory instead
of current directory (this maybe should be using the same `run()`
function as all the others, but isn't).
- Added some more descriptive docstrings and type annotations to test
functions.
- Added new test for logging plugin.
An issue I noticed that existed before this PR and I did not change: the
`benchmark_extended` argument of the `run()` test function is used in
some tests but its value is ignored in the function body.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Tests**
* Added centralized helpers to count and validate log events and
refactored tests to use them.
* Added a test logging plugin that emits structured JSONL events
(including an initial logger_info record) for improved log capture and
verification.
* **Chores**
* Introduced comprehensive type annotations and a new string/path type
alias across test utilities for clearer typing and maintainability.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Cade Mirchandani <cmirchan@ucsc.edu>
commontest module:run()function:pathandtmpdirto be either string oros.Pathlike.subpathargument.test-allPixi task, I don't think it was actually being run before.LOG_COUNTEDevents (multiple jobs can be grouped under a single event), just assert at least one exists.test_rule_failuretest, add expected log counts.test_logging_configin temporary directory instead of current directory (this maybe should be using the samerun()function as all the others, but isn't).An issue I noticed that existed before this PR and I did not change: the
benchmark_extendedargument of therun()test function is used in some tests but its value is ignored in the function body.Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.