File validation and unit testing

Question

I just started a new project which reads in a video file and splits it up into frames. I started by just throwing everything in the GUI, but decided I wanted to make this a more well designed program, backed with unit tests. So I started to split out functionality into its own class and have a few questions on best practice.

from os import path VALID_EXTENTIONS = ["avi", "mp4"] class InvalidVideoSourceException(Exception): pass def isVideoSrcValid(src): if (not path.isfile(src)): return False filenameTokens = path.splitext(src) if (len(filenameTokens) < 1): return False if (not filenameTokens[1][1:].lower() in VALID_EXTENTIONS): return False return True class VideoCapture(object): def __init__(self, videoSrc): if (not isVideoSrcValid(videoSrc)): raise InvalidVideoSourceException("Invalid video file.") self.videoSrc = videoSrc 

I'm setting up my capture class which will end up handling all the interaction with the video-- skipping to a frame, getting meta data, etc. Pretty simple.

My concern is the best way to handle validation of the path. The class is essentially useless until it has a valid video file, so you must provide that to __init__. If the path is invalid for whatever reason, then the class should throw an exception.

Originally, I had the isVideoSrcValid function within the class and it returned nothing, it would go through its validation and throw an exception as it got to it. This was nice because VALID_EXTENTIONS belonged to the class which I like since it doesn't feel good having a global variable hanging around like that. I could pass it into the function, but I like that even less, since it is constant data. The other advantage of this approach was having precise error messages. If the file was invalid because the extension isn't supported, it would report that.

The downside, and why I ultimately choose to return a Boolean from the function rather than throwing exceptions inline, was so that if down the line I want to handle invalid files in a different way, such as throwing up a dialog, this would make it very easy to do so without messy exception handling. This is why I now check to see if the path is valid, and then raise a general exception.

I choose to pull the isVideoSrcValid function out of the class because it isn't closely related semantically to the VideoCapture class. There may be other times when I'd like to check to see if a file is a valid video file without constructing a VideoCapture class. This also makes it much easier to unit test.

Have I made the correct choices here? Is there a better way of doing things? I suppose the third option which I didn't consider is to create a Video class, which when constructed represents a valid video. Then the VideoCapture class would take in a Video rather than a path.

Finally, I'm new to unit testing in Python. Here is my unit tests for isVideoSrcValid. Please let me know if you'd do anything differently.

from capped import VideoCapture as vc class TestValidStream(unittest.TestCase): def setUp(self): open("testFile.avi", 'a').close() open("testFile.txt", 'a').close() open("testFile.avi.txt", 'a').close() open("testFile.abc.avi", 'a').close() open("testFile", 'a').close() def test_empty(self): self.assertFalse(vc.isVideoSrcValid("")) def test_validFile(self): self.assertTrue(vc.isVideoSrcValid("testFile.avi")) def test_noFileExists(self): self.assertFalse(vc.isVideoSrcValid("abcdeg.avi")) def test_noFileExtension(self): self.assertTrue(os.path.isfile("testFile")) self.assertFalse(vc.isVideoSrcValid("testFile")) def test_invalidFileExtension(self): self.assertTrue(os.path.isfile("testFile.txt")) self.assertFalse(vc.isVideoSrcValid("testFile.txt")) def test_invalidDoubleFileExtension(self): self.assertTrue(os.path.isfile("testFile.avi.txt")) self.assertFalse(vc.isVideoSrcValid("testFile.avi.txt")) def test_validDoubleFileExtension(self): self.assertTrue(vc.isVideoSrcValid("testFile.abc.avi")) def tearDown(self): os.remove("testFile.avi") os.remove("testFile.txt") os.remove("testFile.avi.txt") os.remove("testFile.abc.avi") os.remove("testFile") 

Answer 1

As a general statement, Python convention uses underscores_in_names instead of camelCase.

Class Structure

This implementation is really up to you. Without a little more detail on your program structure its hard to suggest how the classes should be arranged. However, it feels like the isVideoSrcValid function as well as VALID_EXTENSIONS should be in some video class.

isVideoSrcValid

This code can be simplified. What does it matter if the file had no extension? If thats the case, then the next check not filenameTokens[1][1:].lower() in VALID_EXTENTIONS would make the function return false. Because of this, we can remove the 2nd if-statement then just return the value from the last if-statement.

Instead of slicing our filenameTokens extension (to remove the .) simply add a period to the VALID_EXTENSIONS. Also, unpack the tuple returned from splitext() for readability.

Here is the revamped code:

VALID_EXTENSIONS = ['.avi', '.mp4'] def isVideoSrcValid(src): if (not path.isfile(src)): return False root, ext = path.splitext(src) return ext.lower() in VALID_EXTENTIONS 

Testing

Your tests look pretty comprehensive. I have two main points:

1. Off-base tests

   Why are you testing os.path.isfile()? I understand its to make sure that your code in setUp functioned properly and open().close() created valid files. However, the only way your open().close() calls would not create valid files is if they threw an error. If thats the case, your tests would not run.

   Moreso the point I want to make is this: testing os.path.isfile() is not the point of your tests and this test class. This test class should only test whether or not isVideoSrcValid works correctly. These tests are independent and thus assume that the functions it uses act politely.

2. Name your test functions to indicate what they test.

   Test functions should be named to indicate what they test, not what they test with. For example, take your first test function: test_empty(). This is saying "I'm testing my function with an empty string". However, how much information does this give us if we needed to debug should it fail? Did the first if-statement fail (yes)? Or was it the second check?

   Names like that can be ambiguous in that sense. I would recommend changing that function to:

   def test_is_file_failure(self): self.assertFalse(vc.isVideoSrcValid("")) # Test with an os resource that is not a file. self.assertFalse(vc.isVideoSrcValid(os.path.expanduser('~')))