20

Is it good or bad to duplicate data between tests and real code? For example, suppose I have a Python class FooSaver that saves files with particular names to a given directory:

class FooSaver(object):
  def __init__(self, out_dir):
    self.out_dir = out_dir

  def _save_foo_named(self, type_, name):
    to_save = None
    if type_ == FOOTYPE_A:
      to_save = make_footype_a()
    elif type == FOOTYPE_B:
      to_save = make_footype_b()
    # etc, repeated
    with open(self.out_dir + name, "w") as f:
      f.write(str(to_save))

  def save_type_a(self):
    self._save_foo_named(a, "a.foo_file")

  def save_type_b(self):
    self._save_foo_named(b, "b.foo_file")

Now in my test I'd like to ensure that all of these files were created, so I want to say something like this:

foo = FooSaver("/tmp/special_name")
foo.save_type_a()
foo.save_type_b()

self.assertTrue(os.path.isfile("/tmp/special_name/a.foo_file"))
self.assertTrue(os.path.isfile("/tmp/special_name/b.foo_file"))

Although this duplicates the filenames in two places, I think it's good: it forces me to write down exactly what I expect to come out the other end, it adds a layer of protection against typos, and generally makes me feel confident that things are working exactly as I expect. I know that if I change a.foo_file to type_a.foo_file in the future I'm going to have to do some search-and-replace in my tests, but I don't think that's too big of a deal. I'd rather have some false positives if I forget to update the test in exchange for making sure that my understanding of the code and the tests are in sync.

A coworker thinks this duplication is bad, and recommended that I refactor both sides to something like this:

class FooSaver(object):
  A_FILENAME = "a.foo_file"
  B_FILENAME = "b.foo_file"

  # as before...

  def save_type_a(self):
    self._save_foo_named(a, self.A_FILENAME)

  def save_type_b(self):
    self._save_foo_named(b, self.B_FILENAME)

and in the test:

self.assertTrue(os.path.isfile("/tmp/special_name/" + FooSaver.A_FILENAME))
self.assertTrue(os.path.isfile("/tmp/special_name/" + FooSaver.B_FILENAME))

I don't like this because it doesn't make me confident that the code is doing what I expected --- I've just duplicated the out_dir + name step on both the production side and the test side. It won't uncover an error in my understanding of how + works on strings, and it won't catch typos.

On the other hand, it's clearly less brittle than writing out those strings twice, and it seems a little wrong to me to duplicate data across two files like that.

Is there a clear precedent here? Is it okay to duplicate constants across tests and production code, or is it too brittle?

15

I think it depends on what you're trying to test, which goes to what the contract of the class is.

If the contract of the class is exactly that FooSaver generates a.foo_file and b.foo_file in a particular location, then you should test that directly, i.e. duplicate the constants in the tests.

If, however, the contract of the class is that it generates two files into a temporary area, the names of each which are easily changed, especially at runtime, then you must test more generically, probably using constants factored out of the tests.

So you should be arguing with your coworker about the true nature and contract of the class from a higher level domain design perspective. If you can't agree then I would say that this is an issue of the understanding and abstraction level of the class itself, rather than of testing it.

It is also reasonable to find the class's contract changing during refactoring, for example, for its abstraction level to be raised over time. At first, it my be about the two specific file in a particular temp location, but over time, you may find additional abstraction is warranted. At such time, change the tests to keep them in sync with the contract of the class. There is no need to over build the class's contract right away just because you're testing it (YAGNI).

When a class's contract is not well defined, testing of it can make us question the nature of class, but so would using it. I would say that you shouldn't upgrade the contract of the class just because you are testing it; you should upgrade the contract of the class for other reasons, such as, it is a weak abstraction for the domain, and if not then test it as it is.

4

What @Erik suggested--in terms of making sure you are clear on what it is you are testing--should certainly be your first point of consideration.

But should your decision lead you to the direction of factoring out the constants, that leaves the interesting part of your question (paraphrasing) "Why should I trade off duplicating constants for duplicating code?". (In reference to where you talk about "duplicat[ing] the out_dir + name step".)

I believe that (modulo Erik's comments) most situations do benefit from removing duplicate constants. But you need to do this in a way that does not duplicate code. In your particular example, this is easy. Instead of dealing with a path as "raw" strings in your production code, treat a path as a path. This is a more robust way to join path components than string concatenation:

os.path.join(self.out_dir, name)

In your test code, on the other hand, I would recommend something like this. Here, the emphasis is showing that you have a path and you are "plugging in" a leaf file name:

self.assertTrue(os.path.isfile("/tmp/special_name/{0}".format(FooSaver.A_FILENAME)))

That is, by more thoughtful selection of language elements, you can automatically avoid code duplication. (Not all the time, but very often in my experience.)

1

I agree with Erik Eidt's answer, but there is a third option: stub out the constant in the test, so the test passes even if you change the value of the constant in the production code.

(see stubbing a constant in python unittest)

foo = FooSaver("/tmp/special_name")
foo.save_type_a()
foo.save_type_b()

with mock.patch.object(FooSaver, 'A_FILENAME', 'unique_to_your_test_a'):
  self.assertTrue(os.path.isfile("/tmp/special_name/unique_to_your_test_a"))
with mock.patch.object(FooSaver, 'B_FILENAME', 'unique_to_your_test_b'):
  self.assertTrue(os.path.isfile("/tmp/special_name/unique_to_your_test_b"))

And when doing things like this I would usually make sure to do a sanity test where I run the tests without the with statement and make sure I see "'a.foo_file' != 'unique_to_your_test_a'", then put the with statement back in the test so it passes again.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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