Refactoring for testing #18

exhuma · 2017-05-03T06:06:26Z

This is a large PR. Sorry for the size but I wanted to submit something coherent. I'll try to add line-comments here on GitHub once I created the PR. I hope it's possible.

Most of the code is adding unit-tests. I tried to refrain from making changes to the codes, but there are some exceptions:

A few places contained bugs and I had to fix them in order to get the tests to pass.
Some minor refactoring was necessary in order to make testing possible (or easier).
The shell script is gone and has been replaced with "entry-points". This has several advantages:
- The application can be bundled and install using the standard python method (pip, setup.py, ...).
- Dependencies can be resolved automatically. It's no longer need to bundle third party libraries.
CLI Argument parsing has been split up for the different commands. This separates the commands into separate modules, and the main executable almost builds itself.

I've done my best to keep the commit messages helpful and make atomic commits so it should be a bit easier to follow.

If you have any questions, feel free to ask.

This prepares the overall code structure for entry points including removal of the shell script. Managing the cron-script is not functional yet. Nothing has been tested either. Committing so I can sync with a box where I can test.

Passing script-path is also no longer required.

No longer need the "vendor" copy

Should play niced with virtualenvs (removes the risk of a name-clash).

Having the CRON commands in a separate module reduces the "contact area" between our application and the external dependency. This makes both testing and replacing the dependency easier in the future.

These tests surfaced two bugs which have been fixed with this commit as well.

It was running under the namespace "InfluxdbClient" yet it had nothing to do with it. This makes it more veratile and removed a dependency on InfluxdbClient from `grafana.py`

Using a string literal here leads to incorrect exception messages when the key "group_fields" does not exist (or any other exception on the preceding line). Using a proper code-comment remedies this.

the existing code does not do anything useful.

exhuma · 2017-05-03T06:10:25Z

muninflux

@@ -1,33 +0,0 @@
-#!/usr/bin/env bash


This shell shript has been replaced with munininfluxdb/main.py

exhuma · 2017-05-03T06:12:27Z

munininfluxdb/commands/cron.py

@@ -0,0 +1,110 @@
+from __future__ import print_function


This is one of the subcommands of the main command. It is loaded in munininfluxdb/main.py. In this particular case, it contains the cron code which I extracted from the other files. This way, the cron installation and deinstallation can be run separately (as also mentioned in the readme file).

exhuma · 2017-05-03T06:12:56Z

munininfluxdb/commands/dump.py

@@ -0,0 +1,80 @@
+import logging


This is one of the subcommands of the main command. It is loaded in munininfluxdb/main.py.

exhuma · 2017-05-03T06:13:24Z

munininfluxdb/commands/fetch.py

@@ -1,31 +1,22 @@
 #!/usr/bin/env python


This is one of the subcommands of the main command. It is loaded in munininfluxdb/main.py.

exhuma · 2017-05-03T06:14:48Z

munininfluxdb/commands/fetch.py


-try:


Before touching munin-influxdb I also sent a PR for storable which makes the module available on pypi. This has been accepted and we can install it via setup.py dependencies. The vendor folder is no longer needed.

exhuma · 2017-05-03T06:15:42Z

munininfluxdb/commands/fetch.py

-try:
-    pwd.getpwnam('munin')
-except KeyError:
-    CRON_USER = 'root'


cron has been extracted to a separate sub-command. So all cron related stuff has been removed from this file and moved to commands/cron.py

exhuma · 2017-05-03T06:17:50Z

munininfluxdb/commands/import_.py

@@ -108,22 +91,15 @@ def main(args):
        print("Then we're good! Have a nice day!")


-if __name__ == "__main__":


The "main" part of the commands is handled in munininfluxdb/main.py

exhuma · 2017-05-03T06:18:21Z

munininfluxdb/grafana.py

@@ -112,7 +111,7 @@ def process_graph_types(self, fields):
        if hasArea:
            self.fill = 5
            self.linewidth = 0
-        if hasArea:
+        if hasStack:


This was a bug which surfaced through the unit-tests.

exhuma · 2017-05-03T06:19:26Z

munininfluxdb/grafana.py

@@ -381,7 +380,7 @@ def create_dashboard(self, dashboardJson):
            r.raise_for_status()


-if __name__ == "__main__":
+if __name__ == "__main__": # pragma: no cover


In case you have not worked with the coverage module yet: pragma: no cover prevents lines from being checked for coverage. There were some cases where this made sense to add (lines which are deliberately left out from unit-testing).

exhuma · 2017-05-03T06:20:02Z

munininfluxdb/influxdbclient.py

            self.client, self.valid = None, False
            if not silent:
                print("  {0} Could not connect to database: {1}".format(Symbol.WARN_YELLOW, e))
        except Exception as e:
+            LOG.debug(str(e), exc_info=True)


Not logging anything made debugging very difficult here. This helps a bit.

exhuma · 2017-05-03T06:20:25Z

munininfluxdb/influxdbclient.py

@@ -107,10 +112,6 @@ def list_columns(self, series="/.*/"):

        return res

-    @staticmethod


extracted for easier mocking/patching.

exhuma · 2017-05-03T06:21:02Z

munininfluxdb/influxdbclient.py

-                | ...                  |       |          |          |           |
-                +----------------------+-------+----------+----------+-----------+
-            """
+            # In "group_fields" mode, all fields of a same plugin (ex: system, user, nice, idle... of CPU usage)


Replaced the string-literal with comments. Otherwise it's included in coverage which does not make sense.

exhuma · 2017-05-03T06:21:38Z

munininfluxdb/munin.py

 import os
 import sys
 import pprint

 from utils import ProgressBar, Symbol
 from settings import Settings

-from vendor import storable
+import storable


Again, storable is now available in pypi and has been added to the setup.py dependencies.

exhuma · 2017-05-03T06:22:26Z

munininfluxdb/munin.py


-def discover_from_datafile(settings):
+DataFileLine = namedtuple(


Just a helper to make the code more readable (less use of indices).

exhuma · 2017-05-03T06:22:55Z

munininfluxdb/munin.py

+    'plugin value domain host field property')
+
+
+def parse_datafile_line(line):


Extracted this block of code as separate function which allows us to test it separately.

exhuma · 2017-05-03T06:23:29Z

munininfluxdb/munin.py

+    return DataFileLine(plugin, value, domain, host, field, property)
+
+
+def populate_settings(settings, datafile):


Sane thing. Makes testing easier.

exhuma added 30 commits March 16, 2017 08:09

Preparation for entry-points.

3defc90

This prepares the overall code structure for entry points including removal of the shell script. Managing the cron-script is not functional yet. Nothing has been tested either. Committing so I can sync with a box where I can test.

CRON subcommand added.

5070576

Passing script-path is also no longer required.

Added "crontab" to the dependencides.

23ec84a

No longer need the "vendor" copy

Moved vendor package into munininfluxdb

f9573e3

Should play niced with virtualenvs (removes the risk of a name-clash).

Removed bash script, updated docs.

2a39260

WIP: separate CLI command to dump the RRD files.

3f18fc2

vendor package no longer required.

e3a0a22

Upgraded to latest storable version (Py3)

73e920a

Added egg-info to gitignore

acea451

Moved "cron" code into an external adapter & tests.

85fe31a

Having the CRON commands in a separate module reduces the "contact area" between our application and the external dependency. This makes both testing and replacing the dependency easier in the future.

Extracted logic for parsing datafile lines.

eb24d43

Refactored datafile parsing for testing.

584baad

Added some simple tests.

99d69c1

Added .cache to gitignore

86844d1

Further refactoring of settings generation.

1b96c71

Added test for RRD and XML filename generation

68c93c5

Addded two placeholder tests.

bc2e102

Tests for Grafana panels and queries & bugfixes

aae4511

These tests surfaced two bugs which have been fixed with this commit as well.

Moved "ask_password" from influxdbclient to utils.

88cc02b

It was running under the namespace "InfluxdbClient" yet it had nothing to do with it. This makes it more veratile and removed a dependency on InfluxdbClient from `grafana.py`

Centralized "mock" import (py2/py3)

6fd6253

Making it easier to mock influxdb

f94fe40

Replaced no-op string statement with code-comment.

6a059d8

Using a string literal here leads to incorrect exception messages when the key "group_fields" does not exist (or any other exception on the preceding line). Using a proper code-comment remedies this.

Added a bunch of tests.

49d7ecf

Add logging to unhide some tracebacks.

0ffe3d8

Add TODO markers.

7b425a4

Add test for grafana panel "to_json" method.

1130b47

Additional grafana tests

aa734ae

Add tests for GrafanaApi

8667971

Add setup.py extra requirements for development.

38f7788

Omit unit-tests from coverage.

018a253

exhuma added 11 commits April 24, 2017 08:16

Added tests for dashboard creation.

816a2f2

Fill in test for "Dashboard.generate_simple"

83f390a

Exclude development code from test-coverage

9f2d225

Add test for munin.cleanup.

7480077

Remove rfetch.py

44a2914

the existing code does not do anything useful.

Add tests for "cron" and "dump" commands.

a717976

Add unit-tests for the "fetch" command.

7b797d3

Add tests for the "import" command.

a4de0e6

Prepare main module for unit-testing.

7bce645

Add unit-test for main entry-point.

b904dcf

Remove "raise" which caused nonexecution of code

5d9855f

exhuma commented May 3, 2017

View reviewed changes

muninflux

@@ -1,33 +0,0 @@

#!/usr/bin/env bash

Copy link

Contributor Author

exhuma May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shell shript has been replaced with munininfluxdb/main.py

exhuma commented May 3, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring for testing #18

Refactoring for testing #18

exhuma commented May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

exhuma May 3, 2017

		@@ -108,22 +91,15 @@ def main(args):
		print("Then we're good! Have a nice day!")


		if __name__ == "__main__":

		@@ -107,10 +112,6 @@ def list_columns(self, series="/.*/"):

		return res

		@staticmethod


		def discover_from_datafile(settings):
		DataFileLine = namedtuple(

		'plugin value domain host field property')


		def parse_datafile_line(line):

		return DataFileLine(plugin, value, domain, host, field, property)


		def populate_settings(settings, datafile):


		try:

Refactoring for testing #18

Are you sure you want to change the base?

Refactoring for testing #18

Conversation

exhuma commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment