fix: Address PR review feedback

edmundmiller · edmundmiller · commit 1ddf9ce1092e · 2025-09-27T18:50:18.000-05:00
- Clean up AI-generated comments and verbose documentation - Improve version hash detection to handle both content and keys - Add test case for version hash in snapshot keys scenario - Update CHANGELOG.md with feature description - Preserve all existing pytest-workflow logic for separate PR Addresses feedback from @mashehu and @mirpedrol Related to: nf-core/modules#6505
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@
 
 - Support modules with `exec:` blocks ([#3633](https://github.com/nf-core/tools/pull/3633))
 - feat: nf-core modules bump-version supports specifying the toolkit ([#3608](https://github.com/nf-core/tools/pull/3608))
+- Lint for version captures in modules - detect hash format instead of actual YAML content ([#3676](https://github.com/nf-core/tools/pull/3676))
 
 ### Subworkflows
 
diff --git a/nf_core/modules/lint/module_tests.py b/nf_core/modules/lint/module_tests.py
@@ -16,25 +16,12 @@
 
 
 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
+    """Check if test content contains version information in hash format rather than actual YAML content."""
     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
+        r"\bversions\.yml:md5,[a-f0-9]{32}\b",
+        r"\bversions\.yml:sha[0-9]*,[a-f0-9]+\b",
     ]
 
-    # Convert to string only once and search efficiently
     content_str = str(test_content)
 
     for pattern in version_hash_patterns:
@@ -44,6 +31,22 @@ def _contains_version_hash(test_content):
     return False
 
 
+def _check_snapshot_for_version_hash(snap_content, test_name):
+    """Check both snapshot content and keys for version hash patterns."""
+    # Check test content for version hashes
+    if _contains_version_hash(snap_content[test_name]):
+        return True
+
+    # Check specific test's keys for version hashes
+    test_data = snap_content.get(test_name, {})
+    if isinstance(test_data, dict):
+        for key in test_data.keys():
+            if _contains_version_hash(str(key)):
+                return True
+
+    return False
+
+
 def module_tests(_, module: NFCoreComponent, allow_missing: bool = False):
     """
     Lint the tests of a module in ``nf-core/modules``
@@ -199,11 +202,8 @@ def module_tests(_, module: NFCoreComponent, allow_missing: bool = False):
                                             snap_file,
                                         )
                                     )
-                                    # Check if version content is actual content vs MD5/SHA hash
-                                    # Related to: https://github.com/nf-core/modules/issues/6505
-                                    # Ensures version snapshots contain actual content instead of hash values
-                                    if _contains_version_hash(snap_content[test_name]):
-                                        # Invalid - contains hash format
+                                    # Check if version content contains hash instead of actual YAML content
+                                    if _check_snapshot_for_version_hash(snap_content, test_name):
                                         module.failed.append(
                                             (
                                                 "test_snap_version_content",
@@ -212,7 +212,6 @@ def module_tests(_, module: NFCoreComponent, allow_missing: bool = False):
                                             )
                                         )
                                     else:
-                                        # Valid - either contains actual content or no version hash detected
                                         module.passed.append(
                                             (
                                                 "test_snap_version_content",
diff --git a/tests/modules/lint/test_module_tests.py b/tests/modules/lint/test_module_tests.py
@@ -171,7 +171,7 @@ def test_modules_empty_file_in_snapshot(self):
         """Test linting a nf-test module with an empty file sha sum in the test snapshot, which should make it fail (if it is not a stub)"""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
         snap["my test"]["content"][0]["0"] = "test:md5,d41d8cd98f00b204e9800998ecf8427e"
 
         with open(snap_file, "w") as fh:
@@ -184,15 +184,11 @@ def test_modules_empty_file_in_snapshot(self):
         assert len(module_lint.warned) >= 0
         assert module_lint.failed[0].lint_test == "test_snap_md5sum"
 
-        # reset the file
-        with open(snap_file, "w") as fh:
-            fh.write(content)
-
     def test_modules_empty_file_in_stub_snapshot(self):
         """Test linting a nf-test module with an empty file sha sum in the stub test snapshot, which should make it not fail"""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
         snap["my_test_stub"] = {"content": [{"0": "test:md5,d41d8cd98f00b204e9800998ecf8427e", "versions": {}}]}
 
         with open(snap_file, "w") as fh:
@@ -219,20 +215,12 @@ def test_modules_empty_file_in_stub_snapshot(self):
 
         assert found_test, "test_snap_md5sum not found in passed tests"
 
-        # 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_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
-        """
+        """Test linting a nf-test module with version information as MD5 hash instead of actual content, which should fail."""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        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"
@@ -249,22 +237,13 @@ def test_modules_version_snapshot_content_md5_hash(self):
         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
-        """
+        """Test linting a nf-test module with version information as actual content, which should pass."""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
 
         # Add a version entry with actual content (the new way that should pass)
         snap["my test"]["content"][0]["versions"] = {"ALE": {"ale": "20180904"}}
@@ -291,20 +270,12 @@ def test_modules_version_snapshot_content_valid(self):
         ]
         assert len(version_content_passed) > 0, "test_snap_version_content not found in passed tests"
 
-        # 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_sha_hash(self):
-        """Test linting a nf-test module with version information as SHA hash, which should fail.
-
-        Related to: https://github.com/nf-core/modules/issues/6505
-        Fixed in: https://github.com/nf-core/tools/pull/3676
-        """
+        """Test linting a nf-test module with version information as SHA hash, which should fail."""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
 
         # Add a version entry with SHA hash format (should be flagged)
         snap["my test"]["content"][0]["versions"] = (
@@ -323,73 +294,50 @@ def test_modules_version_snapshot_content_sha_hash(self):
             f"Expected 1 test_snap_version_content failure, got {len(version_content_failures)}"
         )
 
-        # 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_mixed_scenario(self):
-        """Test linting with mixed version content - some valid, some hash format.
-
-        Related to: https://github.com/nf-core/modules/issues/6505
-        Fixed in: https://github.com/nf-core/tools/pull/3676
-        """
+    def test_modules_version_snapshot_no_version_content(self):
+        """Test linting when no version information is present - should not trigger version content check."""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
 
-        # Create a scenario with multiple tests - one with hash, one with valid content
-        snap["test_with_hash"] = {"content": [{"versions": "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"}]}
-        snap["test_with_valid_content"] = {"content": [{"versions": {"BPIPE": {"bpipe": "0.9.11"}}}]}
+        # Remove version information entirely
+        if "content" in snap["my test"] and snap["my test"]["content"]:
+            snap["my test"]["content"][0].pop("versions", None)
 
         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 have failure for the hash test
+        # Should not have version content check failures when no version data present
         version_content_failures = [x for x in module_lint.failed if x.lint_test == "test_snap_version_content"]
-        assert len(version_content_failures) >= 1, "Expected at least 1 failure for hash format"
-
-        # Should have pass for the valid content test
-        version_content_passed = [
-            x
-            for x in module_lint.passed
-            if (hasattr(x, "lint_test") and x.lint_test == "test_snap_version_content")
-            or (isinstance(x, tuple) and len(x) > 0 and x[0] == "test_snap_version_content")
-        ]
-        assert len(version_content_passed) >= 1, "Expected at least 1 pass for valid content"
+        assert len(version_content_failures) == 0, "Should not have version content failures when no versions present"
 
-        # reset the file
-        with open(snap_file, "w") as fh:
-            fh.write(content)
+        # Should have test_snap_versions failure since no versions are present
+        version_failures = [x for x in module_lint.failed if x.lint_test == "test_snap_versions"]
+        assert len(version_failures) == 1, "Expected test_snap_versions failure when no versions present"
 
     @pytest.mark.issue("https://github.com/nf-core/modules/issues/6505")
-    def test_modules_version_snapshot_no_version_content(self):
-        """Test linting when no version information is present - should not trigger version content check.
-
-        Related to: https://github.com/nf-core/modules/issues/6505
-        Fixed in: https://github.com/nf-core/tools/pull/3676
-        """
+    def test_modules_version_snapshot_hash_in_keys(self):
+        """Test linting when version hash appears in snapshot keys rather than content."""
         snap_file = self.bpipe_test_module_path / "tests" / "main.nf.test.snap"
         snap = json.load(snap_file.open())
-        content = snap_file.read_text()
+        snap_file.read_text()
 
-        # Remove version information entirely
-        if "content" in snap["my test"] and snap["my test"]["content"]:
-            snap["my test"]["content"][0].pop("versions", None)
+        # Create a test where version hash appears in the test keys
+        snap["test_with_hash_key"] = {
+            "content": [{"versions.yml:md5,949da9c6297b613b50e24c421576f3f1": "some_value"}],
+            "versions": {"BPIPE": {"bpipe": "0.9.11"}},
+        }
 
         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 not have version content check failures when no version data present
+        # Should fail because version hash is in the keys
         version_content_failures = [x for x in module_lint.failed if x.lint_test == "test_snap_version_content"]
-        assert len(version_content_failures) == 0, "Should not have version content failures when no versions present"
-
-        # reset the file
-        with open(snap_file, "w") as fh:
-            fh.write(content)
+        assert len(version_content_failures) >= 1, "Expected failure for hash in keys"
