current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

This is my first time using py.test. 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
share|improve this question
    
What you may and may not do after receiving answers –  Jamal♦ May 26 at 5:06
    
Thanks Jamal... Will be more strict about this in the future. –  fr00z May 26 at 9:02

2 Answers 2

up vote 0 down vote

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.

share|improve this answer
up vote 2 down vote

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?
share|improve this answer
    
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. –  fr00z May 26 at 4:45
    
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. –  fr00z May 26 at 4:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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