From 1b7cd56f3916d91afc44db508a64df9f7138c17f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20B=C3=A9tous?= Date: Sat, 2 May 2020 19:40:00 +0200 Subject: [PATCH 1/3] Ignore the log files when they are generated Create two functions in the common module to get the full path of the log files (debug log and reporter log) anywhere in the script. It will avoid duplicate reference to the names of the log files. Then whitelist the log file(s) generated to avoid reporting warnings. Note that log files generated from another run will still be reported. --- kodi_addon_checker/__main__.py | 6 +++--- kodi_addon_checker/check_addon.py | 12 +++++++++++- kodi_addon_checker/check_files.py | 19 ++++++++++++++----- kodi_addon_checker/common/__init__.py | 18 ++++++++++++++++++ kodi_addon_checker/plugins/log_reporter.py | 5 +++-- 5 files changed, 49 insertions(+), 11 deletions(-) diff --git a/kodi_addon_checker/__main__.py b/kodi_addon_checker/__main__.py index f57607de..07f37b5f 100644 --- a/kodi_addon_checker/__main__.py +++ b/kodi_addon_checker/__main__.py @@ -13,7 +13,7 @@ from kodi_addon_checker import __version__, check_addon, ValidKodiVersions from kodi_addon_checker.check_repo import check_repo -from kodi_addon_checker.common import load_plugins +from kodi_addon_checker.common import load_plugins, get_debug_log_path from kodi_addon_checker.config import Config, ConfigManager from kodi_addon_checker.logger import Logger from kodi_addon_checker.record import INFORMATION, PROBLEM, WARNING, Record @@ -75,12 +75,12 @@ def main(): parser.add_argument("--PR", help="Tell if tool is to run on a pull requests or not", action='store_true') parser.add_argument("--allow-folder-id-mismatch", help="Allow the addon's folder name and id to mismatch", action="store_true") - parser.add_argument("--enable-debug-log", help="Enable debug logging to kodi-addon-checker.log", + parser.add_argument("--enable-debug-log", help="Enable debug logging to %s" % os.path.basename(get_debug_log_path()), action="store_true", default=False) ConfigManager.fill_cmd_args(parser) args = parser.parse_args() - log_file_name = os.path.join(os.getcwd(), "kodi-addon-checker.log") + log_file_name = get_debug_log_path() Logger.create_logger(log_file_name, __package__, args.enable_debug_log) all_repo_addons = check_addon.get_all_repo_addons() diff --git a/kodi_addon_checker/check_addon.py b/kodi_addon_checker/check_addon.py index 28c8b0dd..96f1466f 100644 --- a/kodi_addon_checker/check_addon.py +++ b/kodi_addon_checker/check_addon.py @@ -42,6 +42,16 @@ def start(addon_path, args, all_repo_addons, config=None): addon_xml_path = os.path.join(addon_path, "addon.xml") parsed_xml = ET.parse(addon_xml_path).getroot() + # Check if the debug log and/or the reporter log are enabled: it will be + # used to add them to the list of files to ignored + reporter_log_enabled = False + if args.reporter is not None: + if "log" in args.reporter: + reporter_log_enabled = True + debug_log_enabled = False + if args.enable_debug_log is not None: + debug_log_enabled = args.enable_debug_log + # Extract common path from addon paths # All paths will be printed relative to this path common.REL_PATH = os.path.split(addon_path[:-1])[0] @@ -103,7 +113,7 @@ def start(addon_path, args, all_repo_addons, config=None): # General blacklist check_string.find_blacklisted_strings(addon_report, addon_path, [], [], []) - check_files.check_file_whitelist(addon_report, file_index, addon_path) + check_files.check_file_whitelist(addon_report, file_index, addon_path, debug_log_enabled, reporter_log_enabled) else: addon_report.add( Record(INFORMATION, "Addon marked as broken - skipping")) diff --git a/kodi_addon_checker/check_files.py b/kodi_addon_checker/check_files.py index 6aa0000d..3380e9e3 100644 --- a/kodi_addon_checker/check_files.py +++ b/kodi_addon_checker/check_files.py @@ -12,7 +12,7 @@ import xml.etree.ElementTree as ET from . import handle_files -from .common import relative_path +from .common import relative_path, get_debug_log_path, get_reporter_log_path from .common.decorators import posix_only from .record import INFORMATION, PROBLEM, WARNING, Record from .report import Report @@ -111,9 +111,10 @@ def check_for_new_language_directory_structure(report: Report, addon_path: str, "an upper branch/kodi version." % os.path.join(language_path, directory))) -def check_file_whitelist(report: Report, file_index: list, addon_path: str): - """check whether the files present in addon are in whitelist or not - It ignores README.md and .gitignore file +def check_file_whitelist(report: Report, file_index: list, addon_path: str, debug_log_enabled: bool, reporter_log_enabled: bool): + """Check whether the files present in addon are in whitelist or not + It ignores the .gitignore file and the log files generated by the tool + (if the associated arguments are used). :file_index: list having names and path of all the files present in addon :addon_path: path to the addon folder """ @@ -127,12 +128,20 @@ def check_file_whitelist(report: Report, file_index: list, addon_path: str): r"help|list|mpeg|pls|info|ttf|xsp|theme|yaml|dict|crt|ico)?$" ) + # Depending on the arguments used, ignore the log files + files_to_ignore = [] + if reporter_log_enabled: + files_to_ignore.append(get_reporter_log_path()) + if debug_log_enabled: + files_to_ignore.append(get_debug_log_path()) + for file in file_index: file_parts = file["name"].rsplit(".") if len(file_parts) > 1: file_ending = "." + file_parts[len(file_parts) - 1] if re.match(whitelist, file_ending, re.IGNORECASE) is None: - report.add(Record(WARNING, + if os.path.join(file["path"], file["name"]) not in files_to_ignore: + report.add(Record(WARNING, "Found non whitelisted file ending in filename %s" % relative_path(os.path.join(file["path"], file["name"])))) diff --git a/kodi_addon_checker/common/__init__.py b/kodi_addon_checker/common/__init__.py index 00c5a86f..ea169d37 100644 --- a/kodi_addon_checker/common/__init__.py +++ b/kodi_addon_checker/common/__init__.py @@ -49,3 +49,21 @@ def relative_path(file_path): """ path_to_print = file_path[len(REL_PATH):] return ".{}".format(path_to_print) + + +def get_debug_log_path(): + """Return the path of the debug log file (including its name). + + :return: the full path of the debug log file + """ + + return os.path.join(os.getcwd(), "kodi-addon-checker.log") + + +def get_reporter_log_path(): + """Return the path of the reporter log file (including its name). + + :return: the full path of the reporter log file + """ + + return os.path.join(os.getcwd(), "kodi-addon-checker-report.log") diff --git a/kodi_addon_checker/plugins/log_reporter.py b/kodi_addon_checker/plugins/log_reporter.py index 37155b00..a91edbd2 100644 --- a/kodi_addon_checker/plugins/log_reporter.py +++ b/kodi_addon_checker/plugins/log_reporter.py @@ -9,6 +9,7 @@ import logging.handlers import os +from kodi_addon_checker.common import get_reporter_log_path from kodi_addon_checker.report import Record from kodi_addon_checker.reporter import Reporter, reporter @@ -18,14 +19,14 @@ class LogReporter(Reporter): def __init__(self): self.logger = None + self.log_file_path = get_reporter_log_path() def create_logger(self): - log_file_name = os.path.join(os.getcwd(), "kodi-addon-checker-report.log") logger = logging.getLogger("log_reporter") logger.setLevel(logging.DEBUG) logger.propagate = False formatter = logging.Formatter(fmt="%(asctime)s: %(message)s", datefmt="%Y-%m-%d %H:%M:%S") - log_handler = logging.handlers.RotatingFileHandler(log_file_name, encoding="utf-8", mode="w") + log_handler = logging.handlers.RotatingFileHandler(self.log_file_path, encoding="utf-8", mode="w") log_handler.setFormatter(formatter) log_handler.setLevel(logging.DEBUG) logger.addHandler(log_handler) From 2a16084e9c8c3329c03a4723b1fdc02a0bc5379b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20B=C3=A9tous?= Date: Mon, 4 May 2020 22:30:25 +0200 Subject: [PATCH 2/3] Create new test cases to validate the new feature Fix the existing tests by adding the arguments "reporter" and "enable_debug_log" in the stub of argparser (Args()). Create new test cases to validate the log files are correctly ignored or reported. Also improve a bit the code in check_file_whitelist: check if the file needs to be ignored at the very beginning of the loop since the the other check (using regex) requires more lines to be executed. --- kodi_addon_checker/check_files.py | 18 ++-- tests/test_check_addon.py | 2 + tests/test_check_files.py | 136 +++++++++++++++++++++++++++++- 3 files changed, 144 insertions(+), 12 deletions(-) diff --git a/kodi_addon_checker/check_files.py b/kodi_addon_checker/check_files.py index 3380e9e3..a380af89 100644 --- a/kodi_addon_checker/check_files.py +++ b/kodi_addon_checker/check_files.py @@ -130,20 +130,20 @@ def check_file_whitelist(report: Report, file_index: list, addon_path: str, debu # Depending on the arguments used, ignore the log files files_to_ignore = [] - if reporter_log_enabled: - files_to_ignore.append(get_reporter_log_path()) if debug_log_enabled: files_to_ignore.append(get_debug_log_path()) + if reporter_log_enabled: + files_to_ignore.append(get_reporter_log_path()) for file in file_index: - file_parts = file["name"].rsplit(".") - if len(file_parts) > 1: - file_ending = "." + file_parts[len(file_parts) - 1] - if re.match(whitelist, file_ending, re.IGNORECASE) is None: - if os.path.join(file["path"], file["name"]) not in files_to_ignore: + if os.path.join(file["path"], file["name"]) not in files_to_ignore: + file_parts = file["name"].rsplit(".") + if len(file_parts) > 1: + file_ending = "." + file_parts[len(file_parts) - 1] + if re.match(whitelist, file_ending, re.IGNORECASE) is None: report.add(Record(WARNING, - "Found non whitelisted file ending in filename %s" % - relative_path(os.path.join(file["path"], file["name"])))) + "Found non whitelisted file ending in filename %s" % + relative_path(os.path.join(file["path"], file["name"])))) @posix_only diff --git a/tests/test_check_addon.py b/tests/test_check_addon.py index 1e78eb36..9008b3ee 100644 --- a/tests/test_check_addon.py +++ b/tests/test_check_addon.py @@ -11,6 +11,8 @@ class Args(): PR = False allow_folder_id_mismatch = False branch = "krypton" + reporter = None + enable_debug_log = None class TestCheckAddon(unittest.TestCase): diff --git a/tests/test_check_files.py b/tests/test_check_files.py index 38d96a24..f0ba8b28 100644 --- a/tests/test_check_files.py +++ b/tests/test_check_files.py @@ -2,11 +2,10 @@ import unittest from os.path import abspath, dirname, join -from kodi_addon_checker.check_files import check_file_permission +from kodi_addon_checker.check_files import check_file_permission, check_file_whitelist from kodi_addon_checker.handle_files import create_file_index -from kodi_addon_checker.common import load_plugins -from kodi_addon_checker.common import relative_path +from kodi_addon_checker.common import load_plugins, relative_path, get_debug_log_path, get_reporter_log_path from kodi_addon_checker.record import Record from kodi_addon_checker.reporter import ReportManager from kodi_addon_checker.report import Report @@ -39,3 +38,134 @@ def test_check_file_permission_is_None(self): path = join(HERE, 'test_data', 'Non-Executable_file') file_index = create_file_index(path) self.assertIsNone(check_file_permission(self.report, file_index)) + + +class TestCheckFileWhitelist(unittest.TestCase): + """Use cases: + - Ignore the log files if they are generated in the folder that is + being checked + - Report the files which have the same name as the log files but that + are not generated in this run of the checker + - Report the log files in the current folder if one/both reporter(s) + is/are disabled + """ + + def setUp(self): + # General initializaiton + load_plugins() + ReportManager.enable(["array"]) + self.report = Report("") + + # Create a dummy path not containing ".module." so that the check is + # not skipped + self.addon_path = "script.test" + + # Get the name of the log files + self.debug_log_filename = os.path.basename(get_debug_log_path()) + self.reporter_log_filename = os.path.basename(get_reporter_log_path()) + + # Create the file index from the "script.test" folder + script_test_path = join(HERE, "..", 'script.test') + self.file_index = create_file_index(script_test_path) + # The log files are supposed to be generated in the folder being + # checked so add them to the file index but stub the path to be in the + # current folder (since the log files are supposed to be generated there) + self.file_index.append({"path": os.path.dirname(get_debug_log_path()), + "name": self.debug_log_filename}) + self.file_index.append({"path": os.path.dirname(get_reporter_log_path()), + "name": self.reporter_log_filename}) + + def check_if_log_files_were_reported(self): + """Helper function to check if the log files were reported.""" + + # Initialize the returned values + debug_log_reported = False + reporter_log_reported = False + + # For each records generated check if the name of the log files was mentioned + for r in ReportManager.getEnabledReporters()[0].reports: + # Convert the record to a string + record = Record.__str__(r) + # Check if the log files name is in the record + if self.debug_log_filename in record: + debug_log_reported = True + elif self.reporter_log_filename in record: + reporter_log_reported = True + # Note: "elif" is used to improve performance because we assume only 1 + # log file may exist in a give record + + # Resert the report so that the next test case starts fresh + ReportManager.getEnabledReporters()[0].reports = [] + + return debug_log_reported, reporter_log_reported + + def test_check_file_whitelist_both_log_files_enabled(self): + """Test "check_file_whitelist" when both log files are enabled. + + The log files must be ignored because they are being generated in the + folder that is being analyzed. + """ + + # Call the function with both logs enabled + check_file_whitelist(self.report, self.file_index, self.addon_path, True, True) + + # Check if the log files were reported + debug_log_reported, reporter_log_reported = self.check_if_log_files_were_reported() + + # Only the reporter log file must have been reported + self.assertFalse(debug_log_reported) + self.assertFalse(reporter_log_reported) + + def test_check_file_whitelist_debug_log_file_enabled(self): + """Test "check_file_whitelist" when only the debug log is enabled. + + The debug log file must be ignored but if a file having the same as the + reporter log exists it must be reported. + """ + + # Call the function with only the debug log enabled + check_file_whitelist(self.report, self.file_index, self.addon_path, True, False) + + # Check if the log files were reported + debug_log_reported, reporter_log_reported = self.check_if_log_files_were_reported() + + # Only the reporter log file must have been reported + self.assertFalse(debug_log_reported) + self.assertTrue(reporter_log_reported) + + def test_check_file_whitelist_reporter_log_file_enabled(self): + """Test "check_file_whitelist" when only the reporter log is enabled. + + The reporter log file must be ignored but if a file having the same as + the reporter log exists it must be reported. + """ + + # Call the function with only the reporter log enabled + check_file_whitelist(self.report, self.file_index, self.addon_path, False, True) + + # Check if the log files were reported + debug_log_reported, reporter_log_reported = self.check_if_log_files_were_reported() + + # Only the reporter log file must have been reported + self.assertTrue(debug_log_reported) + self.assertFalse(reporter_log_reported) + + def test_check_file_whitelist_log_files_disabled(self): + """Test "check_file_whitelist" when both log files are disabled. + + The log files must be reported because they are being generated in the + folder that is being analyzed. + This use case validates also when the log files are generated out of + the analyzed folder but there are files with the same names in the + analyzed folder. + """ + + # Call the function with only the reporter log enabled + check_file_whitelist(self.report, self.file_index, self.addon_path, False, False) + + # Check if the log files were reported + debug_log_reported, reporter_log_reported = self.check_if_log_files_were_reported() + + # Only the reporter log file must have been reported + self.assertTrue(debug_log_reported) + self.assertTrue(reporter_log_reported) From 9c52179bd1e207d350ad1ee2add642fe5418bcf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20B=C3=A9tous?= Date: Thu, 7 May 2020 13:29:09 +0200 Subject: [PATCH 3/3] Fix pylint errors --- kodi_addon_checker/__main__.py | 3 ++- kodi_addon_checker/check_addon.py | 3 ++- kodi_addon_checker/check_files.py | 6 +++++- kodi_addon_checker/plugins/log_reporter.py | 1 - tests/test_check_files.py | 2 +- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/kodi_addon_checker/__main__.py b/kodi_addon_checker/__main__.py index 07f37b5f..d59783a4 100644 --- a/kodi_addon_checker/__main__.py +++ b/kodi_addon_checker/__main__.py @@ -75,7 +75,8 @@ def main(): parser.add_argument("--PR", help="Tell if tool is to run on a pull requests or not", action='store_true') parser.add_argument("--allow-folder-id-mismatch", help="Allow the addon's folder name and id to mismatch", action="store_true") - parser.add_argument("--enable-debug-log", help="Enable debug logging to %s" % os.path.basename(get_debug_log_path()), + parser.add_argument("--enable-debug-log", + help="Enable debug logging to %s" % os.path.basename(get_debug_log_path()), action="store_true", default=False) ConfigManager.fill_cmd_args(parser) args = parser.parse_args() diff --git a/kodi_addon_checker/check_addon.py b/kodi_addon_checker/check_addon.py index 96f1466f..3f47b7e1 100644 --- a/kodi_addon_checker/check_addon.py +++ b/kodi_addon_checker/check_addon.py @@ -113,7 +113,8 @@ def start(addon_path, args, all_repo_addons, config=None): # General blacklist check_string.find_blacklisted_strings(addon_report, addon_path, [], [], []) - check_files.check_file_whitelist(addon_report, file_index, addon_path, debug_log_enabled, reporter_log_enabled) + check_files.check_file_whitelist(addon_report, file_index, addon_path, + debug_log_enabled, reporter_log_enabled) else: addon_report.add( Record(INFORMATION, "Addon marked as broken - skipping")) diff --git a/kodi_addon_checker/check_files.py b/kodi_addon_checker/check_files.py index a380af89..0269bacc 100644 --- a/kodi_addon_checker/check_files.py +++ b/kodi_addon_checker/check_files.py @@ -111,7 +111,11 @@ def check_for_new_language_directory_structure(report: Report, addon_path: str, "an upper branch/kodi version." % os.path.join(language_path, directory))) -def check_file_whitelist(report: Report, file_index: list, addon_path: str, debug_log_enabled: bool, reporter_log_enabled: bool): +def check_file_whitelist(report: Report, + file_index: list, + addon_path: str, + debug_log_enabled: bool, + reporter_log_enabled: bool): """Check whether the files present in addon are in whitelist or not It ignores the .gitignore file and the log files generated by the tool (if the associated arguments are used). diff --git a/kodi_addon_checker/plugins/log_reporter.py b/kodi_addon_checker/plugins/log_reporter.py index a91edbd2..67ff6535 100644 --- a/kodi_addon_checker/plugins/log_reporter.py +++ b/kodi_addon_checker/plugins/log_reporter.py @@ -7,7 +7,6 @@ """ import logging import logging.handlers -import os from kodi_addon_checker.common import get_reporter_log_path from kodi_addon_checker.report import Record diff --git a/tests/test_check_files.py b/tests/test_check_files.py index f0ba8b28..14758e23 100644 --- a/tests/test_check_files.py +++ b/tests/test_check_files.py @@ -73,7 +73,7 @@ def setUp(self): self.file_index.append({"path": os.path.dirname(get_debug_log_path()), "name": self.debug_log_filename}) self.file_index.append({"path": os.path.dirname(get_reporter_log_path()), - "name": self.reporter_log_filename}) + "name": self.reporter_log_filename}) def check_if_log_files_were_reported(self): """Helper function to check if the log files were reported."""