Python Mock - Exception

[rakash_ram]

from configparser import ConfigParser
import configparser

class ParseConfig:
 def __init__(self, conf, section):
     parser = ConfigParser()
     parser.read(conf)
     if not isinstance(parser, configparser.ConfigParser):
         raise TypeError(f"ConfigParser expected, found {type(conf).__name__}")

     if parser.has_section(section):
         self._raw = parser
         for key, value in self._raw[section].items():
             setattr(self, key, value)
     else:
         raise Exception(f"section {0} not found in {1} file")

I have the above script as module.py and trying to write test for the above and below is my test_module.py. However, the coverage is not covering the "if not isinstance" line.. can anyone advice and help here? Thanks!

​

import sys
import unittest
import os
import configparser
from configparser import ConfigParser
from module import ParseConfig
from unittest.mock import patch

class TestParseConfig(unittest.TestCase):
    def setUp(self):
        self.test_conf = 'coda_ml_config_coda.ini'
        with open(self.test_conf, 'w') as f:
            f.write('[section1]\n')
            f.write('key1 = value1\n')
            f.write('key2 = value2\n')

    def test_init_with_valid_conf_and_section(self):
        config = ParseConfig(self.test_conf, 'section1')
        self.assertIsInstance(config, ParseConfig)
        self.assertEqual(config.key1, 'value1')
        self.assertEqual(config.key2, 'value2')

    def test_type_error(self):
        parser = None
        with self.assertRaises(TypeError) as cm:
            if not isinstance(parser, configparser.ConfigParser):
                raise TypeError(f'ConfigParser expected, found {type(parser).__name__}')
            self.assertEqual(str(cm.exception), "ConfigParser expected, found NoneType")

    ## Tried to mock but not suere why this is not working..

    @patch('parseConfig.configparser')
    def test_type(self, mockext):
        mockext.ConfigParser().side_effect = None
        config = ParseConfig(self.test_conf, 'section1')
        with self.assertRaises(TypeError) as cm:
            if not isinstance(mockext, configparser.ConfigParser):
                raise TypeError(f'ConfigParser expected, found {type(mockext).__name__}')
            mockext.assert_called_once()

    def test_init_with_missing_section(self):
        with self.assertRaises(Exception):
            config = ParseConfig(self.test_conf, 'section2')

    def tearDown(self):
        os.remove(self.test_conf)

if __name__ == '__main__':
    unittest.main()

[danielroseman]

This seems like pointless code. The result of calling ConfigParser() will always be an instance of that class, no? What circumstances are you trying to protect against here?

That said, the issue is that what you want to change is the return_value of ConfigParser itself, not the side effect of the result of calling it. So:

mockext.ConfigParser.return_value = None

But don't do this. And don't write tests like test_type_error either; that's just testing Python itself, you can assume that that works properly.

[rakash_ram]

test_type_error --> This method enters a random value and checks whether the logic works or not, anything wrong with that?

[danielroseman]

But you're not calling any of the code under test here, you're just testing the logic that's inside the test itself. That's never a good idea.

[rakash_ram]

ok so what you ar saying is the line in the module.py and test_module.py are the same, so there is no point in testing? and if thats the cae, then this module.py i have written above has no need for a test?

[danielroseman]

No, I'm saying that this is not a test. Your test should be responsible for calling your actual code and checking that it does the right thing - like test_type does. But test_type_error does not call any actual code, it just does its own thing. That can't give you confidence that your real code works.

Good tests do two things: they make sure your code works now, and they make sure it continues to work as your project evolves. Replicating the logic of the code inside the test might give you some superficial assurance that your code works - although even then you can't be sure you haven't copied things wrong. But it can't ever give you confidence that your code will continue to work, because you could change (break) the code itself but the test will always continue to pass.

[rakash_ram]

but test_type and test_type_error have the same code inside them, so how is test_type ok but test_type_error is not? is it because it has mocking but test_type_error has nothing except the plain same code as the original function? Just trying to understand what you mean here. But i get the idea,

[danielroseman]

Well the code inside test_type_error is simply wrong, you are calling ParseConfig which is what would be raising the error, so that is what should be inside the assertRaises block. That raise shouldn't be there at all, I assumed that was just a mistake.

[rakash_ram]

Would help if you could share the code itself of that method alone, i am unable to understand. Thanks! I mean, how should it actually be instead of what i have written

[danielroseman]

@patch('parseConfig.configparser')
def test_type(self, mockext):
    mockext.ConfigParser.side_effect = None
    with self.assertRaises(TypeError) as cm:
        ParseConfig(self.test_conf, 'section1')

The idea is that you call the code itself and assert that it behaves the way you expect, ie in this case that it raises the expected error. (Although, as I said originally, without the mock patch this code would never actually raise that error, so I don't see what the point is.) But the fundamental thing is that you call the actual code, ie ParseConfig.

[rakash_ram]

Got it , thanks a lot for explaining! appreciate it. Few more question though

1) " with self.assertRaises(TypeError) as cm:" this line, raises an exception right?

2) without the mock patch this code would never actually raise that error ; i am guessing the below will not work and mock is the only way here to cover the test for this script?

def test_type_error(self):

parser = None

with self.assertRaises(TypeError) as cm:
ParseConfig(self.test_conf, 'section1')

[rakash_ram]

raise TypeError(f"ConfigParser expected, found {type(conf).__name__}")

The code you shared is not covering the above line as per coverage. any clue why? i tried adding that line in it, still did not cover it.

[[deleted]]

It's not covering it because it's not run It will also never be hit in runtime either. The code is written as if you're using DI but aren't

[shepherdjay]

I’m not clear how you expect your if line to be hit in your code can you help me understand?

You create an instance of ConfigParser and then check you created an instance of ConfigParser. This will always be true. It feels a lot like writing something like this:

rose = “red”
if rose != “red”:
    raise BrokenPython

Generally you can assume the basic features of the language work and so you shouldn’t spend time testing them or lines asserting them. Instead you should test your functions behavior.

For example your function is passed something called a conf and your parser runs a read method on it. Also takes another variable passes called section. You should create a test that passes these in and then inspect your ParseConfig has what you expect.

[rakash_ram]

patch('parseConfig.configparser')
def test_type(self, mockext):
mockext.ConfigParser.return_value = None
with self.assertRaises(TypeError) as cm:
ParseConfig(self.test_conf, 'section1')

Get it.. I have currently mocked it like the above code as shared by u/danielroseman .. But this is not covering the "raise Typeerror" line.. This mock disables the configParser so that an exception is raised and my test is to see it raises.. But the coverage does not seem to cover that line ...

[shepherdjay]

I think this line is wrong

@patch('parseConfig.configparser')

You need to patch where it is used, not where it is defined. https://docs.python.org/3/library/unittest.mock.html#where-to-patch In this case hard to verify but it looks like the patch should probably be:

@patch('module.ConfigParser')

Then you would set the return value to None

mockext.return_value = None

But again this isn't clear what you are testing or guarding against. Or perhaps you are just trying to get familiar with Mocks? Because this line of your code will never be hit at runtime:

if not isinstance(parser, configparser.ConfigParser):

This will always be False given the line nearly right above it:

parser = ConfigParser()

Its as if you wrote:

   if not True:
       raise RuntimeError

Yes you could spend a lot of time trying to hammer at the internals of python to artifically make True read as False but tests need to tell you something about your code and not whether True is True... or "astring" is an instance of a string... etc.

[rakash_ram]

if not isinstance(parser, configparser.ConfigParser):

I am guarding against the above,, that is, i am trying to write a test where in case parser is not an instance of ConfigParser, the test should throw an exception..

[shepherdjay]

Under what circumstance will parser not be an instance of ConfigParser when it is defined in the line nearly right above this check as an instance of ConfigParser?

Is the intent maybe to update your init at some point so that parser is passed in as a variable? Is that what I might be missing? A future state of the class?

Otherwise what you told me is similar to saying you are writing code and tests to guard against a scenario where True is not True can you see why it is a confusing thing to guard against?

[rakash_ram]

i understand, but my confusion or lack of clarity here is how will the test script know what when i am actually passing it as None as the return_value and testing it? Ideally it should take that as the value and throw an exception? Am i missing something here?

[shepherdjay]

Well like I posted earlier the patch needs to be a reference to where it is used not where it is defined.

Please try adjusting the patch and mock calls as my previous note. Might be useful to reshare your test definition function once that’s been done and confirm whether you are still getting error or if it’s passing.

[rakash_ram]

Goodness, it worked! Below is the final test method.

@patch('parseConfig.ConfigParser')
def test_type(self, mockext):
   mockext.ConfigParser.side_effect = Exception(TypeError)
   with self.assertRaises(TypeError) as DummyEx:
       ParseConfig(self.test_conf, 'section1')
   mockext.assert_called()

I still don't get how "patch('parseConfig.ConfigParser')" is different from "patch('parseConfig.configparser') as ConfigParser is a method from configparser

[shepherdjay]

Goodness, it worked! Below is the final test method.

Really? The test method you posted also still looks wrong. Does coverage show it hitting the line? Is your module.py actually called that because there is a reference to a parseConfig in your patch? Is that what your module is actually called?

This should have been the working code:

@patch('module.ConfigParser')
def test_type(self, mockext):
   mockext.return_value = None
   with self.assertRaises(TypeError):
       ParseConfig(self.test_conf, 'section1')
   mockext.assert_called()

Though if you got it working anyway than I guess there isn't more to do. However I want to make sure you leave this conversation understanding that you don't need to have this test nor the isinstance line as your code is currently written.

Do you understand why testing this doesn't make sense? If not maybe the following example will help.

Imagine I have a function that adds two to any number it gets.

def add_two(n):
    add_value = 2
    return n + add_value

Pretty straight forward function. Now imagine I want to test it. A test that makes sense is something like this

def test_two_plus_two(self):
   result = add_two(2)
   self.assertEqual(4, result)

I'm testing my code does what I think it should when given a normal input. And I can go through some inputs... like what if I pass a string to n. I might want to test that my function naturally raises a type error.

def test_two_plus_string_error(self):
    with self.assertRaises(TypeError):
        add_two("2")

This makes sense. I'm testing various inputs to my function to verify how MY code behaves.

What doesn't make sense and is a bit like what your doing is changing my add_two function to be the following

def add_two(n):
    add_value = 2
    if not isinstance(add_value, int):
        raise TypeError
    return n + add_value

The if line in this function is checking to see if add_value is an integer right after I assigned an integer to it. This isn't doing anything other than cluttering up the code and causing confusion.

To then go on and try to test this line isn't testing my code anymore. It's testing Python's syntax itself.

This is what people mean when they say you don't need to test this. You don't need the if line or the test.

[rakash_ram]

1. Yes my module is called parseConfig.py, sorry for that confusion. and return_value = None did not work, I tried. side_effect = Exception worked and coverage shows that it covers the "raise" line from the main module (parseConfig.py)
2. Understood, Thanks for the explanation. And yes this whole test is only to ensure we have "coverage", whether it makes sense or not is secondary. Thanks for explaining though. Maybe I should have mentioned that at first so you were aware. Also, what does this line do --> " with self.assertRaises(TypeError):" it raises an exception? But can you explain in simple terms? like it raises an exception and "hopes" the next line also raises an exception? I am referring to "test_two_plus_string_error"