Repository: widdowquinn/pyani

Issue:
MUMmer 4 reports version differently to MUMmer 3
#### Summary:
The regex used in `anim.py` to retrieve the version of `nucmer` installed fails with MUMmer v4.0.0rc1, as reported in Issue #325. This issue will theoretically be fixed when PR #276 is merged, but this is a reminder to specifically check that.

#### Current Output:
The output you get from `pyani` or at the terminal, with this issue. It is very useful to know the current "wrong" behavior on your system.

```python
Traceback (most recent call last):
File "~/anaconda3/bin/pyani", line 11, in
load_entry_point('pyani', 'console_scripts', 'pyani')()
File "~/pyani/pyani/scripts/pyani_script.py", line 117, in run_main
returnval = args.func(args)
File "~/pyani/pyani/scripts/subcommands/subcmd_anim.py", line 168, in subcmd_anim
nucmer_version = anim.get_version(args.nucmer_exe)
File "~/pyani/pyani/anim.py", line 110, in get_version
version = match.group() # type: ignore
AttributeError: 'NoneType' object has no attribute 'group'
```

#### pyani Version:

 0.3.0-alpha

Discussion:

Patch:
diff --git a/pyani/anib.py b/pyani/anib.py
index 51084ae..c948bac 100644
--- a/pyani/anib.py
+++ b/pyani/anib.py
@@ -98,9 +98,15 @@ from Bio import SeqIO  # type: ignore
 from . import pyani_config
 from . import pyani_files
 from . import pyani_jobs
+from . import PyaniException
 from .pyani_tools import ANIResults, BLASTcmds, BLASTexes, BLASTfunctions
 
 
+class PyaniANIbException(PyaniException):
+
+    """ANIb-specific exception for pyani."""
+
+
 def get_version(blast_exe: Path = pyani_config.BLASTN_DEFAULT) -> str:
     """Return BLAST+ blastn version as a string.
 
@@ -119,17 +125,20 @@ def get_version(blast_exe: Path = pyani_config.BLASTN_DEFAULT) -> str:
     The following circumstances are explicitly reported as strings
 
     - no executable at passed path
-    - non-executable file at passed path
+    - non-executable file at passed path (this includes cases where the user doesn't have execute permissions on the file)
     - no version info returned
     """
-    blastn_path = Path(shutil.which(blast_exe))  # type:ignore
 
-    if blastn_path is None:
+    try:
+        blastn_path = Path(shutil.which(blast_exe))  # type:ignore
+
+    except TypeError:
         return f"{blast_exe} is not found in $PATH"
 
     if not blastn_path.is_file():  # no executable
         return f"No blastn executable at {blastn_path}"
 
+    # This should catch cases when the file can't be executed by the user
     if not os.access(blastn_path, os.X_OK):  # file exists but not executable
         return f"blastn exists at {blastn_path} but not executable"
 
@@ -141,6 +150,7 @@ def get_version(blast_exe: Path = pyani_config.BLASTN_DEFAULT) -> str:
         stderr=subprocess.PIPE,
         check=True,
     )
+
     version = re.search(  # type: ignore
         r"(?<=blastn:\s)[0-9\.]*\+", str(result.stdout, "utf-8")
     ).group()
diff --git a/pyani/aniblastall.py b/pyani/aniblastall.py
index 70a2b29..32a6025 100644
--- a/pyani/aniblastall.py
+++ b/pyani/aniblastall.py
@@ -45,6 +45,12 @@ import subprocess
 from pathlib import Path
 
 from . import pyani_config
+from . import PyaniException
+
+
+class PyaniblastallException(PyaniException):
+
+    """ANIblastall-specific exception for pyani."""
 
 
 def get_version(blast_exe: Path = pyani_config.BLASTALL_DEFAULT) -> str:
@@ -65,20 +71,21 @@ def get_version(blast_exe: Path = pyani_config.BLASTALL_DEFAULT) -> str:
     The following circumstances are explicitly reported as strings
 
     - no executable at passed path
-    - non-executable file at passed path
+    - non-executable file at passed path (this includes cases where the user doesn't have execute permissions on the file)
     - no version info returned
     - executable cannot be run on this OS
     """
     logger = logging.getLogger(__name__)
 
-    blastall_path = Path(shutil.which(blast_exe))  # type:ignore
-
-    if blastall_path is None:
+    try:
+        blastall_path = Path(shutil.which(blast_exe))  # type:ignore
+    except TypeError:
         return f"{blast_exe} is not found in $PATH"
 
     if not blastall_path.is_file():  # no executable
         return f"No blastall at {blastall_path}"
 
+    # This should catch cases when the file can't be executed by the user
     if not os.access(blastall_path, os.X_OK):  # file exists but not executable
         return f"blastall exists at {blastall_path} but not executable"
 
@@ -95,13 +102,15 @@ def get_version(blast_exe: Path = pyani_config.BLASTALL_DEFAULT) -> str:
             stderr=subprocess.PIPE,
             check=False,  # blastall doesn't return 0
         )
-        version = re.search(  # type: ignore
-            r"(?<=blastall\s)[0-9\.]*", str(result.stderr, "utf-8")
-        ).group()
+
     except OSError:
         logger.warning("blastall executable will not run", exc_info=True)
         return f"blastall exists at {blastall_path} but could not be executed"
 
+    version = re.search(  # type: ignore
+        r"(?<=blastall\s)[0-9\.]*", str(result.stderr, "utf-8")
+    ).group()
+
     if 0 == len(version.strip()):
         return f"blastall exists at {blastall_path} but could not retrieve version"
 
diff --git a/pyani/anim.py b/pyani/anim.py
index de9662a..a638a04 100644
--- a/pyani/anim.py
+++ b/pyani/anim.py
@@ -107,25 +107,40 @@ def get_version(nucmer_exe: Path = pyani_config.NUCMER_DEFAULT) -> str:
     The following circumstances are explicitly reported as strings
 
     - no executable at passed path
-    - non-executable file at passed path
+    - non-executable file at passed path (this includes cases where the user doesn't have execute permissions on the file)
     - no version info returned
     """
-    nucmer_path = Path(shutil.which(nucmer_exe))  # type:ignore
 
-    if nucmer_path is None:
+    try:
+        nucmer_path = Path(shutil.which(nucmer_exe))  # type:ignore
+    except TypeError:
         return f"{nucmer_exe} is not found in $PATH"
 
     if not nucmer_path.is_file():  # no executable
         return f"No nucmer at {nucmer_path}"
 
+    # This should catch cases when the file can't be executed by the user
     if not os.access(nucmer_path, os.X_OK):  # file exists but not executable
         return f"nucmer exists at {nucmer_path} but not executable"
 
     cmdline = [nucmer_exe, "-V"]  # type: List
     result = subprocess.run(
-        cmdline, shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True
+        cmdline,
+        shell=False,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        check=True,
     )
-    match = re.search(r"(?<=version\s)[0-9\.]*", str(result.stderr, "utf-8"))
+
+    # version information appears in different places for
+    # different nucmer releases
+    if result.stderr:  # expected to work for <= MUMmer3
+        match = re.search(
+            r"(?<=version\s)[0-9\.]*", str(result.stderr + result.stdout, "utf-8")
+        )
+    elif result.stdout:  # expected to work for MUMmer4
+        match = re.search(r"[0-9a-z\.]*", str(result.stdout, "utf-8"))
+
     version = match.group()  # type: ignore
 
     if 0 == len(version.strip()):


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index e5bb1d5..8b87611 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -42,6 +42,7 @@ import subprocess
 import shutil
 import os
 import re
+import platform
 
 from pathlib import Path
 from typing import NamedTuple
@@ -273,6 +274,73 @@ def fragment_length():
     return FRAGSIZE
 
 
+@pytest.fixture
+def mock_get_nucmer_3_version(monkeypatch):
+    """Mock the output from NUCmer <= 3's version flag."""
+
+    def mock_which(*args, **kwargs):
+        """Mock an absolute file path."""
+        return args[0]
+
+    def mock_isfile(*args, **kwargs):
+        """Mock a call to `os.path.isfile()`."""
+        return True
+
+    def mock_access(*args, **kwargs):
+        """Mock a call to `os.access()`."""
+        return True
+
+    def mock_subprocess(*args, **kwargs):
+        """Mock a call to `subprocess.run()`."""
+        return MockProcess(
+            stdout=b"",
+            stderr=b"nucmer \nNUCmer (NUCleotide MUMmer) version 3.1\n    \n",
+        )
+
+    def mock_system(*args, **kwargs):
+        """Mock a call to `platform.system()`."""
+        return "Darwin"
+
+    monkeypatch.setattr(shutil, "which", mock_which)
+    monkeypatch.setattr(Path, "is_file", mock_isfile)
+    monkeypatch.setattr(os.path, "isfile", mock_isfile)
+    monkeypatch.setattr(os, "access", mock_access)
+    monkeypatch.setattr(subprocess, "run", mock_subprocess)
+    monkeypatch.setattr(platform, "system", mock_system)
+
+
+@pytest.fixture
+def mock_get_nucmer_4_version(monkeypatch):
+    """Mock the output from NUCmer 4's version flag."""
+
+    def mock_which(*args, **kwargs):
+        """Mock an absolute file path."""
+        return args[0]
+
+    def mock_isfile(*args, **kwargs):
+        """Mock a call to `os.path.isfile()`."""
+        return True
+
+    def mock_access(*args, **kwargs):
+        """Mock a call to `os.access()`."""
+        return True
+
+    def mock_subprocess(*args, **kwargs):
+        """Mock a call to `subprocess.run()`."""
+        return MockProcess(stdout=b"4.0.0rc1\n", stderr=b"")
+
+    def mock_system(*args, **kwargs):
+        """Mock a call to `platform.system()`."""
+        return "Darwin"
+
+    monkeypatch.setattr(shutil, "which", mock_which)
+    monkeypatch.setattr(Path, "is_file", mock_isfile)
+    monkeypatch.setattr(os.path, "isfile", mock_isfile)
+    monkeypatch.setattr(os, "access", mock_access)
+    monkeypatch.setattr(subprocess, "run", mock_subprocess)
+    monkeypatch.setattr(platform, "system", mock_system)
+
+
 @pytest.fixture
 def unsorted_genomes(dir_anim_in):
     """Tests ordering of genome names in output file names for asymmetric analyses."""
diff --git a/tests/test_anib.py b/tests/test_anib.py
index f95a725..2f8d238 100644
--- a/tests/test_anib.py
+++ b/tests/test_anib.py
@@ -77,6 +77,10 @@ class ANIbOutputDir(NamedTuple):
     legacyblastresult: pd.DataFrame
 
 
+# Create object for accessing unittest assertions
+assertions = unittest.TestCase("__init__")
+
+
 @pytest.fixture
 def anib_output(dir_anib_in):
     """Namedtuple of example ANIb output.
@@ -122,6 +126,14 @@ def anib_output_dir(dir_anib_in):
 
 
 # Test get_version()
+# Test case 0: no executable location is specified
+def test_get_version_nonetype():
+    """Test behaviour when no location for the executable is given."""
+    test_file_0 = None
+
+    assert anib.get_version(test_file_0) == f"{test_file_0} is not found in $PATH"
+
+
 # Test case 1: there is no executable
 def test_get_version_no_exe(executable_missing, monkeypatch):
     """Test behaviour when there is no file at the specified executable location."""
diff --git a/tests/test_aniblastall.py b/tests/test_aniblastall.py
index 68eb3ca..4bad424 100644
--- a/tests/test_aniblastall.py
+++ b/tests/test_aniblastall.py
@@ -43,11 +43,24 @@ pytest -v
 """
 
 from pathlib import Path
-
+import unittest
 from pyani import aniblastall
 
+# Create object for accessing unittest assertions
+assertions = unittest.TestCase("__init__")
+
 
 # Test get_version()
+# Test case 0: no executable location is specified
+def test_get_version_nonetype():
+    """Test behaviour when no location for the executable is given."""
+    test_file_0 = None
+
+    assert (
+        aniblastall.get_version(test_file_0) == f"{test_file_0} is not found in $PATH"
+    )
+
+
 # Test case 1: there is no executable
 def test_get_version_missing_exe(executable_missing):
     """Test behaviour when there is no file at the specified executable location."""
diff --git a/tests/test_anim.py b/tests/test_anim.py
index dc3c402..d3c4062 100644
--- a/tests/test_anim.py
+++ b/tests/test_anim.py
@@ -43,12 +43,13 @@ These tests are intended to be run from the repository root using:
 
 pytest -v
 """
-
+import sys
 from pathlib import Path
 from typing import List, NamedTuple, Tuple
 
 import pandas as pd
 import pytest
+import unittest
 
 from pandas.util.testing import assert_frame_equal
 
@@ -145,7 +146,19 @@ def mummer_cmds_four(path_file_four):
     )
 
 
+# Create object for accessing unittest assertions
+assertions = unittest.TestCase("__init__")
+
+
 # Test get_version()
+# Test case 0: no executable location is specified
+def test_get_version_nonetype():
+    """Test behaviour when no location for the executable is given."""
+    test_file_0 = None
+
+    assert anim.get_version(test_file_0) == f"{test_file_0} is not found in $PATH"
+
+
 # Test case 1: there is no executable
 def test_get_version_no_exe(executable_missing):
     """Test behaviour when there is no file at the specified executable location."""
@@ -173,6 +186,21 @@ def test_get_version_exe_no_version(executable_without_version):
     )
 
 
+# Test regex for different NUCmer versions
+def test_get_version_nucmer_3(mock_get_nucmer_3_version):
+    """Tests the regex that gets the version number for NUCmer <= 3."""
+    fake_nucmer_3 = Path("/fake/nucmer3/executable")
+
+    assert anim.get_version(fake_nucmer_3) == f"Darwin_3.1 ({fake_nucmer_3})"
+
+
+def test_get_version_nucmer_4(mock_get_nucmer_4_version):
+    """Tests the regex that gets the version number for NUCmer 4."""
+    fake_nucmer_4 = Path("/fake/nucmer4/executable")
+
+    assert anim.get_version(fake_nucmer_4) == f"Darwin_4.0.0rc1 ({fake_nucmer_4})"
+
+
 # Test .delta output file processing
 def test_deltadir_parsing(delta_output_dir):
     """Process test directory of .delta files into ANIResults."""