3
\$\begingroup\$

This is my first time using pytest. All feedback for this test case is much appreciated.

import pytest
from mock import mock_open, patch


def get_file_contents(file_data):
  with patch.object('builtins.open', mock_open(read_data=file_data)) as mock:
    with open('mocked_file') as f:
      return (line for line in f.readlines())


@pytest.fixture(scope="module")
def text_parser(request):
  from tparse import TextParser
  file_data = getattr(request.module, 'file_contents')
  tparse = TextParser(file_data)
  def fin():
    tparse.close()
  request.addfinalizer(fin)
  return tparse


def test_get_system_entry(self):
  file_data = 'dc nyc server server001 ipaddress 10.10.10.10'
  file_iterable = get_file_contents(file_data)
  assert file_iterable == file_data
\$\endgroup\$
3
\$\begingroup\$

I'm not familiar using py.test, but here are some comments I have after reading your code:

  1. There are no comments - this would be helpful to the reader.
  2. In text_parser, you have from tparse import TextParser. I would advise moving this import to the top of the file, for readability.
  3. What is your code trying to do with these tests? What are you testing?
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback Ryan. I was trying to follow the examples from py.test where it seems like including the imports in text fixtures is a pattern, however had I been performing traditional unit tests, I would have moved the imports to the top of the file. However, I am in agreement with you, and will most likely move the imports to the top of the file anyway. Currently I am testing a method that parses a string returned from an iterable. So basically open a file, read each line in the file, and the method will return the entry once the pattern is found. \$\endgroup\$ – fr00z1 May 26 '15 at 4:45
  • \$\begingroup\$ Also, apologize for the lack of comments. Was trying to be descriptive in the code itself, however it will be something I keep in mind in the future so I can be respectful of ppls time. Thanks for taking the time to look at this. \$\endgroup\$ – fr00z1 May 26 '15 at 4:47
2
\$\begingroup\$

I don't understand this test:

def test_get_system_entry(self):
  file_data = 'dc nyc server server001 ipaddress 10.10.10.10'
  file_iterable = get_file_contents(file_data)
  assert file_iterable == file_data

The name of the test method suggests testing of getting "system entry". Inside, I see a string (file_data), and something that appears to be an iterable (file_iterable), and a call to get_file_contents, such that this expression is expected to be true:

file_data == get_file_contents(file_data)

So, we test getting a system entry by verifying that x == get_file_contents(x) ? I don't know what to make of that.

A good unit test case should be:

  • perfectly clear
  • easy to read and understand without deciphering
  • have a good, descriptive name

And what is the self parameter for? "self" is used as the first parameter of class methods. But the test_get_system_entry method is not within a class, and the parameter is not used anyway. This is confusing code.

\$\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.