Updated merlin server unit testing (#372)

* Added additional tests for merlin server in test definitions
* Fixed directory change to create a new directory if one doesn't exist
* Updated redis version to provide acl user channel support

CHANGELOG.md

@@ -10,13 +10,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Added lgtm.com Badge for README.md
 - More fixes for lgtm checks.
 - Added merlin server command as a container option for broker and results_backend servers.
+- Added merlin server unit tests to test exisiting merlin server commands.
 - Added the flux_exec batch argument to allow for flux exec arguments,
   e.g. flux_exec: flux exec -r "0-1" to run celery workers only on
   ranks 0 and 1 of a multi-rank allocation
+- Added additional argument in test definitions to allow for a "cleanup" command
 
 ### Changed
 - Rename lgtm.yml to .lgtm.yml
 - Changed "default" user password to be "merlin_password" as default.
+- Update requirements to require redis 4.3.4 for acl user channel support
 
 ### Fixed
 - Fixed return values from scripts with main() to fix testing errors. 

merlin/server/server_util.py

@@ -332,14 +332,17 @@ def get_password(self) -> str:
     def set_directory(self, directory: str) -> bool:
         if directory is None:
             return False
+        if not os.path.exists(directory):
+            os.mkdir(directory)
+            LOG.info(f"Created directory {directory}")
         # Validate the directory input
         if os.path.exists(directory):
             # Set the save directory to the redis config
             if not self.set_config_value("dir", directory):
                 LOG.error("Unable to set directory for redis config")
                 return False
         else:
-            LOG.error("Directory given does not exist.")
+            LOG.error(f"Directory {directory} given does not exist and could not be created.")
             return False
         LOG.info(f"Directory is set to {directory}")
         return True
@@ -413,7 +416,7 @@ def set_append_file(self, file: str) -> bool:
         if file is None:
             return False
         # Set the append file in the redis config
-        if not self.set_config_value("appendfilename", file):
+        if not self.set_config_value("appendfilename", f'"{file}"'):
             LOG.error("Unable to set append filename.")
             return False
         LOG.info(f"Append file is set to {file}")
@@ -431,24 +434,33 @@ class User:
         status = "on"
         hash_password = hashlib.sha256(b"password").hexdigest()
         keys = "*"
+        channels = "*"
         commands = "@all"
 
-        def __init__(self, status="on", keys="*", commands="@all", password=None) -> None:
+        def __init__(self, status="on", keys="*", channels="*", commands="@all", password=None) -> None:
             self.status = status
             self.keys = keys
+            self.channels = channels
             self.commands = commands
             if password is not None:
                 self.set_password(password)
 
         def parse_dict(self, dict: dict) -> None:
             self.status = dict["status"]
             self.keys = dict["keys"]
+            self.channels = dict["channels"]
             self.commands = dict["commands"]
             self.hash_password = dict["hash_password"]
 
         def get_user_dict(self) -> dict:
             self.status = "on"
-            return {"status": self.status, "hash_password": self.hash_password, "keys": self.keys, "commands": self.commands}
+            return {
+                "status": self.status,
+                "hash_password": self.hash_password,
+                "keys": self.keys,
+                "channels": self.channels,
+                "commands": self.commands,
+            }
 
         def __repr__(self) -> str:
             return str(self.get_user_dict())
@@ -482,10 +494,10 @@ def write(self) -> None:
         with open(self.filename, "w") as f:
             yaml.dump(data, f, yaml.Dumper)
 
-    def add_user(self, user, status="on", keys="*", commands="@all", password=None) -> bool:
+    def add_user(self, user, status="on", keys="*", channels="*", commands="@all", password=None) -> bool:
         if user in self.users:
             return False
-        self.users[user] = self.User(status, keys, commands, password)
+        self.users[user] = self.User(status, keys, channels, commands, password)
         return True
 
     def set_password(self, user: str, password: str):
@@ -510,6 +522,7 @@ def apply_to_redis(self, host: str, port: int, password: str) -> None:
                     hashed_passwords=[f"+{data.hash_password}"],
                     enabled=(data.status == "on"),
                     keys=data.keys,
+                    channels=data.channels,
                     commands=[f"+{data.commands}"],
                 )
 

requirements/release.txt

@@ -9,3 +9,4 @@ parse
 psutil>=5.1.0
 pyyaml>=5.1.2
 tabulate
+redis>=4.3.4

tests/integration/conditions.py

@@ -238,3 +238,65 @@ def passes(self):
         if self.negate:
             return not self.is_within()
         return self.is_within()
+
+
+class PathExists(Condition):
+    """
+    A condition for checking if a path to a file or directory exists
+    """
+
+    def __init__(self, pathname) -> None:
+        self.pathname = pathname
+
+    def path_exists(self) -> bool:
+        return os.path.exists(self.pathname)
+
+    def __str__(self) -> str:
+        return f"{__class__.__name__} expected to find file or directory at {self.pathname}"
+
+    @property
+    def passes(self):
+        return self.path_exists()
+
+
+class FileHasRegex(Condition):
+    """
+    A condition that some body of text within a file
+    MUST match a given regular expression.
+    """
+
+    def __init__(self, filename, regex) -> None:
+        self.filename = filename
+        self.regex = regex
+
+    def contains(self) -> bool:
+        try:
+            with open(self.filename, "r") as f:
+                filetext = f.read()
+            return self.is_within(filetext)
+        except Exception:
+            return False
+
+    def is_within(self, text):
+        return search(self.regex, text) is not None
+
+    def __str__(self) -> str:
+        return f"{__class__.__name__} expected to find {self.regex} regex match within {self.filename} file but no match was found"
+
+    @property
+    def passes(self):
+        return self.contains()
+
+
+class FileHasNoRegex(FileHasRegex):
+    """
+    A condition that some body of text within a file
+    MUST NOT match a given regular expression.
+    """
+
+    def __str__(self) -> str:
+        return f"{__class__.__name__} expected to find {self.regex} regex to not match within {self.filename} file but a match was found"
+
+    @property
+    def passes(self):
+        return not self.contains()

tests/integration/run_tests.py

@@ -78,6 +78,12 @@ def run_single_test(name, test, test_label="", buffer_length=50):
             info["violated_condition"] = (condition, i, len(conditions))
             break
 
+    if len(test) == 4:
+        end_process = Popen(test[3], stdout=PIPE, stderr=PIPE, shell=True)
+        end_stdout, end_stderr = end_process.communicate()
+        info["end_stdout"] = end_stdout
+        info["end_stderr"] = end_stderr
+
     return passed, info
 
 
@@ -139,7 +145,11 @@ def run_tests(args, tests):
         n_to_run = 0
         selective = True
         for test_id, test in enumerate(tests.values()):
-            if len(test) == 3 and test[2] == "local":
+            # Ensures that test definitions are atleast size 3.
+            # 'local' variable is stored in 3rd element of the test definitions,
+            # but an optional 4th element can be provided for an ending command
+            # to be ran after all checks have been made.
+            if len(test) >= 3 and test[2] == "local":
                 args.ids.append(test_id + 1)
                 n_to_run += 1
 

tests/integration/test_definitions.py

@@ -1,9 +1,19 @@
-from conditions import HasRegex, HasReturnCode, ProvenanceYAMLFileHasRegex, StepFileExists, StepFileHasRegex
+from conditions import (
+    FileHasNoRegex,
+    FileHasRegex,
+    HasRegex,
+    HasReturnCode,
+    PathExists,
+    ProvenanceYAMLFileHasRegex,
+    StepFileExists,
+    StepFileHasRegex,
+)
 
 from merlin.utils import get_flux_cmd
 
 
 OUTPUT_DIR = "cli_test_studies"
+CLEAN_MERLIN_SERVER = "rm -rf appendonly.aof dump.rdb merlin_server/"
 
 
 def define_tests():
@@ -45,18 +55,80 @@ def define_tests():
             "local",
         ),
     }
-    server_tests = {
-        "merlin server init": ("merlin server init", HasRegex(".*successful"), "local"),
+    server_basic_tests = {
+        "merlin server init": (
+            "merlin server init",
+            HasRegex(".*successful"),
+            "local",
+            CLEAN_MERLIN_SERVER,
+        ),
         "merlin server start/stop": (
-            "merlin server start; merlin server status; merlin server stop",
+            """merlin server init;
+            merlin server start;
+            merlin server status;
+            merlin server stop;""",
+            [
+                HasRegex("Server started with PID [0-9]*"),
+                HasRegex("Merlin server is running"),
+                HasRegex("Merlin server terminated"),
+            ],
+            "local",
+            CLEAN_MERLIN_SERVER,
+        ),
+        "merlin server restart": (
+            """merlin server init;
+            merlin server start;
+            merlin server restart;
+            merlin server status;
+            merlin server stop;""",
             [
                 HasRegex("Server started with PID [0-9]*"),
                 HasRegex("Merlin server is running"),
                 HasRegex("Merlin server terminated"),
             ],
             "local",
+            CLEAN_MERLIN_SERVER,
+        ),
+    }
+    server_config_tests = {
+        "merlin server change config": (
+            """merlin server init;
+            merlin server config -p 8888 -pwd new_password -d ./config_dir -ss 80 -sc 8 -sf new_sf -am always -af new_af.aof;
+            merlin server start;
+            merlin server stop;""",
+            [
+                FileHasRegex("merlin_server/redis.conf", "port 8888"),
+                FileHasRegex("merlin_server/redis.conf", "requirepass new_password"),
+                FileHasRegex("merlin_server/redis.conf", "dir ./config_dir"),
+                FileHasRegex("merlin_server/redis.conf", "save 80 8"),
+                FileHasRegex("merlin_server/redis.conf", "dbfilename new_sf"),
+                FileHasRegex("merlin_server/redis.conf", "appendfsync always"),
+                FileHasRegex("merlin_server/redis.conf", 'appendfilename "new_af.aof"'),
+                PathExists("./config_dir/new_sf"),
+                PathExists("./config_dir/appendonlydir"),
+                HasRegex("Server started with PID [0-9]*"),
+                HasRegex("Merlin server terminated"),
+            ],
+            "local",
+            "rm -rf appendonly.aof dump.rdb merlin_server/ config_dir/",
+        ),
+        "merlin server config add/remove user": (
+            """merlin server init;
+            merlin server start;
+            merlin server config --add-user new_user new_password;
+            merlin server stop;
+            scp ./merlin_server/redis.users ./merlin_server/redis.users_new
+            merlin server start;
+            merlin server config --remove-user new_user;
+            merlin server stop;
+            """,
+            [
+                FileHasRegex("./merlin_server/redis.users_new", "new_user"),
+                FileHasNoRegex("./merlin_server/redis.users", "new_user"),
+            ],
+            "local",
+            CLEAN_MERLIN_SERVER,
         ),
-        "clean merlin server": ("rm -rf appendonly.aof dump.rdb merlin_server/"),
     }
     examples_check = {
         "example list": (
@@ -384,7 +456,8 @@ def define_tests():
     all_tests = {}
     for test_dict in [
         basic_checks,
-        server_tests,
+        server_basic_tests,
+        server_config_tests,
         examples_check,
         run_workers_echo_tests,
         wf_format_tests,