Triggering an old bug with a new code

The article covers a real-life adventure. The example needs to be more concise for readability purposes.

Imagine a common situation where you are developing a new task in an existing project. The tests cover the code well (now this might be unusual).

The job is to write a function returning a value of a field from a given dictionary. It has to be error-resistant and receive the dictionary and key as optional key arguments. You might end up with something like this:

# get_field.py
from typing import Any, Optional
 
 
def get_field(dictionary: Optional[dict] = None, key: str = "field") -> Any:
    """Getting a field value from a dictionary."""
    dictionary = dictionary or {}
    return dictionary.get(key)

The code requires some testing. You might have even been writing it in TDD. The following is a pretty unreasonable test file:

# test_b_get_field.py
from .get_field import get_field
from .resource import IMPORTANT_DICT
 
 
def test_get_field_minimal():
    assert get_field() is None
 
 
def test_get_field_another_key():
    assert get_field(key="another key") is None
 
 
def test_get_field_existing_dict():
    assert get_field(dictionary=IMPORTANT_DICT) == {"key": "value"}
 
 
def test_get_field_existing_dict_another_key():
    assert get_field(dictionary=IMPORTANT_DICT, key="another key") is None

Using the IMPORTANT_DICT imported from resource.py file.

#resource.py
from typing import Union
 
 
IMPORTANT_DICT: dict[str, Union[str, dict]] = {"field": {"key": "value"}}

Running the tests shows no issues:

pytest test_b_get_field.py -q
…. [100%]
4 passed in 0.01s

You're happy with the solution, so you run the entire suite and see the test_get_field_existing_dict failing, which seems impossible.

pytest -vv
============================ test session starts ===========================
collected 8 items
test_1_trivia.py::test_trivia PASSED [ 12%]
test_a_set_field.py::test_set_field_provided_key PASSED [ 28%]
test_a_set_field.py::test_set_field_existing_dict PASSED [ 42%]
test_b_get_field.py::test_get_field_minimal PASSED [ 57%]
test_b_get_field.py::test_get_field_another_key PASSED [ 71%]
test_b_get_field.py::test_get_field_existing_dict FAILED [ 85%]
test_b_get_field.py::test_get_field_existing_dict_another_key PASSED [100%]
================================== FAILURES ================================
************************* test_get_field_existing_dict **********************
def test_get_field_existing_dict():
> assert get_field(dictionary=IMPORTANT_DICT) == {"key": "value"}
>
> E AssertionError: assert 1 == {'key': 'value'}
> E + where 1 = get_field(dictionary={'field': 1})
test_b_get_field.py:14: AssertionError
========================== short test summary info =========================
FAILED test_b_get_field.py::test_get_field_existing_dict -
 AssertionError: assert 1 == {'key': 'value'}
======================== 1 failed, 7 passed in 0.12s =======================

The get_field(dictionary=IMPORTANT_DICT) is equal to 1 and not {"key": "value"} as it was set in resource.py. This behavior is usually caused by modifying an imported variable in a test or another function. To locate the issue, you need to find the test causing the variable change. In the real world, there are hundreds of tests in multiple files.

Let's find the one which makes the test fail. First, try to run the test_b_get_field.py after the test_1_trivia.py:

pytest -q test_1_trivia.py test_b_get_field.py
….. [100%]
5 passed in 0.01s

Test passes, which means it's not the problematic one. Let's try the following test file — test_a_set_field.py:

pytest -q test_a_set_field.py test_b_get_field.py
[…]
1 failed, 6 passed in 0.10s

OK, you're closer to the solution. The test_a_set_field.py file has multiple tests inside. You can find which one to blame by running them one by one

pytest -q test_a_set_field.py::test_set_field_provided_key test_b_get_field.py
….. [100%]
5 passed in 0.02s

Until the test fails and you get the right one:

pytest -q test_a_set_field.py::test_set_field_existing_dict test_b_get_field.py
[…]
1 failed, 4 passed in 0.09s

Now you can investigate the "villain" test:

# test_a_set_field.py
from .set_field import set_field
from .resource import IMPORTANT_DICT
 
 
# …
def test_set_field_existing_dict():
     assert set_field(1, dictionary=IMPORTANT_DICT) == {"field": 1}

You find out that the method set_field is called using the same IMPOTRANT_DICT you're using in your test. Let's take a look at the function.

# set_field.py
from typing import Optional
 
def set_field(value: str, dictionary: Optional[dict] = None, key: str = "field") -> dict:
    """Setting a field value in a dictionary."""
    dictionary = dictionary or {}
    dictionary[key] = value
    return dictionary

As you realize, the function modifies the dictionary key argument by setting the given key. Dictionaries are mutable in Python. That happens because a reference is passed as an argument instead of the value of a dict variable. You can see it in a following code:

>>> def change(var):
...     var["key"] = "value"
...
>>> d = {}
>>> change(d)
>>> d
{'key': 'value'}

Going back to our example. The current code might be a desired situation. If so, calling the test using an imported variable shouldn't happen. You would change it to a local one.

More often, developers want to use "pure" functions that do not modify the argument. Let's check how the function is called in the project. You've run a search for set_value( in the entire project and found the following line:

    something = set_value(new_value, dictionary=old_dict)

You've also realized the original developer no longer uses the old_dict variable afterward. That would mean the set_value function shouldn't change the dictionary argument. You need to refactor the method.

There are multiple solutions to this problem. For example, you could use the deepcopy() method. You've decided to use the union argument — | introduced in Python 3.8 and return the modified dictionary. The set_value function looks now like this:

# set_value.py
from typing import Optional
def set_field(value: str, dictionary: Optional[dict] = None, key: str = "field") -> dict:
    """Setting a field value in a dictionary."""
    dictionary = dictionary or {}
    return dictionary | {key: value}

Let's test again:

poetry run pytest -q test_a_set_field.py::test_set_field_existing_dict test_b_get_field.py
….. [100%]
5 passed in 0.02s

And with the entire testing suite:

poetry run pytest -q
…….. [100%]
8 passed in 0.03s

Job done. You've fixed the code. This issue will show up no more.

The process might be long and repetitive. You can create a script that will do the job for you. I haven't done it. If I'd decide to, I'd start with pytest --collect-only:

pytest  --collect-only
============================ test session starts ============================
collected 8 items
 
<Package article>
  <Module test_1_trivia.py>
    <Function test_trivia>
  <Module test_a_set_field.py>
    <Function test_set_field_minimal>
    <Function test_set_field_provided_key>
    <Function test_set_field_existing_dict>
  <Module test_b_get_field.py>
    <Function test_get_field_minimal>
    <Function test_get_field_another_key>
    <Function test_get_field_existing_dict>
    <Function test_get_field_existing_dict_another_key>
 
======================== 8 tests collected in 0.04s =========================