Make CLI arguments consistent (#372)

* Use argparse action='append' consistently, not nargs='*' except when enumerating tests on the command-line
* Create cli.util to factor out ways to create TestPlan components from command-line options
* Make arguments to the various cli command consistent, and use the same code to parse them where applicable

---------

Co-authored-by: Johannes Ernst <git@j12t.org>

src/feditest/cli/commands/create_constellation.py

@@ -2,8 +2,9 @@
 Combine node definitions into a constellation.
 """
 
-from argparse import ArgumentError, ArgumentParser, Namespace, _SubParsersAction
+from argparse import ArgumentParser, Namespace, _SubParsersAction
 
+from feditest.cli.util import create_constellation_from_nodes
 from feditest.testplan import TestPlanConstellation, TestPlanConstellationNode
 
 
@@ -15,22 +16,12 @@ def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
     roles : dict[str,TestPlanConstellationNode | None] = {}
 
     if remaining:
-        parser.print_help() # Would be nice to print help for this sub-command but I can't figure out how to do it
+        parser.print_help()
         return 0
 
-    for nodepair in args.node:
-        rolename, nodefile = nodepair.split('=', 1)
-        if not rolename:
-            raise ArgumentError(None, f'Rolename component of --node must not be empty: "{ nodepair }".')
-        if rolename in roles:
-            raise ArgumentError(None, f'Role is already taken: "{ rolename }".')
-        if not nodefile:
-            raise ArgumentError(None, f'Filename component must not be empty: "{ nodepair }".')
-        node = TestPlanConstellationNode.load(nodefile)
-        roles[rolename] = node
-
     constellation = TestPlanConstellation(roles)
 
+    constellation = create_constellation_from_nodes(args)
     if args.name:
         constellation.name = args.name
 

src/feditest/cli/commands/create_session_template.py

@@ -4,11 +4,9 @@
 """
 
 from argparse import ArgumentParser, Namespace, _SubParsersAction
-import re
-from typing import Any
 
 import feditest
-from feditest.testplan import TestPlanTestSpec, TestPlanSession, TestPlanConstellation, TestPlanConstellationNode
+from feditest.cli.util import create_session_template_from_tests
 
 
 def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
@@ -20,39 +18,15 @@ def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
         parser.print_help()
         return 0
 
-    pattern = re.compile(args.filter_regex) if args.filter_regex else None
-
     feditest.load_default_tests()
     feditest.load_tests_from(args.testsdir)
 
-    test_plan_specs : list[TestPlanTestSpec]= []
-    constellation_role_names : dict[str,Any] = {}
-    for name in sorted(feditest.all_tests.keys()):
-        if pattern is None or pattern.match(name):
-            test = feditest.all_tests.get(name)
-            if test is None: # make linter happy
-                continue
-            if test.builtin:
-                continue
-
-            test_plan_spec = TestPlanTestSpec(name)
-            test_plan_specs.append(test_plan_spec)
-
-            for role_name in test.needed_local_role_names():
-                constellation_role_names[role_name] = 1
-                if not test_plan_spec.rolemapping:
-                    test_plan_spec.rolemapping = {}
-                test_plan_spec.rolemapping[role_name] = role_name
+    session_template = create_session_template_from_tests(args)
 
-    constellation_roles: dict[str,TestPlanConstellationNode | None] = {}
-    for constellation_role_name in constellation_role_names:
-        constellation_roles[constellation_role_name] = None
-
-    session = TestPlanSession(TestPlanConstellation(constellation_roles), test_plan_specs, args.name)
     if args.out:
-        session.save(args.out)
+        session_template.save(args.out)
     else:
-        session.print()
+        session_template.print()
 
     return 0
 
@@ -63,10 +37,16 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
     parent_parser: the parent argparse parser
     cmd_name: name of this command
     """
+    # general flags and options
     parser = parent_parser.add_parser(cmd_name, help='Create a template for a test session')
+    parser.add_argument('--testsdir', action='append', default=['tests'], help='Directory or directories where to find tests')
+
+    # session template options
     parser.add_argument('--name', default=None, required=False, help='Name of the created test session template')
     parser.add_argument('--filter-regex', default=None, help='Only include tests whose name matches this regular expression')
+    parser.add_argument('--test', nargs='+', help='Include this/these named tests(s)')
+
+    # output options
     parser.add_argument('--out', '-o', default=None, required=False, help='Name of the file for the created test session template')
-    parser.add_argument('--testsdir', nargs='*', default=['tests'], help='Directory or directories where to find tests')
 
     return parser

src/feditest/cli/commands/create_testplan.py

@@ -4,8 +4,8 @@
 
 from argparse import ArgumentParser, Namespace, _SubParsersAction
 import feditest
-from feditest.testplan import TestPlan, TestPlanConstellation, TestPlanSession
-
+from feditest.cli.util import create_plan_from_session_templates_and_constellations
+from feditest.reporting import fatal
 
 def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
     """
@@ -18,27 +18,14 @@ def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
     feditest.load_default_tests()
     feditest.load_tests_from(args.testsdir)
 
-    constellations = []
-    for constellation_file in args.constellation:
-        constellations.append(TestPlanConstellation.load(constellation_file))
-
-    session_templates = []
-    for session_file in args.session:
-        session_templates.append(TestPlanSession.load(session_file))
-
-    sessions = []
-    for session_template in session_templates:
-        for constellation in constellations:
-            session = session_template.instantiate_with_constellation(constellation, constellation.name)
-            sessions.append(session)
-
-    test_plan = TestPlan(sessions, args.name)
-    test_plan.simplify()
-
-    if args.out:
-        test_plan.save(args.out)
+    test_plan = create_plan_from_session_templates_and_constellations(args)
+    if test_plan:
+        if args.out:
+            test_plan.save(args.out)
+        else:
+            test_plan.print()
     else:
-        test_plan.print()
+        fatal('Failed to create test plan from the provided arguments')
 
     return 0
 
@@ -49,11 +36,20 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
     parent_parser: the parent argparse parser
     cmd_name: name of this command
     """
+    # general flags and options
     parser = parent_parser.add_parser(cmd_name, help='Create a test plan by running all provided test sessions in all provided constellations')
+    parser.add_argument('--testsdir', action='append', default=['tests'], help='Directory or directories where to find tests')
+
+    # test plan options
     parser.add_argument('--name', default=None, required=False, help='Name of the generated test plan')
-    parser.add_argument('--constellation', required=True, nargs='+', help='File(s) each containing a JSON fragment defining a constellation')
-    parser.add_argument('--session', '--session-template', required=True, nargs='+', help='File(s) each containing a JSON fragment defining a test session')
+    parser.add_argument('--constellation', action='append', help='File(s) each containing a JSON fragment defining a constellation')
+    parser.add_argument('--session', '--session-template', action='append', help='File(s) each containing a JSON fragment defining a test session')
+    parser.add_argument('--node', action='append',
+                        help="Use role=file to specify that the node definition in 'file' is supposed to be used for constellation role 'role'")
+    parser.add_argument('--filter-regex', default=None, help='Only include tests whose name matches this regular expression')
+    parser.add_argument('--test', nargs='+', help='Run this/these named tests(s)')
+
+    # output options
     parser.add_argument('--out', '-o', default=None, required=False, help='Name of the file for the generated test plan')
-    parser.add_argument('--testsdir', nargs='*', default=['tests'], help='Directory or directories where to find tests')
 
     return parser

src/feditest/cli/commands/info.py

@@ -99,7 +99,7 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
     cmd_name: name of this command
     """
     parser = parent_parser.add_parser( cmd_name, help='Provide information on a variety of objects')
-    parser.add_argument('--testsdir', nargs='*', default=['tests'], help='Directory or directories where to find tests')
+    parser.add_argument('--testsdir', action='append', default=['tests'], help='Directory or directories where to find tests')
     parser.add_argument('--nodedriversdir', action='append', help='Directory or directories where to find extra drivers for nodes that can be tested')
     type_group = parser.add_mutually_exclusive_group(required=True)
     type_group.add_argument('--test',  help='Provide information about a test.')

src/feditest/cli/commands/list_tests.py

@@ -34,6 +34,6 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
     """
     parser = parent_parser.add_parser(cmd_name, help='List the available tests')
     parser.add_argument('--filter-regex', default=None, help='Only list tests whose name matches this regular expression')
-    parser.add_argument('--testsdir', nargs='*', default=['tests'], help='Directory or directories where to find tests')
+    parser.add_argument('--testsdir', action='append', default=['tests'], help='Directory or directories where to find tests')
 
     return parser

src/feditest/cli/commands/run.py

@@ -2,17 +2,16 @@
 Run one or more tests
 """
 
-from argparse import ArgumentError, ArgumentParser, Namespace, _SubParsersAction
-import re
-from typing import Any
-
-from msgspec import ValidationError
+from argparse import ArgumentParser, Namespace, _SubParsersAction
 
 import feditest
+from feditest.cli.util import (
+    create_plan_from_session_templates_and_constellations,
+    create_plan_from_testplan
+)
 from feditest.registry import Registry, set_registry_singleton
 from feditest.reporting import warning
-from feditest.tests import Test
-from feditest.testplan import TestPlan, TestPlanConstellation, TestPlanConstellationNode, TestPlanSession, TestPlanTestSpec
+from feditest.testplan import TestPlan
 from feditest.testrun import TestRun
 from feditest.testruncontroller import AutomaticTestRunController, InteractiveTestRunController, TestRunController
 from feditest.testruntranscript import (
@@ -46,22 +45,15 @@ def run(parser: ArgumentParser, args: Namespace, remaining: list[str]) -> int:
         set_registry_singleton(Registry.create(args.domain)) # overwrite
 
     # Determine testplan. While we are at it, check consistency of arguments.
+    plan : TestPlan | None = None
     if args.testplan:
-        plan = _create_plan_from_testplan(args)
+        plan = create_plan_from_testplan(args)
     else:
-        session_templates = _create_session_templates(args)
-        constellations = _create_constellations(args)
-
-        sessions = []
-        for session_template in session_templates:
-            for constellation in constellations:
-                session = session_template.instantiate_with_constellation(constellation, constellation.name)
-                sessions.append(session)
-        if sessions:
-            plan = TestPlan(sessions, None)
-            plan.simplify()
-        else: # neither sessions nor testplan specified
-            plan = TestPlan.load("feditest-default.json")
+        plan = create_plan_from_session_templates_and_constellations(args)
+
+    if not plan:
+        # neither sessions nor testplan specified
+        plan = TestPlan.load("feditest-default.json")
 
     if not plan.is_compatible_type():
         warning(f'Test plan has unexpected type { plan.type }: incompatibilities may occur.')
@@ -110,18 +102,17 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
     """
     # general flags and options
     parser = parent_parser.add_parser(cmd_name, help='Run one or more tests' )
-    parser.add_argument('--testsdir', nargs='*', default=['tests'], help='Directory or directories where to find tests')
+    parser.add_argument('--testsdir', action='append', default=['tests'], help='Directory or directories where to find tests')
     parser.add_argument('--nodedriversdir', action='append', help='Directory or directories where to find extra drivers for nodes that can be tested')
     parser.add_argument('--domain', type=hostname_validate, help='Local-only DNS domain for the DNS hostnames that are auto-generated for nodes')
-    parser.add_argument('--interactive', action="store_true",
-                        help="Run the tests interactively")
-    parser.add_argument('--who', action='store_true',
-                        help="Record who ran the test plan on what host.")
+    parser.add_argument('--interactive', action="store_true", help="Run the tests interactively")
+    parser.add_argument('--who', action='store_true', help="Record who ran the test plan on what host.")
 
     # test plan options. We do not use argparse groups, as the situation is more complicated than argparse seems to support
+    parser.add_argument('--name', default=None, required=False, help='Name of the generated test plan')
     parser.add_argument('--testplan', help='Name of the file that contains the test plan to run')
-    parser.add_argument('--constellation', nargs='+', help='File(s) each containing a JSON fragment defining a constellation')
-    parser.add_argument('--session', '--session-template', nargs='+', help='File(s) each containing a JSON fragment defining a test session')
+    parser.add_argument('--constellation', action='append', help='File(s) each containing a JSON fragment defining a constellation')
+    parser.add_argument('--session', '--session-template', action='append', help='File(s) each containing a JSON fragment defining a test session')
     parser.add_argument('--node', action='append',
                         help="Use role=file to specify that the node definition in 'file' is supposed to be used for constellation role 'role'")
     parser.add_argument('--filter-regex', default=None, help='Only include tests whose name matches this regular expression')
@@ -141,108 +132,3 @@ def add_sub_parser(parent_parser: _SubParsersAction, cmd_name: str) -> None:
                         help="Write summary to stdout, or to the provided file (if given). This is the default if no other output option is given")
 
     return parser
-
-
-def _create_plan_from_testplan(args: Namespace) -> TestPlan:
-    if args.constellation:
-        raise ArgumentError(None, '--testplan already defines --constellation. Do not provide both.')
-    if args.session:
-        raise ArgumentError(None, '--testplan already defines --session-template. Do not provide both.')
-    if args.node:
-        raise ArgumentError(None, '--testplan already defines --node via the contained constellation. Do not provide both.')
-    if args.test:
-        raise ArgumentError(None, '--testplan already defines --test via the contained session. Do not provide both.')
-    plan = TestPlan.load(args.testplan)
-    return plan
-
-
-def _create_session_templates(args: Namespace) -> list[TestPlanSession]:
-    if args.session:
-        if args.filter_regex:
-            raise ArgumentError(None, '--session already defines the tests, do not provide --filter-regex')
-        if args.test:
-            raise ArgumentError(None, '--session already defines --test. Do not provide both.')
-        session_templates = []
-        for session_file in args.session:
-            session_templates.append(TestPlanSession.load(session_file))
-        return session_templates
-
-    test_plan_specs : list[TestPlanTestSpec]= []
-    constellation_role_names : dict[str,Any] = {}
-    constellation_roles: dict[str,TestPlanConstellationNode | None] = {}
-    tests : list[Test]= []
-
-    if args.test:
-        if args.filter_regex:
-            raise ArgumentError(None, '--filter-regex already defines --test. Do not provide both.')
-        for name in args.test:
-            test = feditest.all_tests.get(name)
-            if test is None:
-                raise ArgumentError(None, f'Cannot find test: "{ name }".')
-            tests.append(test)
-
-    elif args.filter_regex:
-        pattern = re.compile(args.filter_regex)
-        for name in sorted(feditest.all_tests.keys()):
-            if pattern.match(name):
-                test = feditest.all_tests.get(name)
-                if test is None: # make linter happy
-                    continue
-                if test.builtin:
-                    continue
-                tests.append(test)
-
-    else:
-        for name in sorted(feditest.all_tests.keys()):
-            test = feditest.all_tests.get(name)
-            if test is None: # make linter happy
-                continue
-            if test.builtin:
-                continue
-            tests.append(test)
-
-    for test in tests:
-        test_plan_spec = TestPlanTestSpec(name)
-        test_plan_specs.append(test_plan_spec)
-
-        for role_name in test.needed_local_role_names():
-            constellation_role_names[role_name] = 1
-            if not test_plan_spec.rolemapping:
-                test_plan_spec.rolemapping = {}
-            test_plan_spec.rolemapping[role_name] = role_name
-
-    for constellation_role_name in constellation_role_names:
-        constellation_roles[constellation_role_name] = None
-
-    session = TestPlanSession(TestPlanConstellation(constellation_roles), test_plan_specs)
-    return [ session ]
-
-
-def _create_constellations(args: Namespace) -> list[TestPlanConstellation]:
-    if args.constellation:
-        if args.node:
-            raise ArgumentError(None, '--constellation already defines --node. Do not provide both.')
-
-        constellations = []
-        for constellation_file in args.constellation:
-            try:
-                constellations.append(TestPlanConstellation.load(constellation_file))
-            except ValidationError as e:
-                raise ArgumentError(None, f'Constellation file { constellation_file }: { e }')
-        return constellations
-
-    # Don't check for empty nodes: we need that for testing feditest
-    roles : dict[str, TestPlanConstellationNode | None] = {}
-    for nodepair in args.node:
-        rolename, nodefile = nodepair.split('=', 1)
-        if not rolename:
-            raise ArgumentError(None, f'Rolename component of --node must not be empty: "{ nodepair }".')
-        if rolename in roles:
-            raise ArgumentError(None, f'Role is already taken: "{ rolename }".')
-        if not nodefile:
-            raise ArgumentError(None, f'Filename component must not be empty: "{ nodepair }".')
-        node = TestPlanConstellationNode.load(nodefile)
-        roles[rolename] = node
-
-    constellation = TestPlanConstellation(roles)
-    return [ constellation ]