-
Notifications
You must be signed in to change notification settings - Fork 218
Lint for version captures in modules #3676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 3 commits
a35f9bc
ff546d6
11bcdcb
1ddf9ce
2852207
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -15,6 +15,35 @@ | |||
| log = logging.getLogger(__name__) | ||||
|
|
||||
|
|
||||
| def _contains_version_hash(test_content): | ||||
| """ | ||||
| Check if test content contains version information in hash format. | ||||
|
|
||||
| Uses precise regex patterns to detect version hash formats while avoiding | ||||
| false positives from similar strings. | ||||
|
|
||||
| Args: | ||||
| test_content: Content of a single test from snapshot | ||||
|
|
||||
| Returns: | ||||
| bool: True if hash format detected, False otherwise | ||||
| """ | ||||
| # More precise regex patterns with proper boundaries | ||||
edmundmiller marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| version_hash_patterns = [ | ||||
| r"\bversions\.yml:md5,[a-f0-9]{32}\b", # Exact MD5 format (32 hex chars) | ||||
| r"\bversions\.yml:sha[0-9]*,[a-f0-9]+\b", # SHA format with variable length | ||||
| ] | ||||
|
|
||||
| # Convert to string only once and search efficiently | ||||
|
||||
| # Convert to string only once and search efficiently |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the previous if statement, looks like the version can be a key of snap_content, so we whould take this option into account too.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||
| import json | ||||||
| from pathlib import Path | ||||||
|
|
||||||
| import pytest | ||||||
| from git.repo import Repo | ||||||
|
|
||||||
| import nf_core.modules.lint | ||||||
|
|
@@ -134,12 +135,20 @@ def test_nftest_failing_linting(self): | |||||
| module_lint = nf_core.modules.lint.ModuleLint(directory=self.nfcore_modules) | ||||||
| module_lint.lint(print_results=False, module="kallisto/quant") | ||||||
|
|
||||||
| assert len(module_lint.failed) == 2, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" | ||||||
| assert len(module_lint.failed) == 4, f"Linting failed with {[x.__dict__ for x in module_lint.failed]}" | ||||||
| assert len(module_lint.passed) >= 0 | ||||||
| assert len(module_lint.warned) >= 0 | ||||||
| assert module_lint.failed[0].lint_test == "meta_yml_valid" | ||||||
| assert module_lint.failed[1].lint_test == "test_main_tags" | ||||||
| assert "kallisto/index" in module_lint.failed[1].message | ||||||
|
|
||||||
| # Check for expected failure types | ||||||
| failed_tests = [x.lint_test for x in module_lint.failed] | ||||||
| assert "meta_yml_valid" in failed_tests | ||||||
| assert "test_main_tags" in failed_tests | ||||||
| assert failed_tests.count("test_snap_version_content") == 2 # Should appear twice for the two version entries | ||||||
|
|
||||||
| # Check test_main_tags failure contains the expected message | ||||||
| main_tags_failures = [x for x in module_lint.failed if x.lint_test == "test_main_tags"] | ||||||
| assert len(main_tags_failures) == 1 | ||||||
| assert "kallisto/index" in main_tags_failures[0].message | ||||||
|
|
||||||
| def test_modules_absent_version(self): | ||||||
| """Test linting a nf-test module if the versions is absent in the snapshot file `""" | ||||||
|
|
@@ -213,3 +222,174 @@ def test_modules_empty_file_in_stub_snapshot(self): | |||||
| # reset the file | ||||||
| with open(snap_file, "w") as fh: | ||||||
| fh.write(content) | ||||||
|
|
||||||
| @pytest.mark.issue("https://github.com/nf-core/modules/issues/6505") | ||||||
edmundmiller marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| def test_modules_version_snapshot_content_md5_hash(self): | ||||||
| """Test linting a nf-test module with version information as MD5 hash instead of actual content, which should fail. | ||||||
|
|
||||||
| Related to: https://github.com/nf-core/modules/issues/6505 | ||||||
| Fixed in: https://github.com/nf-core/tools/pull/3676 | ||||||
edmundmiller marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| """ | ||||||
| snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap" | ||||||
| snap = json.load(snap_file.open()) | ||||||
| content = snap_file.read_text() | ||||||
|
|
||||||
| # Add a version entry with MD5 hash format (the old way that should be flagged) | ||||||
| snap["my test"]["content"][0]["versions"] = "versions.yml:md5,949da9c6297b613b50e24c421576f3f1" | ||||||
|
|
||||||
| with open(snap_file, "w") as fh: | ||||||
| json.dump(snap, fh) | ||||||
|
|
||||||
| module_lint = nf_core.modules.lint.ModuleLint(directory=self.nfcore_modules) | ||||||
| module_lint.lint(print_results=False, module="bpipe/test") | ||||||
|
|
||||||
| # Should fail because version is using MD5 hash instead of actual content | ||||||
| # Filter for only our specific test | ||||||
| version_content_failures = [x for x in module_lint.failed if x.lint_test == "test_snap_version_content"] | ||||||
| assert len(version_content_failures) == 1, ( | ||||||
| f"Expected 1 test_snap_version_content failure, got {len(version_content_failures)}" | ||||||
| ) | ||||||
| assert version_content_failures[0].lint_test == "test_snap_version_content" | ||||||
|
||||||
|
|
||||||
| # reset the file | ||||||
| with open(snap_file, "w") as fh: | ||||||
| fh.write(content) | ||||||
|
||||||
|
|
||||||
| @pytest.mark.issue("https://github.com/nf-core/modules/issues/6505") | ||||||
| def test_modules_version_snapshot_content_valid(self): | ||||||
| """Test linting a nf-test module with version information as actual content, which should pass. | ||||||
|
|
||||||
| Related to: https://github.com/nf-core/modules/issues/6505 | ||||||
| Fixed in: https://github.com/nf-core/tools/pull/3676 | ||||||
|
||||||
| Related to: https://github.com/nf-core/modules/issues/6505 | |
| Fixed in: https://github.com/nf-core/tools/pull/3676 |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before, not sure if this is needed
Uh oh!
There was an error while loading. Please reload this page.