Clean up tests and make them use temporary directories

Additionally removes the workaround of MdpFile.save returning
a path to an eventual backed-up file since it was an ugly
workaround to implement testing

MdpFile.search now returns number of items found after a
unit test was implemented to control for it

Adds more tests for comments and set_options

pygromacs/gmxfiles.py

@@ -64,7 +64,7 @@ def print(self, comment=True):
             """Print option as a line.
 
             Uses a standard MDP format. Use ``comment`` to print or ignore
-            a comment. Returns printed string.
+            a comment.
 
             """
 
@@ -75,7 +75,6 @@ def print(self, comment=True):
                 string += "; %s" % self.comment
             if self.parameter or comment:
                 print(string)
-            return string
 
     def get_option(self, parameter):
         """Return the value of a parameter.
@@ -104,7 +103,7 @@ def set_comment(self, parameter, comment=""):
             return None
 
         # Verify that comment is of good form
-        self.options[parameter].comment = comment.lstrip('; ')
+        self.options[parameter].comment = comment.lstrip(';').strip()
 
         return None
 
@@ -144,24 +143,31 @@ def remove_option(self, parameter):
             for option in self.lines[index:]:
                 option.index -= 1
 
-    def search(self, string):
+    def search(self, parameter):
         """Search for a parameter in the file.
 
         Prints any matching option and its value.
 
+        Returns:
+            int: Number of options found
+
         """
 
-        string = str(string).strip()
-        test = [self.print_option(option) for option in self.options.keys()
-                if option.find(string) != -1]
-        return test
+        query = str(parameter).strip().lower()
+
+        i = 0
+        for option in self.options.keys():
+            if option.lower().find(query) != -1:
+                self.print_option(option)
+                i += 1
+
+        return i
 
     def print_option(self, parameter):
         """Print a parameter, its value and comment."""
 
         if parameter in self.options.keys():
-            string = self.options[parameter].print()
-        return string
+            self.options[parameter].print()
 
     def print(self, comment=True):
         """Print the current file.
@@ -171,7 +177,8 @@ def print(self, comment=True):
 
         """
 
-        [option.print(comment) for option in self.lines]
+        for option in self.lines:
+            option.print(comment)
 
     def read(self, path):
         """Read an MDP file at ``path``.
@@ -203,7 +210,7 @@ def add_line(line, index):
             return option
 
         # Verify file extension
-        if not os.access(path, os.R_OK) and not path.endswith('.mdp'):
+        if (not os.access(path, os.F_OK)) and (not path.endswith('.mdp')):
             path += '.mdp'
 
         self.path = path
@@ -215,8 +222,8 @@ def add_line(line, index):
                         for index, line in enumerate(fp.readlines())]
 
         except FileNotFoundError:
+            self.path = ""
             print("could not open '%s' for reading" % self.path)
-            self.path = None
 
     def save(self, path="", verbose=True, ext='mdp'):
         """Save current MDP file.
@@ -228,29 +235,23 @@ def save(self, path="", verbose=True, ext='mdp'):
             verbose (bool): Print information about save
             ext (str): Use this file extension (by default 'mdp')
 
-        Returns:
-            str: The path to a file if one was backed up
-
         """
 
         if path == "":
             path = self.path
 
         # Verify file extension
         if not path.endswith(ext):
-            path = ''.join([path, '.', ext])
+            path = '.'.join([path, ext])
 
         # Verify path and backup collision
-        backup = verify_path(path, verbose)
-
-        if verbose:
-            print("Saving MDP file as '%s' ... " % path, end = "")
+        verify_path(path, verbose)
 
         # Actually save the file
         with open(path, 'w') as fp:
             with redirect_stdout(fp):
                 self.print()
+
         if verbose:
-            print("Done!")
+            print("Saved MDP file to '%s'." % path, end = "")
 
-        return backup

pygromacs/tests/test_mdp.py

@@ -1,14 +1,16 @@
-import filecmp
 import os
 import random
+import shutil
+import tempfile as tmp
 
 from pygromacs.gmxfiles import *
 
 path = 'pygromacs/tests/grompp.mdp'
+num_tests = 10 # number of parameters to modify
 
-def test_init():
+def test_empty_init():
     mdp = MdpFile()
-    assert (mdp.path == None)
+    assert (mdp.path == "")
 
 def test_read():
     mdp = MdpFile(path)
@@ -20,46 +22,77 @@ def test_read():
     mdp = MdpFile(noext)
     assert (mdp.path == path)
 
-    # Try reading bad file name
-    mdp = MdpFile(str(os.urandom(8)))
-    assert (mdp.path == None)
+    # Try reading bad files
+    with tmp.TemporaryDirectory() as tmp_dir:
+        not_a_file = os.path.join(tmp_dir, 'test.mdp')
+        mdp = MdpFile(not_a_file)
+        assert (mdp.path == "")
+
+    with tmp.NamedTemporaryFile() as tmp_file:
+        assert (os.access(tmp_file.name, os.F_OK) == True)
+        mdp = MdpFile(tmp_file.name)
+        assert (mdp.path == tmp_file.name)
+        assert (mdp.lines == [])
+
+        # Fill file with random data
+        tmp_file.file.write(os.urandom(128))
+        mdp = MdpFile(tmp_file.name)
+        assert (mdp.path == tmp_file.name)
+        assert (mdp.lines == [])
 
 def test_get_option():
     mdp = MdpFile(path)
     assert (mdp.get_option('nsteps') == '10000')
     assert (mdp.get_option('Tcoupl') == 'v-rescale')
-    assert (mdp.get_option('not-a-parameter') == None)
-    assert (mdp.get_option(10) == None)
+    assert (mdp.get_option('include') == '')
+    assert (mdp.get_option('not-a-parameter') == "")
+    assert (mdp.get_option(10) == "")
+    assert (mdp.get_option(True) == "")
 
 def test_set_option():
     mdp = MdpFile(path)
     mdp.set_option('nsteps', 25000)
     assert (mdp.get_option('nsteps') == '25000')
-    mdp.set_option('verlet-buffer-drift', 0.15, 'test-comment')
-    assert (mdp.get_option('verlet-buffer-drift') == '0.15')
-    assert (mdp.options['verlet-buffer-drift'].comment == 'test-comment')
-    assert (mdp.lines[-1] == mdp.options['verlet-buffer-drift'])
 
-def test_remove_option():
-    def verify_lines(test_lines, control_lines, index):
-        """Verify that indices of MdpFile.lines are adjusted."""
+    # Try setting an option not in list
+    mdp.set_option('not-a-parameter', 0.15, 'test-comment')
+    assert (mdp.get_option('not-a-parameter') == '0.15')
+    assert (mdp.options['not-a-parameter'].comment == 'test-comment')
+    assert (mdp.lines[-1] == mdp.options['not-a-parameter'])
+
+    # Try adding the same option again
+    length, index = len(mdp.lines), mdp.options['not-a-parameter'].index
+    mdp.set_option('not-a-parameter', 0.30, 'new-comment')
+    assert (mdp.get_option('not-a-parameter') == '0.3')
+    assert (mdp.options['not-a-parameter'].comment == 'new-comment')
+    assert (len(mdp.lines) == length)
+    assert (mdp.options['not-a-parameter'].index == index)
+
+    # Try modifying many random options
+    keys_change = random.sample(mdp.options.keys(), num_tests)
+    for parameter in keys_change:
+        value = str(random.random())
+        mdp.set_option(parameter, value)
+        assert (mdp.get_option(parameter) == value)
 
+def test_remove_option():
+    # Verify that indices of MdpFile.lines are adjusted
+    def verify_indices(test_lines, control_lines, index):
         for test, control in zip(test_lines[index:], control_lines[index+1:]):
             assert (test.parameter == control.parameter)
 
     mdp = MdpFile(path)
     length = len(mdp.lines)
 
-    num_test = 10
-    keys = random.sample(mdp.options.keys(), num_test)
+    keys = random.sample(mdp.options.keys(), num_tests)
     for parameter in keys:
         index = mdp.options[parameter].index
         copy_lines = mdp.lines.copy()
 
         # Verify that option is removed
         mdp.remove_option(parameter)
         assert (parameter not in mdp.options)
-        verify_lines(mdp.lines, copy_lines, index)
+        verify_indices(mdp.lines, copy_lines, index)
 
         # Try to remove again
         copy_options = mdp.options.copy()
@@ -68,80 +101,91 @@ def verify_lines(test_lines, control_lines, index):
         assert (copy_options == mdp.options)
         assert (copy_lines == mdp.lines)
 
-    assert (len(mdp.lines) == length-num_test)
+    assert (len(mdp.lines) == length - num_tests)
 
 def test_comment():
     mdp = MdpFile(path)
+
     assert (mdp.lines[0].comment == "")
     assert (mdp.lines[1].comment == "File 'mdout.mdp' was generated")
     assert (mdp.options['nsteps'].comment == '4 ns')
-    mdp.set_comment('nsteps', '4 ns run')
-    assert (mdp.options['nsteps'].comment == '4 ns run')
-    mdp.set_comment('nsteps', ' 4 ns with space')
-    assert (mdp.options['nsteps'].comment == '4 ns with space')
-    mdp.set_comment('nsteps', ';4 ns with semicolon')
-    assert (mdp.options['nsteps'].comment == '4 ns with semicolon')
-    mdp.set_comment('nsteps', '4 ns ; with several semicolon ;')
-    assert (mdp.options['nsteps'].comment == '4 ns ; with several semicolon ;')
+
+    # Try some different inputs for setting commment
+    mdp.set_comment('nsteps', '4 ns set  ')
+    assert (mdp.options['nsteps'].comment == '4 ns set')
+    mdp.set_comment('nsteps', '   4 ns initial space')
+    assert (mdp.options['nsteps'].comment == '4 ns initial space')
+    mdp.set_comment('nsteps', ';4 ns initial semicolon')
+    assert (mdp.options['nsteps'].comment == '4 ns initial semicolon')
+    mdp.set_comment('nsteps', '4 ns ; many semicolon ;')
+    assert (mdp.options['nsteps'].comment == '4 ns ; many semicolon ;')
+
+    # Try setting a comment to non-set parameter
     mdp.set_comment('not-a-parameter', 'really important parameter')
+    assert ('not-a-parameter' not in mdp.options.keys())
 
 def test_search():
     mdp = MdpFile(path)
-    assert (mdp.search('step', True) != [])
-    assert (mdp.search('step', False) != [])
-    assert (mdp.search('not-a-parameter') == [])
-    assert (mdp.search(10) == [])
+    assert (mdp.search('step') == 4)
+    assert (mdp.search('sTeP') == 4)
+    assert (mdp.search('not-a-parameter') == 0)
+    assert (mdp.search(10) == 0)
 
 def test_print():
     mdp = MdpFile(path)
     mdp.print(True)
     mdp.print(False)
 
 def test_save():
-    mdp = MdpFile(path)
-
-    # Save to same path
-    backup = mdp.save()
-    test_file = MdpFile(path)
-
-    # Control content of copy
-    for test, control in zip(test_file.lines, mdp.lines):
-        assert (test.parameter == control.parameter)
-        assert (test.value == control.value)
-        assert (test.comment.strip() == control.comment.strip())
-        assert (test.index == control.index)
-
-    # Return file to original state
-    os.rename(backup, mdp.path)
-
-    # Change options
-    num_test = 10
-    keys_remove = random.sample(mdp.options.keys(), num_test)
-    for parameter in keys_remove:
-        mdp.remove_option(parameter)
-
-    # Set options to random float
-    keys_change = random.sample(mdp.options.keys(), num_test)
-    rand = {}
-    for parameter in keys_change:
-        rand[parameter] = str(random.random())
-        mdp.set_option(parameter, rand[parameter])
-
-    # Save to new path and compare
-    directory, filename = os.path.split(path)
-    directory += '/test/'
-    newpath = directory + filename
-    backup = mdp.save(newpath.rsplit('.mdp')[0], True)
-    control = MdpFile(newpath)
-    for remove, change in zip(keys_remove, keys_change):
-        assert (remove not in control.options.keys())
-        assert (control.get_option(change) == rand[change])
-    os.remove(newpath)
-
-    # Save with custom extension
-    ext = 'new'
-    newpath = newpath.rsplit('.mdp')[0] + '.' + ext
-    backup = mdp.save(newpath, ext=ext)
-    assert (os.access(newpath, os.F_OK) == True)
-    os.remove(newpath)
-    os.rmdir(directory)
+    # Find a default backup path
+    def backup_path(path, i=1):
+        directory, filename = os.path.split(path)
+        backup_filename = ''.join(['#', filename, '.%d#' % i])
+        return os.path.join(directory, backup_filename)
+
+    # Copy file to temporary directory
+    with tmp.TemporaryDirectory() as tmp_dir:
+        _, filename = os.path.split(path)
+        tmp_path = os.path.join(tmp_dir, filename)
+        shutil.copyfile(path, tmp_path)
+
+        # Open file and try saving to own path
+        mdp = MdpFile(tmp_path)
+        mdp.save()
+        backup = backup_path(tmp_path)
+        assert (os.access(backup, os.F_OK) == True)
+
+        # Compare the two versions line by line
+        test = MdpFile(backup)
+        for line, control in zip(test.lines, mdp.lines):
+            assert (line.parameter == control.parameter)
+            assert (line.value == control.value)
+            assert (line.comment.strip() == control.comment.strip())
+            assert (line.index == control.index)
+
+        # Change some options
+        keys_remove = random.sample(mdp.options.keys(), num_tests)
+        for parameter in keys_remove:
+            mdp.remove_option(parameter)
+        keys_change = random.sample(mdp.options.keys(), num_tests)
+        values = {}
+        for parameter in keys_change:
+            values[parameter] = str(random.random())
+            mdp.set_option(parameter, values[parameter])
+
+        # Save to new path
+        new_filename = 'new'
+        new_path = os.path.join(tmp_dir, new_filename)
+        mdp.save(new_path, True)
+
+        # Verify that changes were written
+        control = MdpFile(new_path)
+        for remove, change in zip(keys_remove, keys_change):
+            assert (remove not in control.options.keys())
+            assert (control.get_option(change) == values[change])
+
+        # Try to save with a custom extension
+        ext = 'pdm'
+        ext_path = '.'.join([new_path, ext])
+        mdp.save(ext_path, ext=ext)
+        assert (os.access(ext_path, os.F_OK) == True)