[qa] Add mocks to unit tests

[purhan]

Replaced calls with mocks for the following unit tests:

• tests/snmp/base.py
• tests/snmp/openwrt.py
• tests/snmp/airos.py

Closes #46

[nemesifier]

JSON formatting looks a lot better now. Thank you! Regarding the technical limitation with SNMP and in general on this subject, I understand, do what you can but if it's too much work leave it.

Let's focus your effort on achieving the goal of mocking each call to the network during tests.

[pandafy]

@purhan since we are mocking tests, can we remove the need to defined test-settings.json? What will we have to achieve to be able to do that?

[purhan]

@purhan since we are mocking tests, can we remove the need to defined test-settings.json? What will we have to achieve to be able to do that?

We can hard-code some fake credentials in the tests, what do you think?

[purhan]

@pandafy Done, all tests should now run without test-settings.json.
Just run

nosetests

PS: I noticed that tests for HTTP backend are not included when you run this command.

[pandafy]

PS: I noticed that tests for HTTP backend are not included when you run this command.

We will need to fix this. Adding from .http import * to tests/__init__.py seems to fix this issue. Can you confirm?

We can hard-code some fake credentials in the tests, what do you think?

Thank you for making this change.

[pandafy]

@purhan since we are mocking tests, can we remove the need to defined test-settings.json? What will we have to achieve to be able to do that?

I developed some doubts regarding above review of mine. I forgot to bring it up on the call but after a quick discussion with Federico, we concluded that it be good to have the ability to run the tests with or without real device.

The current state of the patch works well to run tests without a real device. This is good for running tests in CI. So we have one part already done.

For the second part, I was thinking to have a runtests script (like other modules of openwisp) for running tests. In that script we can add a flag like --test-settings that can be used to provide path to test-settings.json.

User/Developer will be required to call the script like this

./runtests.py --test-settings=test-settings.json

On executing above command actual network calls should be executed in tests instead of mocks. This will ensure that along with the test suite, our mocks are also correct.

What do you think about this?

[purhan]

On executing above command actual network calls should be executed in tests instead of mocks. This will ensure that along with the test suite, our mocks are also correct.

What do you think about this?

I have a couple of queries related to this:

1. What if we want to run a test separately? eg: nosetests tests.snmp.openwrt
   Then we lost this ability

Possible solution: we can make use of environment variables in settings.py which will be used in each test file.

2. We use patches in context managers, and I could not find an inbuilt solution to disable/unpatch those patches

Possible solution: we can define a class like this:

class DisableMock(object):
        ...
        def start(self):
            pass

        def stop(self):
            pass

        def __enter__(self, *args, **kwargs):
            return self

        def __exit__(self, *args, **kwargs):
            pass

We can then use this class to disable any patch:

if settings['disable_mocks']:
        self.ssh_patcher = DisableMock()

Edit: Done in f6f4eb5

[nemesifier]

Is this PR meant to be tested with python2?

[purhan]

⚠️⚠️⚠️ @nemesisdesign @pandafy I've made an important change in 4e240ec that could've been a blunder.:warning::warning::warning:

Earlier, I was patching internal functions of netengine, eg:

# This will not detect if `next` breaks, which is an internal function of netengine
'netengine.backends.snmp.openwrt.SNMP.next'

Now:

# `nextCmd` comes from pysnmp and is not a part of netengine
'netengine.backends.snmp.base.cmdgen.CommandGenerator.nextCmd'

Now all the patched functions are from external libraries only

[pandafy]

GitHub was not letting me comment without posting this incomplete review. i will do a followup with another review.

[nemesifier]

@nemesisdesign @pandafy I've made an important change in 4e240ec that could've been a blunder.

Earlier, I was patching internal functions of netengine, eg:

# This will not detect if `next` breaks, which is an internal function of netengine
'netengine.backends.snmp.openwrt.SNMP.next'

Now:

# `nextCmd` comes from pysnmp and is not a part of netengine
'netengine.backends.snmp.base.cmdgen.CommandGenerator.nextCmd'

Now all the patched functions are from external libraries only

Yes, It may have caused issues.

[pandafy]

Good progress @purhan!

I like the SpyMock implementation. But, I believe @nemesisdesign meant decorating the tests with the patched object instead of using context manager each time like it is possible with unittest.mock.patch. See this example:

https://github.com/openwisp/openwisp-notifications/blob/e2f48f8bd6debc69064e46b1daf2252873af5734/openwisp_notifications/tests/test_ignore_object_notification.py#L27-L32

[pandafy]

LGTM 👍🏼

Let's make improvements to specific things in another PR and merge this one to proceed to other things. I have opened #60 for discussing such improvements.

Deferring merge to @nemesisdesign

[nemesifier]

@purhan this should not be necessary, please double check