4
\$\begingroup\$

Before I get to the question, let me note that I'm a trained scientist, not a programmer; I've done my best to self-teach what I've needed to know so far, but in the interest of making my code usable for other people in my research group, I'd like to improve my code. I spent a weekend writing actual documentation for stuff, and the next thing on the list of making this more stable is unit testing, which I think I have a basic understanding of, but have never implemented in anything I've written.

I've developed a wrapper for paramiko that allows me load scientific data from a remote host. I was curious about how to best design a unit test for other scripts that rely on this function, or if one is even necessary? If there are any security holes or performance improvements, suggestions would be very appreciated!

def get_remote_data(filepath, time=False, info=False, copy_to_local=False):
    """
    A paramiko wrapper that gets file from a remote computer. Parses
    hostname from filepath. Works only for netcdf files!

    Keyword Arguments:
    filepath         -- the path of the file with hostname.
    time             -- print time required for loading(default False)
    info             -- print some information about the file after loading 
                        (default False)
    copy_to_local    -- copies file to local /tmp/remote_data/ and checks 
                        if this file already exists there (default False)

    Example:
    >>> get_remote_data('pgierz@rayo3:/csys/paleo2/pgierz/GR30s.nc')
    # TODO: Find out if this is the right way to do this, and make a unit test.
    Paul J. Gierz, Sat Feb 14 14:20:43 2015
    """
    # Import stuff from your own library:
    from UI_Stuff import print_colors
    if time:
        import time
    import paramiko
    import os
    from scipy.io import netcdf
    if time:
        now = time.time()
    print "Trying to load ".ljust(40) \
          +print_colors.WARNING("{f}").format(f=os.path.basename(filepath)).ljust(100)
    if not copy_to_local:
        # We wish to split the filepath to get the username, hostname, and
        # path on the remote machine.
        user = filepath.split(':')[0].split('@')[0]
        host = filepath.split(':')[0].split('@')[1]
        rfile = filepath.split(':')[1]
        # FIXME: This function only works if .ssh/id_rsa exists
        privatekeyfile = os.path.expanduser('~/.ssh/id_rsa')
        mykey = paramiko.RSAKey.from_private_key_file(privatekeyfile)
        client = paramiko.SSHClient()
        client.load_system_host_keys()
        client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
        client.connect(host, username=user, pkey=mykey)
        sftp = client.open_sftp()
        fileObject = sftp.file(rfile)
        file = netcdf.netcdf_file(fileObject)
        pre_s = "Loaded from "+host
    else:
        pre_s, fileObject = _copy_remote_file(file path)  # This is my own function that creates an rsync process
        file = netcdf.netcdf_file(fileObject)        
    if time:
        print pre_s.ljust(40) \
        +print_colors.OKGREEN("{filepath}").format(filepath=os.path.basename(filepath)).ljust(100) \
        +" in ".rjust(0) \
        +print_colors.OKBLUE("{time}").format(time=round(time.time()-now)) \
        +" seconds"
    if info:
        s = print_colors.HEADER("#"*30+" INFO of "+os.path.basename(filepath)+" "+"#"*30)
        print s
        print "Variables: \n"
        for k, v in file.variables.iteritems():
            print k, ":  dimensions -"+str(v.dimensions)+" shape - "+str(v.shape)
        print "Dimensions: \n"
        print file.dimensions
        print print_colors.HEADER("#"*len(s))
    return file
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

Single responsibility principle

This function does too many things:

  • maybe it copies a remote file
  • maybe it prints info

The function may also print timing stats, which probably doesn't make much sense when not copying the remote file.

Unit testing is most effective when testing small independent units that have a single responsibility, well-defined inputs and outputs, and ideally no side effects.

As a first step, it would be good to split this function to multiple smaller functions:

def get_remote_file(filepath, time=False):
    # ...

def get_local_file(filepath):
    # ...

def get_file(filepath, time=False, copy_to_local=False):
    if copy_to_local:
        return get_remote_file(filepath, time)
    return get_local_file(filepath)

def print_file_info(filepath, file):
    # ...

Each of these functions have well-defined, simpler responsibilities. The get_file function may seem to do multiple things, but it's ok, as it only orchestrates those things, it's more of a controller and has trivially simple implementation.

Now that you have smaller units, you can think about testing them individually, if necessary. Actually I don't see much to test here, because of the code is either printing, or making paramiko API calls. Testing that the paramiko calls work and you can fetch files will not be unit testing anymore, because it will require external factors correctly setup, such as a remote server with a file at a specific location, which would violate the principle of unit testing. That is not to say it's not worth testing, maybe it is, to test your setup, but it's not "unit testing", it's integration testing.

Code review

There is just one part that might be worth testing:

user = filepath.split(':')[0].split('@')[0]
host = filepath.split(':')[0].split('@')[1]
rfile = filepath.split(':')[1]

I suggest to put this in a function of its own:

def split_ssh_filepath(filepath):
    user = filepath.split(':')[0].split('@')[0]
    host = filepath.split(':')[0].split('@')[1]
    rfile = filepath.split(':')[1]
    return user, host, rfile

And eliminate the repeated calls to filepath.split:

def split_ssh_filepath(ssh_filepath):
    user_host_rfile = ssh_filepath.split(':')[:2]
    user, host = user_host_rfile[0].split('@')[:2]
    rfile = user_host_rfile[1]
    return user, host, rfile
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for taking the time to explain that to me! As perhaps a more general question, is it worthwhile to test things that are effectively just wrappers for other APIs? More often than not the work I do with python is loading in some data, visualizing it somehow, and saving the graphics, none of which requires me to "create new features", so to speak. \$\endgroup\$ Commented Feb 20, 2015 at 21:05
  • \$\begingroup\$ I agree with you, there's not a lot to unit test here (and I'm a big fan of unit testing). \$\endgroup\$ Commented Feb 20, 2015 at 21:08
3
\$\begingroup\$

The one small advantage to unit testing your wrapper might be that when you eventually upgrade versions of your wrapped thing, the unit tests might provide early warning that features you use changed in an incompatible way, or confidence that they still work.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.