fix: fix bug in code change detection leading to spurious code change reporting when relying on older snakemake metadata#3144
Conversation
… reporting when relying on older snakemake metadata
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
tests/tests.py (4)
267-272: LGTM! Consider usingassert_called_once_withfor more precise testing.The function has been appropriately renamed to reflect its focus on outdated metadata. The test logic looks correct, using a spy to check the behavior of
Persistence.has_outdated_metadata.For more precise testing, consider using
assert_called_once_with(True)instead of checkingspy.spy_return. This ensures that the method is called exactly once with the expected return value:assert spy.assert_called_once_with(True)🧰 Tools
🪛 Ruff
271-271: Avoid equality comparisons to
True; useif spy.spy_return:for truth checksReplace with
spy.spy_return(E712)
Line range hint
1089-1090: New test function added. Consider adding a TODO comment.The
test_expand_list_of_functionshas been added, which is good for outlining the test suite structure. However, it lacks implementation details.Consider adding a TODO comment to remind yourself or other developers to implement this test in the future:
def test_expand_list_of_functions(): # TODO: Implement test for expanding list of functions pass🧰 Tools
🪛 Ruff
271-271: Avoid equality comparisons to
True; useif spy.spy_return:for truth checksReplace with
spy.spy_return(E712)
Line range hint
1094-1095: New test function added. Consider adding a detailed TODO comment.The
test_scheduler_sequential_all_coresfunction has been added, which is good for outlining the test suite structure. However, it lacks implementation details.Consider adding a more detailed TODO comment to clarify the purpose of this test and guide future implementation:
def test_scheduler_sequential_all_cores(): # TODO: Implement test for scheduler behavior when using all cores sequentially # This test should verify that: # 1. The scheduler correctly allocates all available cores # 2. Jobs are executed in a sequential manner # 3. Core utilization is maximized throughout the workflow pass🧰 Tools
🪛 Ruff
271-271: Avoid equality comparisons to
True; useif spy.spy_return:for truth checksReplace with
spy.spy_return(E712)
Line range hint
1099-1104: LGTM! Consider adding a brief comment explaining the test's purpose.The
test_checkpoint_openfunction has been implemented with specific storage configuration, which is good for testing checkpoint functionality with storage integration.Consider adding a brief comment to explain the purpose of this test:
def test_checkpoint_open(): # Test checkpoint functionality with file system storage provider run( dpath("test_checkpoint_open"), default_storage_provider="fs", default_storage_prefix="storage", )This comment will help other developers quickly understand the test's purpose and configuration.
🧰 Tools
🪛 Ruff
271-271: Avoid equality comparisons to
True; useif spy.spy_return:for truth checksReplace with
spy.spy_return(E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- tests/test_params_outdated_code/Snakefile (0 hunks)
- tests/tests.py (1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_params_outdated_code/Snakefile
🧰 Additional context used
📓 Path-based instructions (1)
tests/tests.py (1)
Pattern
**/*.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.
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
Bug Fixes
New Features
Style