Improve Testability

[asomers]

This PR refactors zabbix-smartmontools to make it more testable. Now, the core code can be unit tested by mocking process.checkOutput. I've also separated the configurable bits from the main Python file into a dedicated config file. That will make it easier to use zabbix-smartmontools with a configuration management system. Finally, I've added a setup.py file for installation.

[nobody43]

I guess it will not work without, at least, any type timeout: https://github.com/nobodysu/zabbix-smartmontools/commit/c07098be5b9896959e3fcda56543940a1eae62b6 And config does not support comments on the same line. Not essential - the comments could be just removed. I'm passing out, will get back to it.

[nobody43]

$ zabbix_get -k smartctl.discovery[getverb,NON] -s windows.lan
...
"NON" smartctl.info[ConfigStatus] "CONFIGURED"
Traceback (most recent call last):
  File "C:\zabbix-agent\scripts\sender_wrapper.py", line 55, in <module>
    send()
  File "C:\zabbix-agent\scripts\sender_wrapper.py", line 44, in send
    stdin=subprocess.PIPE, universal_newlines=True, close_fds=(not isWindows()))
  File "C:\Program Files\Python37\lib\subprocess.py", line 775, in __init__
    restore_signals, start_new_session)
  File "C:\Program Files\Python37\lib\subprocess.py", line 1178, in _execute_child
    startupinfo)
FileNotFoundError: [WinError 2] The system cannot find the file specified

zabbix-smartmontools.conf is commented out.

[nobody43]

debian9# python3 setup.py install
Traceback (most recent call last):
  File "setup.py", line 1, in <module>
    from setuptools import setup, find_packages
ImportError: No module named 'setuptools'

I'm strongly against non-essential dependencies. Besides, as far as I understand, a module will make the solution release-dependent. E.g. when python will be updated on Windows the solution cease to work. I understand it might be beneficial for large installations, but for regular user it adds complexity and more rigid in usability. Maybe make module installation optional?

[asomers]

$ zabbix_get -k smartctl.discovery[getverb,NON] -s windows.lan
...
"NON" smartctl.info[ConfigStatus] "CONFIGURED"
Traceback (most recent call last):
  File "C:\zabbix-agent\scripts\sender_wrapper.py", line 55, in <module>
    send()
  File "C:\zabbix-agent\scripts\sender_wrapper.py", line 44, in send
    stdin=subprocess.PIPE, universal_newlines=True, close_fds=(not isWindows()))
  File "C:\Program Files\Python37\lib\subprocess.py", line 775, in __init__
    restore_signals, start_new_session)
  File "C:\Program Files\Python37\lib\subprocess.py", line 1178, in _execute_child
    startupinfo)
FileNotFoundError: [WinError 2] The system cannot find the file specified

zabbix-smartmontools.conf is commented out.

It looks like you're trying to run zabbix-smartmontools using my branch, using the wrong copy of sender_wrapper.py? The line numbers in the stack trace don't match up with this branch.

[nobody43]

Here's the new version which I mentioned earlier: https://github.com/nobodysu/zabbix-smartmontools/commit/c07098be5b9896959e3fcda56543940a1eae62b6 Without the new file it won't work at all (wrong type for timeout).

[asomers]

Here's the new version which I mentioned earlier: c07098b Without the new file it won't work at all (wrong type for timeout).

I already handle converint timeout to an int. See https://github.com/nobodysu/zabbix-smartmontools/pull/27/files#diff-12ab9df7791f61b30669defc646456baR397 . So what error do you get when running zabbix_get with the testability branch, and what do you mean by "zabbix-smartmontools.conf is commented out."

[nobody43]

1. I think it's str when reading from zabbix-smartmontools.conf. Can't tell what error at the moment, but new version is required anyway cause sender_wrapper.py can receive both str and int as timeout.

2. All lines in zabbix-smartmontools.conf is commented out and this error appears.

[asomers]

1. I think it's `str` when reading from `zabbix-smartmontools.conf`. Can't tell what error at the moment, but new version is required anyway cause sender_wrapper.py can receive both `str` and `int` as timeout.

Notice the difference between config.getint and config.get. That's what converts the string value to an integer. Also, since I turned this project into a python module, sender_wrapper is no longer shared between multiple projects. So we can be assured that it will also receive the timeout as an int.

2. All lines in `zabbix-smartmontools.conf` is commented out and this error appears.

zabbix-smartmontools.conf ought to be fully commented out for most users. If you're seeing an error, it's probably because one of the default values for Windows is wrong. But I can't tell which, because you're mixing files from two branches. Could you please rerun zabbix_get on Windows, using only this branch?

[nobody43]

Alright, I'll test it out once more, give me 24 hours.

Any chance to make the module installation optional? This really bugs me.

[asomers]

Alright, I'll test it out once more, give me 24 hours.

Any chance to make the module installation optional? This really bugs me.

Module installation is key to testability. It's how the unittests find the code. It also allows zabbix-smartmontools to use its own copy of sender_wrapper.py (though I could also accomplish that by simply combining the two files). What I don't like about setuptools is that it doesn't install everything; the user still has to install a few things manually. That can probably be fixed. I'll keep working on it. Ideally the user would be able to pip install zabbix-smartmontools and be ready to go.

Would you accept this PR while I keep working on a better setuptools config, or do you want me to finish it first?

[nobody43]

It could be done in later PR.

[asomers]

I've made some progress on this front. However, the way that setuptools packages modules is incompatible with reexecuting sender_wrapper.py. So I ask that you accept this PR as-is. Then I'll submit a patch that eliminates the reexecution, and then submit the rest of the setuptools changes.