Refactored prelude and dcm2niix to avoid bug when not on PATH (#275)

* Refactored prelude and dcm2niix to avoid error

* Change conftest to reflect new changes

* Ad coverage of failed cases

* Better way

* Move dcm2niix to local bin

.github/workflows/tests.yml

@@ -58,7 +58,7 @@ jobs:
         run: |
           curl -JLO https://github.com/rordenlab/dcm2niix/releases/latest/download/dcm2niix_lnx.zip
           unzip -o dcm2niix_lnx.zip
-          sudo install dcm2nii* /usr/bin/
+          sudo install dcm2nii* /usr/local/bin/
 
       - name: Spinal Cord Toolbox clone
         run: |

conftest.py

@@ -39,17 +39,13 @@ def test_data_path_fixture():
 
 @pytest.fixture(params=[pytest.param(0, marks=pytest.mark.prelude)])
 def test_prelude_installation():
-    # note that check_prelude_installation() returns 0 on success, so it must
-    # be negated for the assertion.
-    assert not check_prelude_installation()
+    assert check_prelude_installation()
     return
 
 
 @pytest.fixture(params=[pytest.param(0, marks=pytest.mark.dcm2niix)])
 def test_dcm2niix_installation():
-    # note that check_dcm2niix_installation() returns 0 on success, so it must
-    # be negated for the assertion.
-    assert not check_dcm2niix_installation()
+    assert check_dcm2niix_installation()
     return
 
 

shimmingtoolbox/cli/check_env.py

@@ -57,28 +57,12 @@ def check_dependencies():
     # Prelude
     prelude_check_msg = check_name.format("prelude")
     print_line(prelude_check_msg)
-    # 0 indicates prelude is installed.
-    prelude_exit_code: int = check_prelude_installation()
-    # negating condition because 0 indicates prelude is installed.
-    if not prelude_exit_code:
-        print_ok()
-        print("    " + get_prelude_version().replace("\n", "\n    "))
-    else:
-        print_fail()
-        print(f"Error {prelude_exit_code}: prelude is not installed or not in your PATH.")
+    check_prelude_installation()
 
     # dcm2niix
     dcm2niix_check_msg = check_name.format("dcm2niix")
     print_line(dcm2niix_check_msg)
-    # 0 indicates dcm2niix is installed.
-    dcm2niix_exit_code: int = check_dcm2niix_installation()
-    # negating condition because 0 indicates dcm2niix is installed.
-    if not dcm2niix_exit_code:
-        print_ok()
-        print("    " + get_dcm2niix_version().replace("\n", "\n    "))
-    else:
-        print_fail()
-        print(f"Error {dcm2niix_exit_code}: dcm2niix is not installed or not in your PATH.")
+    check_dcm2niix_installation()
 
     # SCT
     sct_check_msg = check_name.format("Spinal Cord Toolbox")
@@ -88,43 +72,57 @@ def check_dependencies():
     return
 
 
-def check_prelude_installation() -> int:
+def check_prelude_installation():
     """Checks that ``prelude`` is installed.
 
-    This function calls ``which prelude`` and checks the exit code to verify
-    that ``prelude`` is installed.
+    This function calls ``which prelude`` and checks the exit code to verify that ``prelude`` is installed.
 
     Returns:
-        int: Exit code. 0 on success, nonzero on failure.
+        bool: True if prelude is installed, False if not.
     """
-
-    return subprocess.check_call(['which', 'prelude'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+    try:
+        subprocess.check_call(['which', 'prelude'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+    except subprocess.CalledProcessError as error:
+        print_fail()
+        print(f"Error {error.returncode}: prelude is not installed or not in your PATH.")
+        return False
+    else:
+        print_ok()
+        print("    " + get_prelude_version().replace("\n", "\n    "))
+        return True
 
 
-def check_dcm2niix_installation() -> int:
+def check_dcm2niix_installation():
     """Checks that ``dcm2niix`` is installed.
 
-    This function calls ``which dcm2niix`` and checks the exit code to verify
-    that ``dcm2niix`` is installed.
+    This function calls ``which dcm2niix`` and checks the exit code to verify that ``dcm2niix`` is installed.
 
     Returns:
-        int: Exit code. 0 on success, nonzero on failure.
+        bool: True if dcm2niix is installed, False if not.
     """
-    return subprocess.check_call(['which', 'dcm2niix'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+    try:
+        subprocess.check_call(['which', 'dcm2niix'], stdout=subprocess.DEVNULL,
+                              stderr=subprocess.DEVNULL)
+    except subprocess.CalledProcessError as error:
+        print_fail()
+        print(f"Error {error.returncode}: dcm2niix is not installed or not in your PATH.")
+        return False
+    else:
+        print_ok()
+        print("    " + get_dcm2niix_version().replace("\n", "\n    "))
+        return True
 
 
-def check_sct_installation() -> int:
+def check_sct_installation():
     """Checks that ``SCT`` is installed.
 
-    This function calls ``which sct_check_dependencies`` and checks the exit code to verify
-    that ``sct`` is installed.
+    This function calls ``which sct_check_dependencies`` and checks the exit code to verify that ``sct`` is installed.
 
     Returns:
         bool: True if sct is installed, False if not.
     """
     try:
-        subprocess.check_call(['which', 'sct_check_dependencies'], stdout=subprocess.DEVNULL,
-                              stderr=subprocess.DEVNULL)
+        subprocess.check_call(['which', 'sct_check_dependencies'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
     except subprocess.CalledProcessError as error:
         print_fail()
         print(f"Error {error.returncode}: Spinal Cord Toolbox is not installed or not in your PATH.")
@@ -153,7 +151,7 @@ def get_prelude_version() -> str:
     # we're capturing stderr instead of stdout
     help_output: str = prelude_help.stderr.rstrip()
     # remove beginning newline and drop help info to keep version info
-    version: str = help_output.split("\n\n")[0].replace("\n","", 1)
+    version: str = help_output.split("\n\n")[0].replace("\n", "", 1)
     return version
 
 

test/cli/test_check_env.py

@@ -1,9 +1,11 @@
 #!/usr/bin/env python
 # -*- coding: utf-8 -*-
-import pytest
 
+import pytest
 import re
 from click.testing import CliRunner
+import os
+
 import shimmingtoolbox.cli.check_env as st_ce
 
 
@@ -24,6 +26,16 @@ def test_dump_env_info():
     assert result.exit_code == 0
 
 
+def test_check_installation_errors():
+    """Tests that the function returns False as expected
+    """
+    runner = CliRunner(env={'PATH': '/usr/bin'})
+
+    result = runner.invoke(st_ce.check_dependencies, catch_exceptions=False)
+    assert result.exit_code == 0
+    assert result.stdout.count('FAIL') == 3
+
+
 def test_check_prelude_installation():
     """Tests that the function returns an exit code as expected, does not test
     the exit code value itself, so it does not depend on prelude being