Doing QA Wrong

I've been working on some test code that originated from some of the other developers in support. I thought at first it had come from someone who had left a little over a year ago, but it turns out it did not. I wish I could say more, but I don't like saying much about people I have to work with if it isn't nice.

1. Using network locations for image sources and temporary storage.

Reason: This shouldn't require too much explanation. If there is no network connection, the tests won't work. If the machine the images are on is off, gets destroyed, goes down, crashes, etc, the tests won't work. If someone replaces one of the files due to some reason the tests won't work. The tests already require that one machine be in sane condition - adding another machine to that AND a network connection between the two when it's not necessary is just plain dumb. In addition to the basics, when you access resources across a network permission issues tend to arise. This is not something you want to deal with when simply running tests. The test code itself should not be able to fail easily.

What I Do: All test code is in source control. I keep files used by that code under the same tree the code is in. This means that baselines and test files are also in source control (as they should be). I usually call this folder TestFiles. Under that folder I keep two more folders: SourceFiles and VerificationFiles. Source files would be the location for source images, or any other file that is used in a test. Verification files is where I keep all files that are used to compare with the output of a test for pass determination. This allows for really simple running of the test code. Source control also allows for many benefits in this scenario. If a file changes then there should be a comment as to why it changed. We can see what about a file changed because we still have both versions. Unwanted overwrites and corruption are also prevented in this scenario.

2. Saving files used in comparisons to hard coded paths (or saving anything to a hard coded path really)

Reason: You can never really be sure that a hard coded path will exist on a system. Especially C:\. Not only that, but writing files to the root of C is just a really bad idea for a number of reasons. If you use the same hard coded path for output files, and run more than one test, the second test that uses that file is invalid. This is because the file exists in the first place, so if the line in the test that outputs the file is skipped, the test either still has a chance to pass, or will fail for unknown or strange reasons. Assuming that you are testing that a component is outputting a file, this is exactly what you want to prevent in your test code.

What I Do: I always use the windows GetTempFileName and GetTempPath api or their environment's equivalent when outputting temporary files. This can create an issue where temp files build up on a system, so it is necessary to ensure that the files are removed after the test completes. This requires a little more work than hard coding a path, but the test quality you achieve as a result is definitely worth it.

3. Not testing the result of an operation

Reason: I really don't think I need to explain this. I was REALLY surprised to see code like that in the tests. Code that literally did nothing but call a function. The test makes sure the function doesn't throw an error or make the suite crash. <THICK SARCASM>Oh yeah, that's really useful!</THICK SARCASM> That's like manually testing if MS Word can open a file by going to file:open, selecting the file, hitting accept, and doing nothing more. If the dialog goes away the test passed. Whether or not it actually opened the file is not tested - there is no verification that there's anything on the screen. It's just testing that it looks like you can open a file. That's a useless test!

What I Do: Verify the output of the call/method or verify that the property has the right value. The method is supposed to do something, so check the things it was supposed to do. I really don't think I need to go more in depth here.

4. Use 1 Error handler that only says the test failed.

Reason: This provides very little information to the developer or even the tester. Without any more detail this gives us no information on why the test failed. This means that someone will have to step through the entire test to find the line of code that it failed on. This does not mean there has to be an error handler for every line of code though.

What I Do: In VB (COMUnit) I try to use an error handler for every method that outputs the error number and description. That's all that is really needed there. For .NET, use ample try/catch blocks, and give valid descriptions for your assertions and exceptions. This isn't hard - it's just that people can be really lazy sometimes.

5. Not thinking about what the test is really doing

Reason: This has a specific case to accommodate it. There was a test that was outputting a color image, in memory, that measured 50in x 30in x 300dpi on a 32bit machine. The test never passed. The test never will pass. It is impossible to fit that image into the data type that it was being output in. The requirement was ridiculous, and no one had taken the test out.

What I Do: Make sure what the test is doing is sane. No reason to test things that can't be tested. No reason to test absurd things.

6. Checking that an output file is correct by making sure is is greater than a hardcoded number of bytes.

Reason: I shit you not that this code was in there. It outputs a file, and asserts that it's greater than the "baseline file size". The baseline file size is 4000. I really don't need to go into further depth as this also falls under other categories.

What I Do: When testing files against baseline files I have a set of methods I use depending on the file type and one method for generic files. Length is asserted to be the same for both files first, then they are opened and tested byte by byte. When there is a byte difference, the location in the file where they differed is in the test error/exception. Simple.

7. Using reserved words for labels/names/variables (On Error GoTo error)

Reason: If you need to ask, you need to take an introduction to computer programming course.

What I Do: I don't use reserved words for variables, names, or labels. Simple.

8. Using Assert True or testing a condition then raising a failure

Reason: First, using a constant true in the expression will never issue the message. A good compiler will even take the line out completely. Why the code was there in the first place simply shows me that the original coder has no idea what they are actually doing. The assert statement's expression is the conditional. Adding more code to issue a failure on a conditional is counter-productive. It is bad to add useless code.

Example Bad Code:

Assert(True, "This message will never ever show.");
If (someCondition)
Assert(False, "There was an error");

Proper Code:
Assert(someCondition, "There was an error");

Some of these issues seem absolutely ridiculous to me. I almost cannot believe I found code like this here. What's worse is that it won't be fixed any time soon because it would take too much time, and the people are working on other things. If I find anything else I'll add it here...