Error testing file reading function with pytest

Question

I have a simple function:

def read_file(fp):
    with open(fp) as fr:
        for line in fr.readlines():
            yield line

When I run this function on a non-existent file, I get:

FileNotFoundError: [Errno 2] No such file or directory: 'idontexist.txt'

In another file, I am trying to test this function using pytest:

import pytest
from utils import read_file

def test_file_not_exist():
    filepath = 'idontexist.txt'
    with pytest.raises(FileNotFoundError):
        read_file(filepath)

However, running pytest, I get the message:

E           Failed: DID NOT RAISE <class 'FileNotFoundError'>

Why does this test not pass?

Answer 1

You are creating a generator function. Calling the generator function returns a generator object:

>>> def read_file(fp):
...     with open(filepath) as fr:
...         for line in fr.readlines():
...             yield line
... 
>>> read_file('asd')
<generator object read_file at 0x10554ee08>

The open call (which should raise FileNotFoundError) will not be called until you loop over the generator. Then you will see

>>> g = read_file('asd')
>>> for x in g:
...     pass
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in read_file
NameError: name 'filepath' is not defined

I am guessing that you half-changed fp to filepath before posting here. You can fix that but either way you won't see any error from read_file unless you consume the returned generator by iterating over it.

EDIT: I don't recommend using a with statement inside a generator like this. The reason is that there is no guarantee that the file will be closed.

To understand this consider what the purpose of with is. Before context managers you would open a file like

f = open(filename)
f.read(4)
# etc
f.close()

The problem with this is that the file may not get closed if something happens (e.g. an exception or return etc) between the open and the close. You can fix this with try/finally i.e.

f = open(filename)
try:
    f.read(4)
    # etc
finally:
    f.close()

This is cumbersome so we have the with statement to shorten it to

with open(filename) as f:
    f.read(4)
    # etc

This is good because it reduces clutter and there's no way to leave the with statement without the file being closed. However when you do it in a generator like your read_file above someone might call the generator with

for line in read_file(filename):
    if line.startswith('#'):
        break

Now after the break the generator is suspended at the yield it has no way to know that it will not be iterated on again so it waits there. The yield inside the with block allows you to leave the context manager without closing the file. (The same problem would also occur when using try/finally but it's perhaps more obvious in that case.) Even if you know that you won't break an exception from the loop body will have the same effect.

What happens in this case is that the file will probably be closed as a result of the ref-counting GC in CPython: when the generator is collected it will be closed, an exception is thrown in terminating the with block and hence closing the file. This isn't much better than allowing the GC to collect the file object f directly (which also closes the file through file.__del__).

The simple rule is:

Don't yield inside a with statement

That means you should usually take the with statement outside the generator. So you do something like

def read_file(f):
    for line in f.readlines():
        yield line

# Control resource at top level
with open(filename) as fin:
    for line in read_file(fin) # pass the resource to generator
        # do something with line

Another point: the whole point of iterators is that they allow us not to have to do things like read a whole file into memory. So rather than calling readlines() which reads the while file into memory you should iterate over the file directly which only reads one line at a time. With these two changes your function looks like:

def read_file(f):
    for line in f:
        yield line

Or even:

def read_file(f):
    yield from f

In terms of iterators this is just the identity function so it is redundant and can be removed. So wherever you use the read_lines function you can instead just use

with open(filename) as fin:
    for line in fin:
        # do stuff

(i.e. there is no read_lines function in the code any more)