4
\$\begingroup\$

I wrote a Python script to collect a list of directories containing third party libraries and submit them all to dependency-check, a program that compares library versions to known-vulnerable third party libraries. This code works correctly and I'm after feedback about Python programming, especially my testing methods.

Here are the tests:

from unittest import TestCase
import unittest
import run_check
import platform
import os

# requirements:
# platform independent
# start from base directory and drill down looking for jar files
# use base path given to create app_name
# launch 'dependency-check' with three arguments:  app_name, path, output file dir
# test that expected HTML files are generated


class TestDirectoryCheck(TestCase):
    def test_app_name(self):
        if platform.system() == "Windows":
            test_path = "count5\\some\\more"
            test_path2 = "count5\\some\\maybe\\"
            test_path3 = "C:\\myuser\\mydocs\\mylibs\\"
            self.assertEqual("count5-some-more", run_check.get_app_name(test_path))
            self.assertEqual("count5-some-maybe", run_check.get_app_name(test_path2))
            self.assertEqual("C-myuser-mydocs-mylibs", run_check.get_app_name(test_path3))
        else:
            test_path = "/srv/tomcata/webapps/myapp"
            other_path = "/count3/maybe/maybemore"
            self.assertEqual("srv-tomcata-webapps-myapp", run_check.get_app_name(test_path))
            self.assertEqual("count3-maybe-maybemore", run_check.get_app_name(other_path))


    def test_path_list(self):
        if platform.system() == "Windows":
            base_path = "C:\\Users\\manamana\\Downloads\\badjar\\count3"
            app_list = ["C:\\Users\\manamana\\Downloads\\badjar\\count3\\some",
                        "C:\\Users\\manamana\\Downloads\\badjar\\count3\\someothers",
                        "C:\\Users\\manamana\\Downloads\\badjar\\count3\\someothers\\more"
            ]


        else:
            base_path = "/home/mcgyver5/Desktop/badjar/count3"
            app_list = ["/home/mcgyver5/Desktop/badjar/count3/some",
                        "/home/mcgyver5/Desktop/badjar/count3/someothers",
                        "/home/mcgyver5/Desktop/badjar/count3/someothers/more"
            ]

        self.assertEqual(sorted(app_list), sorted(run_check.get_path_list(base_path)))


    def remove_old_files(self, test_dir=""):
        if platform.system() == "Windows":
            if test_dir == "":
                test_dir = "C:\\users\\manamana\\testme"

        else:
            if test_dir == "":
                test_dir = "/home/mcgyver5/Desktop/badjar/"
        for root, dirs, files in os.walk(test_dir):
            for f in files:
                if f.endswith("html"):
                    file_to_remove = os.path.join(root, f)
                    os.remove(file_to_remove)


    def test_dependency_check(self):
        self.remove_old_files()
        if platform.system() == "Windows":
            bin_path = "C:/Users/manamana/Downloads/dependency-check-1.2.7-release/bin"
            base_path = "C:/Users/manamana/testme"
            output_file = "C:/Users/manamana/testme/" + run_check.get_app_name(base_path) + ".html"
        else:
            bin_path = "/home/mcgyver5/Desktop/dependency-check-1.2.7-release/bin"
            base_path = "/home/mcgyver5/Desktop/badjar/count4"
            output_file = "/home/mcgyver5/Desktop/" + run_check.get_app_name(base_path) + ".html"

        app_list = run_check.get_path_list(base_path)
        results = run_check.dependency_check(bin_path, app_list, output_file)
        self.assertFalse("invalid" in results)
        self.assertFalse("usage" in results)

        # check that expected html files are present
        self.assertTrue(os.path.exists(output_file))

    def test_command_creation(self):
        if platform.system() == "Windows":
            bin_path = "C:/Users/manamana/Downloads/dependency-check-1.2.7-release/bin"
            base_path = "C:/Users/manamana/testme"
            output_file = "C:/Users/manamana/testme/" + run_check.get_app_name(base_path) + ".html"
            expected_command = "C:/Users/manamana/Downloads/dependency-check-1.2.7-release/bin\dependency-check.bat Core " \
                               "-a C-Users-manamana-testme -o C:/Users/manamana/testme/C-Users-manamana-testme.html " \
                               "-s C:/Users/manamana/testme " \
                               "-s C:/Users/manamana/testme\someothers " \
                               "-s C:/Users/manamana/testme\someothers\more"
        else:
            bin_path = "/home/mcgyver5/Desktop/dependency-check-1.2.7-release/bin"
            base_path = "/home/mcgyver5/Desktop/badjar/count4"
            output_file = "/home/mcgyver5/Desktop/" + run_check.get_app_name(base_path) + ".html"
            expected_command = "'/home/mcgyver5/Desktop/dependency-check-1.2.7-release/bin/dependency-check.sh Core " \
                               "-a home-mcgyver5-Desktop-badjar-count4-someothers " \
                               "-o /home/mcgyver5/Desktop/home-mcgyver5-Desktop-badjar-count4.html " \
                               "-s /home/mcgyver5/Desktop/badjar/count4/someothers " \
                               "-s /home/mcgyver5/Desktop/badjar/count4/someothers/more " \
                               "-s /home/mcgyver5/Desktop/badjar/count4/maybe/lib " \
                               "-s /home/mcgyver5/Desktop/badjar/count4/some'"

        app_list = run_check.get_path_list(base_path)
        self.assertEqual(expected_command, run_check.create_command(bin_path, app_list, output_file))


if __name__ == "__main__":
    unittest.main()

Here is the actual script:

import argparse
import subprocess
import platform
import os


def get_extension():
    if platform.system() == "Windows":
        return ".bat"
    else:
        return ".sh"

def get_path_list(base_path):
    app_list = []
    for root, dirs, files in os.walk(base_path):
        for f in files:
            if f.endswith(".jar"):
                app_list.append(root)
                break
        for d in dirs:
            get_path_list(d)
    return app_list


# "/srv/tomcata/webapps/myapp" will return srv-tomcata-webaps-myapp
def get_app_name(lib_path):
    lib_path = os.path.normpath(lib_path)
    lib_path = lib_path.replace(":", "")
    app_name = lib_path.replace(os.path.sep, "-")
    if app_name.startswith("-"):
        return app_name[1:]
    else:
        return app_name


def dependency_check(bin_path, app_list, output_file):
    command = create_command(bin_path, app_list, output_file)
    return subprocess.check_output(command, shell=True)


def create_command(bin_path, app_list, output_file):
    if len(app_list) > 0:
        full_path = os.path.join(bin_path, "dependency-check")
        app_name = get_app_name(app_list[0])
        command = "{0}{1} Core -a {2} -o {3}".format(full_path, get_extension(), app_name, output_file)
        for app_path in app_list:
            command += " -s {0}".format(app_path)
        return command


if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    bin_path = "C:/Users/manamana/Downloads/dependency-check-1.2.7-release/bin"
    parser.add_argument('--basepath', default='.', help="path to start recursive search for .jar files")
    parser.add_argument('-b', default=bin_path, help="path to shell script that calls dependency-check")
    parser.add_argument('-o', default='.', help="path to write output file (optional:file name)")
    args = parser.parse_args()
    appList = get_path_list(args.basepath)
    dependency_check(args.b, appList, args.o)
\$\endgroup\$
  • \$\begingroup\$ The code is functional and you just want the code reviewed? Or something else? I didn't see a question framed, hence confused. \$\endgroup\$ – Prashant Dec 31 '14 at 21:26
  • \$\begingroup\$ thanks for pointing that out. Yes, the code is functional and I'm after feedback about my python coding, especially in the testing methodology. I edited the question to reflect this. \$\endgroup\$ – mcgyver5 Dec 31 '14 at 21:30
4
\$\begingroup\$

get_path_lists uses recursion as so:

for d in dirs:
    get_path_list(d)

Namely, for every directory you do a full traversal of the whole tree: walk is already recursive! Or you would, if you actually passed in a full path os.path.join(root, d). You don't even use the result, so remove it.

Your

for f in files:
    if f.endswith(".jar"):
        app_list.append(root)
        break

can be simplified to

if any(f.endswith(".jar") for f in files):
    app_list.append(root)

I would convert get_path_list to an iterator:

def get_path_list(base_path):
    for root, dirs, files in os.walk(base_path):
        if any(f.endswith(".jar") for f in files):
            yield root

Success!

I would also consider a move to pathlib for its extended globbing:

def get_path_list(base_path):
    return set(path.parent for path in Path(base_path).glob("**/*.jar"))

although this comes with its own disadvantages.

get_app_name can be prettier more platform independent with pathlib.PurePath:

def get_app_name(lib_path):
    path = PurePath(lib_path)
    path = path.relative_to(path.anchor)

    return "-".join(path.parts)

The .relative_to stuff removes any drive or root, and then you can just join the parts.

In create_command you check if len(app_list) > 0; it's more idiomatic to just check if app_list.

You implicitly return None when empty; it seems to me you should be throwing an error. You should also use early exit to keep things cleaner.

Your formatting strings don't need numbers:

command = "{}{} Core -a {} -o {}".format(full_path, get_extension(), app_name, output_file)
for app_path in app_list:
    command += " -s {}".format(app_path)

Your for loop is non-idiomatic; you should never use += on immutable objects in a list without good reason. Here, just use:

command = "{}{} Core -a {} -o {}".format(full_path, get_extension(), app_name, output_file)
command += "".join(" -s {}".format(app_path) for app_path in app_list)

Even so, the real problem is that dependency_check uses shell=True; it shouldn't. Pass in a list instead:

command = [full_path + get_extension(), "Core", "-a", app_name, "-o", output_file]

for app_path in app_list:
    command.append("-s")
    command.append(app_path)

return command

The old method broke when any of the inputs had, say, spaces or asterisks in.

\$\endgroup\$
8
\$\begingroup\$
  • Ideally you should not have if conditions in your tests. You should always know exactly what it is that you want to test. test_app_name has a check on the platform. You should be able to mock it out and test the functionality under a 'windows' and a 'non-windows' environment. I'd split it out into different tests.

  • remove_old_files walks through a dir on your machine. Mock it out or include it in the repo for your code. Ideally mock it out if you don't really care about particulars of the files.

  • You have a lot of hardcoded paths in your code. This will not work on any other machine. You'll need some way of fixing this.

On a larger note:

You have the correct picture of a unit test, but you seem to try adn cover all cases under a single test. You need to test out all the methods, but you can split it out into multiple tests:

eg: Let's pick get_extension. There are 2 code paths in that method. So lets write 2 methods for it.

1. test_get_extension_windows
2. test_get_extension_non_windows

That should cover all cases for that method and we know exactly what gets executed in there. No need for conditions in our code. You can go on the same path for the other tests too.

\$\endgroup\$
3
\$\begingroup\$

For this code:

    if platform.system() == "Windows":
        test_path = "count5\\some\\more"
        test_path2 = "count5\\some\\maybe\\"
        test_path3 = "C:\\myuser\\mydocs\\mylibs\\"
        self.assertEqual("count5-some-more", run_check.get_app_name(test_path))
        self.assertEqual("count5-some-maybe", run_check.get_app_name(test_path2))
        self.assertEqual("C-myuser-mydocs-mylibs", run_check.get_app_name(test_path3))
    else:
        test_path = "/srv/tomcata/webapps/myapp"
        other_path = "/count3/maybe/maybemore"
        self.assertEqual("srv-tomcata-webapps-myapp", run_check.get_app_name(test_path))
        self.assertEqual("count3-maybe-maybemore", run_check.get_app_name(other_path))

you might actually want to use a dict.

    if platform.system() == "Windows":
        testPaths = {
            "count5\\some\\more": "count5-some-more",
            "count5\\some\\maybe\\": "cout5-some-maybe",
            "C:\\myuser\\mydocs\\mylibs\\": "C-myuser-mydocs-mylibs"
        }
    else:
        testPaths = {
            "/srv/tomcata/webapps/myapp": "srv-tomcata-webapps-myapp",
            "/count3/maybe/maybemore": "count3-maybe-maybemore"
        }
    for testPath, expectedAppName in testPaths.items():
        self.assertEqual(expectedAppName, run_check.get_app_name(testPath))

This communicates the intent better, which is to run the same check of input to output over a group of value pairs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.