Story SW-300: implemented reviewer suggestions

CMake/LofarPackageList.cmake

 # - Create for each LOFAR package a variable containing the absolute path to
 # its source directory. 
 #
-# Generated by gen_LofarPackageList_cmake.sh at Sun May 13 19:07:03 CEST 2018
+# Generated by gen_LofarPackageList_cmake.sh at Mon Jun 11 14:09:52 CEST 2018
 #
 #                      ---- DO NOT EDIT ----
 #

LCU/Maintenance/MDB_tools/CMakeLists.txt

 # $Id$
 
+
+lofar_package(MDB_tools 1.0)
+
 lofar_find_package(Python 2.6 REQUIRED)
 include(PythonInstall)
 include(FindPythonModule)

LCU/Maintenance/MDB_tools/cli/mdb_loader.py

-#! /usr/bin/env python
+# coding=utf-8
+"""
+This program is meant to load the station tests and RTSM present in a certain directory to the database
+"""
 import argparse
 import logging
 import re
 import sys
 from glob import glob
-from os.path import isfile
+import os.path as path
 
 import requests
 
 logger = logging.getLogger('mdb_loader')
 
-"""
-This program is meant to load the station tests and RTSM present in a certain directory to the database
-"""
-
 
 def setup_argument_parser():
     """
     Set up the argument parser and returns it
-    :return:
+    :return: ArgumentParser instance filled with the command arguments
     """
     parser = argparse.ArgumentParser(prog='mdb_loader')
     parser.add_argument('path', help='format or path format of the files to load. ex. /where/is/stored/*.dat')
@@ -28,24 +27,30 @@ def setup_argument_parser():
     return parser
 
 
-def obtain_file_list(path):
+def obtain_file_list(file_path):
     """
-    Searches in the directory the list of files that satisfy a certain pattern
-    :param path: where to look the files
-    :return:
+    Searches in the directory the list of files which path satisfy the pattern specified in file_path
+    :param file_path: strings that represents the path pattern to be satisfied
+    :return: a list of file paths
     """
-    logger.info('listing file in %s', path)
-    file_list = list(filter(isfile, glob(path)))
+    logger.info('listing file in %s', file_path)
+    file_list = list(filter(path.isfile, glob(file_path)))
     logger.debug('found files: %s', file_list)
     return file_list
 
 
-def read_file_to_dict(file_path):
+def read_stationtest_rtsm_output_to_dict(file_path):
     """
-    Reads the content of the file into a dict {'content': [file_content]}. If the file contains a RTSM
-    it also append the station name -> ex. {'content': [file_content], 'station_name': CS001C}
+    Reads the content of the stationtest or RTSM output file into a dict
+    -> ex. {'content': [file_content]}.
+    If the file contains a RTSM it also append the station name
+    -> ex. {'content': [file_content], 'station_name': CS001C}
     :param file_path: path to the file
-    :return:
+    :return: return a dict containing a content field if the file refers to a station test ->
+    ex. {'content': [file_content]}
+    or if the file_path refers to a RTSM it returns a dict containing the content of the file and the station name
+    to which the test refers
+    ex. {'content': [file_content], 'station_name'}
     """
     rtsm_filename_pattern = '(?P<station_name>\w*)_\d*_\d*.dat'
     try:
@@ -62,12 +67,12 @@ def read_file_to_dict(file_path):
 
 def is_station_test(content):
     """
-    Check if the first 10 lines of the content file have the structure of a station test
+    Check if the content of the file have the structure of a station test
     :param content: list of strings representing the content of the file
-    :return:
+    :return: True if the content refers to a station test
     """
     pattern = r'(?:\d{8})(?:\,.*)*'
-    for line in content[:10]:
+    for line in content:
         if not re.match(pattern, line):
             return False
     return True
@@ -75,31 +80,27 @@ def is_station_test(content):
 
 def is_rtsm_test(content):
     """
-    Check if the first 10 lines of the content file have the structure of a station test
+    Check if the content of the file have the structure of a RTSM
     :param content: list of strings representing the content of the file
-    :return:
+    :return: True if the content refers to a RTSM
     """
     pattern = r'(?:#\.*)|(?:[^0-9\,]*=.*)'
-    for line in content[:10]:
+    for line in content:
         if line is '':
             continue
         if not re.match(pattern, line):
-            logger.debug('cannot match line %s',line)
             return False
     return True
 
 
-def perform_request(address, content):
+def send_stationtest_rtsm_content_to_address(address, content):
     """
-    Send the content to the web site API
-    :param args: object
-    :param content:
-    :return:
+    Send the stationtest RTSM content to the web site API at the given address
+    :param address: url where the API is hosted
+    :param content: content of the API call
+    :return: True if the request was successful False otherwise
     """
-    content_is_station_test = is_station_test(content)
-    content_is_rtsm = is_rtsm_test(content)
-    full_address = '/'.join([address, create_query_string(is_station_test=content_is_station_test,
-                                                          is_rtsm=content_is_rtsm)])
+    full_address = '/'.join([address, compose_api_url_for_given_test_type(content)])
     logging.info('performing request to address %s', full_address)
 
     logging.debug('request content %s', content)
@@ -114,40 +115,45 @@ def perform_request(address, content):
         return False
 
 
-def create_query_string(is_station_test=False, is_rtsm=False):
+def compose_api_url_for_given_test_type(content):
     """
-    Create the query strings from the program arguments.
-    In case they are both set to true or none it raises a ValueError exception.
-    :param is_station_test: is the content station test
-    :param is_rtsm: is the content RTSM
+    Create the url from the content type
     :return: the query needed to insert the raw results
     """
-    if is_station_test and (not is_rtsm):
+    if is_station_test(content):
         query = 'stationtests/insert_raw'
-    elif is_station_test:
-        raise ValueError('The path format cannot refer both to RTSM and station test. ' +
-                         'Please specify either --station_test or --rtsm')
-    elif is_rtsm:
+    elif is_rtsm_test(content):
         query = 'rtsm/insert_raw'
     else:
-        raise ValueError('Please specify either --station_test or --rtsm')
+        raise ValueError('The path format cannot refer both to RTSM and station test. ' +
+                         'Please specify either --station_test or --rtsm')
     return query
 
 
-def main():
+def obtain_stationtest_file_list_and_process(file_path_pattern, address):
     """
-    Main entry point of the program
+    Finds all the stationtest files which path satisfy the file_path_pattern and send them to address that implement the
+    MaintenanceDB API
+    :param file_path_pattern: file path pattern
+    :param address: address of the REST API
     """
-    parser = setup_argument_parser()
-    args = parser.parse_args()
-
-    for filename in obtain_file_list(args.path):
-        logging.info('processing file %s', filename)
-        content = read_file_to_dict(filename, args.rtsm)
+    for file_path in obtain_file_list(file_path_pattern):
+        logging.info('processing file %s', file_path)
+        content = read_stationtest_rtsm_output_to_dict(file_path)
         if content is None:
+            logger.error('error processing file %s', file_path)
             continue
-        if perform_request(args, content):
-            logging.info('file %s processed', filename)
+        if send_stationtest_rtsm_content_to_address(address, content):
+            logger.info('file %s processed', file_path)
         else:
-            logging.error('error on file %s', filename)
+            logger.error('error on file %s', file_path)
             sys.exit(1)
+
+
+def main():
+    """
+    Main entry point of the program
+    """
+    parser = setup_argument_parser()
+    args = parser.parse_args()
+    obtain_stationtest_file_list_and_process(args.path, args.address)
\ No newline at end of file

LCU/Maintenance/MDB_tools/test/t_mdb_loader.py

 import unittest
 
-from mock import patch, MagicMock, Mock
+from mock import patch, MagicMock, Mock, call
 
 from lofar.maintenance.utils.cli.mdb_loader import *
 
@@ -41,58 +41,57 @@ class TESTMDBLoader(unittest.TestCase):
         self.assertFalse(is_rtsm_test(rtsm_test_content))
 
     @patch('requests.post')
-    def test_perform_request_success(self, mocked_post):
+    def test_send_stationtest_rtsm_content_to_address_success(self, mocked_post):
         station_test_content = load_station_test()
         address = 'http://testaddress'
         response = Mock(status_code=200, reason='ALL GOOD', content='YEAH')
         mocked_post.return_value = response
 
-        full_address = '/'.join([address, create_query_string(is_station_test=True)])
-        result = perform_request('http://testaddress', station_test_content)
+        full_address = '/'.join([address, compose_api_url_for_given_test_type(station_test_content)])
+        result = send_stationtest_rtsm_content_to_address('http://testaddress', station_test_content)
         self.assertTrue(mocked_post.called)
         mocked_post.assert_called_with(full_address, data=station_test_content)
         self.assertTrue(result)
 
     @patch('requests.post')
-    def test_perform_request_failure(self, mocked_post):
+    def test_send_stationtest_rtsm_content_to_address_failure(self, mocked_post):
         station_test_content = load_station_test()
         address = 'http://testaddress'
         response = Mock(status_code=400, reason='Bad Request', content='Sorry')
         mocked_post.return_value = response
 
-        full_address = '/'.join([address, create_query_string(is_station_test=True)])
-        result = perform_request('http://testaddress', station_test_content)
+        full_address = '/'.join([address, compose_api_url_for_given_test_type(station_test_content)])
+        result = send_stationtest_rtsm_content_to_address('http://testaddress', station_test_content)
         self.assertTrue(mocked_post.called)
         mocked_post.assert_called_with(full_address, data=station_test_content)
         self.assertFalse(result)
 
-    def test_create_query_string_is_rtsm(self):
-        self.assertEqual(create_query_string(is_rtsm=True), 'rtsm/insert_raw')
-
-    def test_create_query_string_is_station_test(self):
-        self.assertEqual(create_query_string(is_station_test=True), 'stationtests/insert_raw')
+    def test_compose_api_url_for_given_test_type_is_rtsm(self):
+        rtsm_test_content = load_rtsm_test()
+        self.assertEqual(compose_api_url_for_given_test_type(rtsm_test_content), 'rtsm/insert_raw')
 
-    def test_create_query_string_raises_cannot_be_both(self):
-        with self.assertRaises(ValueError):
-            create_query_string(is_station_test=True, is_rtsm=True)
+    def test_compose_api_url_for_given_test_type_is_station_test(self):
+        station_test_content = load_station_test()
+        self.assertEqual(compose_api_url_for_given_test_type(station_test_content), 'stationtests/insert_raw')
 
-    def test_create_query_string_raises_has_to_be_either(self):
+    def test_compose_api_url_for_given_test_type_raises_content_unrecognized(self):
         with self.assertRaises(ValueError):
-            create_query_string(is_station_test=False, is_rtsm=False)
+            compose_api_url_for_given_test_type(['Neque porro quisquam est qui dolorem ipsum quia dolor sit amet',
+                                                 'consectetur, adipisci velit...'])
 
-    @patch('lofar.maintenance.utils.cli.mdb_loader.isfile')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.path')
     @patch('lofar.maintenance.utils.cli.mdb_loader.glob')
-    def test_obtain_file_path_list(self, glob_mock, isfile_mock):
+    def test_obtain_file_path_list(self, glob_mock, path_mock):
         path = 'test'
         glob_return_values = ['test', 'test1', 'test2']
         glob_mock.return_value = glob_return_values
         obtain_file_list(path)
         glob_mock.assert_called_with(path)
-        self.assertEqual(isfile_mock.call_count, 3)
-        isfile_mock.assert_called_with('test2')
+        self.assertEqual(path_mock.isfile.call_count, 3)
+        path_mock.isfile.assert_called_with('test2')
 
     @patch('lofar.maintenance.utils.cli.mdb_loader.open', create=True)
-    def test_read_file_into_dict_rtsm_success(self, open_mock):
+    def test_read_stationtest_rtsm_output_to_dict_rtsm_success(self, open_mock):
         file_name = 'CS001C_12345_20180305.dat'
         file_content = 'testline1\ntestline2'
         open_mock.return_value = MagicMock()
@@ -100,13 +99,13 @@ class TESTMDBLoader(unittest.TestCase):
 
         file_handle.read.return_value = file_content
 
-        result = read_file_to_dict(file_name)
+        result = read_stationtest_rtsm_output_to_dict(file_name)
         open_mock.assert_called_with(file_name, 'r')
         file_handle.read.assert_called()
         self.assertEqual(result, dict(content=file_content, station_name='CS001C'))
 
     @patch('lofar.maintenance.utils.cli.mdb_loader.open', create=True)
-    def test_read_file_into_dict_rtsm_failure(self, open_mock):
+    def test_read_stationtest_rtsm_output_to_dict_rtsm_failure(self, open_mock):
         file_name = 'CS001C_12345_20180305.dat'
         open_mock.return_value = MagicMock()
 
@@ -114,14 +113,13 @@ class TESTMDBLoader(unittest.TestCase):
 
         file_handle.read.side_effect = IOError('Cannot read file')
 
-        result = read_file_to_dict(file_name)
+        result = read_stationtest_rtsm_output_to_dict(file_name)
         open_mock.assert_called_with(file_name, 'r')
         file_handle.read.assert_called()
         self.assertIsNone(result)
 
-
     @patch('lofar.maintenance.utils.cli.mdb_loader.open', create=True)
-    def test_read_file_into_dict_station_test_success(self, open_mock):
+    def test_read_stationtest_rtsm_output_to_dict_station_test_success(self, open_mock):
         file_name = 'CS031C_L2_StationTestHistory.csv'
         file_content = 'testline1\ntestline2'
         open_mock.return_value = MagicMock()
@@ -129,11 +127,91 @@ class TESTMDBLoader(unittest.TestCase):
 
         file_handle.read.return_value = file_content
 
-        result = read_file_to_dict(file_name)
+        result = read_stationtest_rtsm_output_to_dict(file_name)
         open_mock.assert_called_with(file_name, 'r')
         file_handle.read.assert_called()
         self.assertEqual(result, dict(content=file_content))
 
+    @patch('lofar.maintenance.utils.cli.mdb_loader.logger')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.obtain_file_list')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.read_stationtest_rtsm_output_to_dict')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.send_stationtest_rtsm_content_to_address')
+    def test_obtain_stationtest_file_list_and_process_all_working(self, send_content_to_address_mock,
+                                                                  read_tests_mock, obtain_file_list_mock, logger_mock):
+        path_list = ['path1', 'path2', 'path3']
+        obtain_file_list_mock.return_value = path_list
+        address = 'test_address'
+        file_pattern = 'data/*history*.csv'
+        mock_file_content = {'content': [{'item': 'key'}]}
+        read_tests_mock.return_value = mock_file_content
+
+        send_content_to_address_mock.return_value = True
+
+        obtain_stationtest_file_list_and_process(file_pattern, address)
+        obtain_file_list_mock.assert_any_call(file_pattern)
+
+        for file_path in path_list:
+            read_tests_mock.assert_any_call(file_path)
+            send_content_to_address_mock.assert_any_call(address, mock_file_content)
+            logger_mock.info.assert_any_call('file %s processed', file_path)
+
+    @patch('lofar.maintenance.utils.cli.mdb_loader.logger')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.obtain_file_list')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.read_stationtest_rtsm_output_to_dict')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.send_stationtest_rtsm_content_to_address')
+    def test_obtain_stationtest_file_list_and_process_found_irregular_test_file(self, send_content_to_address_mock,
+                                                                                read_tests_mock, obtain_file_list_mock,
+                                                                                logger_mock):
+        path_list = ['path1', 'path2', 'path3']
+        obtain_file_list_mock.return_value = path_list
+        address = 'test_address'
+        file_pattern = 'data/*history*.csv'
+        mock_file_content0 = {'content': [{'item0': 'key0'}]}
+        mock_file_content1 = {'content': [{'item1': 'key1'}]}
+
+        read_side_effect = [mock_file_content0, None, mock_file_content1]
+        read_tests_mock.side_effect = read_side_effect
+
+        send_content_to_address_mock.return_value = True
+
+        obtain_stationtest_file_list_and_process(file_pattern, address)
+        obtain_file_list_mock.assert_any_call(file_pattern)
+
+        for file_path, read_return_value in zip(path_list, read_side_effect):
+            read_tests_mock.assert_any_call(file_path)
+
+        send_content_to_address_mock.assert_has_calls([call(address, mock_file_content0),
+                                                       call(address, mock_file_content1)])
+        logger_mock.info.assert_has_calls([call('file %s processed', path_list[0]),
+                                           call('file %s processed', path_list[2])])
+
+        logger_mock.error.assert_has_calls([call('error processing file %s', path_list[1])])
+
+    @patch('lofar.maintenance.utils.cli.mdb_loader.logger')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.obtain_file_list')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.read_stationtest_rtsm_output_to_dict')
+    @patch('lofar.maintenance.utils.cli.mdb_loader.send_stationtest_rtsm_content_to_address')
+    def test_obtain_stationtest_file_list_and_process_sending_content_failure(self, send_content_to_address_mock,
+                                                                                read_tests_mock, obtain_file_list_mock,
+                                                                                logger_mock):
+        path_list = ['path1', 'path2', 'path3']
+        obtain_file_list_mock.return_value = path_list
+        address = 'test_address'
+        file_pattern = 'data/*history*.csv'
+        mock_file_content0 = {'content': [{'item0': 'key0'}]}
+
+        read_side_effect = [mock_file_content0]
+        read_tests_mock.side_effect = read_side_effect
+
+        send_content_to_address_mock.return_value = False
+        with self.assertRaises(SystemExit):
+            obtain_stationtest_file_list_and_process(file_pattern, address)
+        obtain_file_list_mock.assert_any_call(file_pattern)
+
+        read_tests_mock.assert_called_once_with(path_list[0])
+
+        send_content_to_address_mock.assert_called_once_with(address, mock_file_content0)
+
 
 if __name__ == '__main__':
     logging.basicConfig(format="%(asctime)s %(levelname)s: %(message)s", level=logging.DEBUG)

LCU/Maintenance/MDB_tools/test/t_probe_mdb.py

+import logging
 import unittest
+from datetime import datetime
 
 from mock import patch, Mock
 
-from lofar.maintenance.utils.cli.probe_mdb import *
+from lofar.maintenance.utils.cli.probe_mdb import get_query_string_for_time_limit, TestType, \
+    get_query_string_for_type, CACHING_SIZE, get_query_string_for_station_name, query_station_test, query_rtsm, \
+    reformat_values, sort_component_errors, rtsm_sort_errors, perform_query
 
 logger = logging.getLogger(__name__)