From e6ffd9c5d063055cdd25813f3f1d2f17fb1c9f3c Mon Sep 17 00:00:00 2001 From: Shahzeb Siddiqui Date: Mon, 15 May 2023 12:55:31 -0500 Subject: [PATCH 1/4] change behavior of 'buildtest build --filter' where filter fields are separated by semicolon and multiple values can be specified as comma separated list. refactor the codebase for filtering by maintainers, type and tags to support multiple items in list --- buildtest/buildsystem/builders.py | 55 +++++++++++++++++++------------ buildtest/cli/__init__.py | 41 +++++++++++++++++++++-- buildtest/cli/build.py | 5 ++- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/buildtest/buildsystem/builders.py b/buildtest/buildsystem/builders.py index 875e15cb3..92c9b4452 100644 --- a/buildtest/buildsystem/builders.py +++ b/buildtest/buildsystem/builders.py @@ -87,7 +87,12 @@ def __init__( ) return - if self.filters["maintainers"] not in self.bp.recipe.get("maintainers"): + maintainer_match = bool( + set(self.filters["maintainers"]) + & set(self.bp.recipe.get("maintainers")) + ) + + if not maintainer_match: console.print( f"{self.bp.buildspec}: unable to find maintainer: {self.filters['maintainers']} in buildspec which contains the following maintainers: {self.bp.recipe.get('maintainers')} therefore we skip this test" ) @@ -462,22 +467,29 @@ def _skip_tests_by_tags(self, recipe, name): bool: False if ``buildtest build --filter tags`` is not specified. If specified we return ``True`` if ``tags`` field is not in test recipe or there is a matching tag. """ + # if no filter tags specified lets return immediately and test is not skipped + if not self.filters.get("tags"): + return False + + # if tags field in buildspec is empty, then we skip test only if user filters by tags + if not recipe.get("tags"): + return True - if self.filters.get("tags"): - # if tags field in buildspec is empty, then we skip test only if user filters by tags - if not recipe.get("tags"): - return True + found = False + # the input tag names from test can be list or string + tests_in_tags = recipe["tags"] + # if input is string, convert to list otherwise we assume its a list + tests_in_tags = ( + [tests_in_tags] if isinstance(tests_in_tags, str) else tests_in_tags + ) - found = False - # for tagname in self.filters: - if self.filters["tags"] in recipe.get("tags"): - found = True + found = bool(set(self.filters["tags"]) & set(tests_in_tags)) - if not found: - msg = f"[{name}][{self.bp.buildspec}]: test is skipped because it is not in tag filter list: {self.filters}" - self.logger.info(msg) - print(msg) - return True + if not found: + msg = f"[{name}][{self.bp.buildspec}]: test is skipped because it is not in tag filter list: {self.filters}" + self.logger.info(msg) + print(msg) + return True return False @@ -493,15 +505,16 @@ def _skip_tests_by_type(self, recipe, name): bool: False if ``buildtest build --filter type`` is not specified. If there is a match with input filter and ``type`` field in test we return ``True`` """ + if not self.filters.get("type"): + return False - if self.filters.get("type"): - found = self.filters["type"] == recipe["type"] + found = recipe["type"] in self.filters["type"] - if not found: - msg = f"[{name}][{self.bp.buildspec}]: test is skipped because it is not in type filter list: {self.filters['type']}" - self.logger.info(msg) - print(msg) - return True + if not found: + msg = f"[{name}][{self.bp.buildspec}]: test is skipped because it is not in type filter list: {self.filters['type']}" + self.logger.info(msg) + print(msg) + return True return False diff --git a/buildtest/cli/__init__.py b/buildtest/cli/__init__.py index 39f3e1972..10c39e58f 100644 --- a/buildtest/cli/__init__.py +++ b/buildtest/cli/__init__.py @@ -12,6 +12,43 @@ from rich.color import Color, ColorParseError +def build_filters_format(val): + """This method is used as validate argument type for ``buildtest build --filter``. + This method returns a dict of key, value pairs where input is in the format + **key1=val1,val2;key2=val3**. The semicolon is used to separate the keys and multiple values + can be specified via comma + + Args: + val (str): Input string in ``key1=value1,val2;key2=value3`` format that is processed into a dictionary type + + Returns: + dict: A dict mapping of key=value pairs + """ + + kv_dict = {} + + if ";" in val: + entries = val.split(";") + for entry in entries: + if "=" not in entry: + raise argparse.ArgumentTypeError("Must specify k=v") + + key, values = entry.split("=")[0], entry.split("=")[1] + value_list = values.split(",") + kv_dict[key] = value_list + + return kv_dict + + if "=" not in val: + raise argparse.ArgumentTypeError("Must specify in key=value format") + + key, values = val.split("=")[0], val.split("=")[1] + value_list = values.split(",") + kv_dict[key] = value_list + + return kv_dict + + def handle_kv_string(val): """This method is used as type field in --filter argument in ``buildtest buildspec find``. This method returns a dict of key,value pair where input is in format @@ -572,8 +609,8 @@ def build_menu(subparsers): filter_group.add_argument( "-f", "--filter", - type=handle_kv_string, - help="Filter buildspec based on tags, type, or maintainers. Usage: --filter key1=val1,key2=val2", + type=build_filters_format, + help="Filter buildspec based on tags, type, or maintainers. Usage: --filter key1=val1,val2;key2=val3;key3=val4,val5", ) filter_group.add_argument( "--helpfilter", diff --git a/buildtest/cli/build.py b/buildtest/cli/build.py index cd902aee6..3e0addbde 100644 --- a/buildtest/cli/build.py +++ b/buildtest/cli/build.py @@ -795,7 +795,10 @@ def _validate_filters(self): ) if key == "type": - if self.filter_buildspecs[key] not in schema_table["types"]: + if any( + type_value not in schema_table["types"] + for type_value in self.filter_buildspecs[key] + ): raise BuildTestError( f"Invalid value for filter 'type': '{self.filter_buildspecs[key]}', valid schema types are : {schema_table['types']}" ) From 82027fdb98f3a9408c1f7ccad556e565c205bd6d Mon Sep 17 00:00:00 2001 From: Shahzeb Siddiqui Date: Mon, 15 May 2023 13:02:59 -0500 Subject: [PATCH 2/4] make minor changes to documentation on filtering tests --- docs/gettingstarted/buildingtest.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/gettingstarted/buildingtest.rst b/docs/gettingstarted/buildingtest.rst index 8ef2910fc..9ca5675a7 100644 --- a/docs/gettingstarted/buildingtest.rst +++ b/docs/gettingstarted/buildingtest.rst @@ -225,8 +225,8 @@ Filtering Buildspecs --------------------- buildtest has support for filtering buildspecs based on certain attributes defined in buildspec file. Upon :ref:`discover_buildspecs`, buildtest -will filter out tests or entire buildspec files. The ``buildtest build --filter`` option can be used to filter buildspecs which expects a **single** -key=value pair. Currently, buildtest can filter tests based on ``tags``, ``type`` and ``maintainers``. +will filter out tests or entire buildspec files. The ``buildtest build --filter`` option can be used to filter tests where the format is ``key1=val1;key2=val2,val3``. +The semicolon is used to specify multiple filter fields and the comma is used to specify multiple values for a given field. To see all available filter fields you can run ``buildtest build --helpfilter`` and buildtest will report the fields followed by description. From 9f33879034491710482365876f89bb76586458c2 Mon Sep 17 00:00:00 2001 From: Shahzeb Siddiqui Date: Mon, 15 May 2023 15:29:19 -0500 Subject: [PATCH 3/4] increase test coverage for method valid_time, supported_color, build_filters_format and handle_kv_string method. Fix issue with regression test pertaining to filters by tags, maintainers, and type --- tests/cli/test_argparse.py | 62 +++++++++++++++++++++++++++++++++++++- tests/cli/test_build.py | 8 ++--- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/tests/cli/test_argparse.py b/tests/cli/test_argparse.py index cbfb9787a..c5998822e 100644 --- a/tests/cli/test_argparse.py +++ b/tests/cli/test_argparse.py @@ -1,7 +1,14 @@ import argparse import pytest -from buildtest.cli import get_parser, handle_kv_string, positive_number +from buildtest.cli import ( + build_filters_format, + get_parser, + handle_kv_string, + positive_number, + supported_color, + valid_time, +) def test_positive_number(): @@ -10,6 +17,13 @@ def test_positive_number(): positive_number(0) print(err) + # input argument must be integer or string + with pytest.raises(argparse.ArgumentTypeError): + positive_number([1, 2, 3]) + + with pytest.raises(ValueError): + positive_number("hello") + def test_handle_kv_string(): assert {"tags": "fail"} == handle_kv_string("tags=fail") @@ -18,10 +32,56 @@ def test_handle_kv_string(): "tags=fail,type=script" ) + # missing equal sign with multiple keys with pytest.raises(argparse.ArgumentTypeError) as err: handle_kv_string("tags,type,script") print(err) + # missing equal sign with single key + with pytest.raises(argparse.ArgumentTypeError): + handle_kv_string("tags") + + +def test_buildtest_build_filters(): + assert { + "tags": ["fail", "pass"], + "type": ["script"], + "maintainers": ["shahzebsiddiqui"], + } == build_filters_format("tags=fail,pass;type=script;maintainers=shahzebsiddiqui") + build_filters_format("tags=fail") + + # missing equal sign with multiple keys + with pytest.raises(argparse.ArgumentTypeError) as err: + build_filters_format("tags=fail,pass;type;maintainers=shahzebsiddiqui") + print(err) + + # missing equal sign with single key + with pytest.raises(argparse.ArgumentTypeError) as err: + build_filters_format("tags") + print(err) + + +def test_supported_colors(): + rich_color = supported_color("red") + assert rich_color.name == "red" + + # input must be a string + with pytest.raises(argparse.ArgumentTypeError): + supported_color(["red"]) + + assert supported_color("xyz") is None + + +def test_valid_time(): + valid_time("2022-01-01") + # input must be a string not a list + with pytest.raises(argparse.ArgumentTypeError): + valid_time(["2022-01-01"]) + + # raises exception when its unable to convert time + with pytest.raises(ValueError): + valid_time("2022-01-01 abcdef") + def test_arg_parse(): parser = get_parser() diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py index 25ec22b67..31189dbdb 100644 --- a/tests/cli/test_build.py +++ b/tests/cli/test_build.py @@ -114,7 +114,7 @@ def test_build_filter_check(): cmd = BuildTest( configuration=configuration, tags=["pass"], - filter_buildspecs={"tags": "pass"}, + filter_buildspecs={"tags": ["pass"]}, buildtest_system=system, ) cmd.build() @@ -123,7 +123,7 @@ def test_build_filter_check(): cmd = BuildTest( configuration=configuration, buildspecs=[os.path.join(BUILDTEST_ROOT, "tutorials")], - filter_buildspecs={"maintainers": "@shahzebsiddiqui"}, + filter_buildspecs={"maintainers": ["@shahzebsiddiqui"]}, buildtest_system=system, ) cmd.build() @@ -132,7 +132,7 @@ def test_build_filter_check(): cmd = BuildTest( configuration=configuration, buildspecs=[os.path.join(BUILDTEST_ROOT, "tutorials", "shell_examples.yml")], - filter_buildspecs={"type": "script"}, + filter_buildspecs={"type": ["script"]}, buildtest_system=system, ) cmd.build() @@ -144,7 +144,7 @@ def test_build_filter_check(): buildspecs=[ os.path.join(BUILDTEST_ROOT, "tutorials", "shell_examples.yml") ], - filter_buildspecs={"type": "spack"}, + filter_buildspecs={"type": ["spack"]}, buildtest_system=system, ) cmd.build() From 50c7493fd53d30614204dd1615804e080e8cf3d4 Mon Sep 17 00:00:00 2001 From: Shahzeb Siddiqui Date: Mon, 15 May 2023 15:45:01 -0500 Subject: [PATCH 4/4] add regression test coverage for excluding tags with 'buildtest build --exclude-tags' --- tests/cli/test_build.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/cli/test_build.py b/tests/cli/test_build.py index 31189dbdb..b680bbfc0 100644 --- a/tests/cli/test_build.py +++ b/tests/cli/test_build.py @@ -250,6 +250,31 @@ def test_buildspec_tag_executor(): cmd.build() +# pytest.mark.cli +def test_exclude_tags(): + system = BuildTestSystem() + + # testing buildtest build --tags fail --exclude-tags fail + cmd = BuildTest( + configuration=configuration, + tags=["fail"], + exclude_tags=["fail"], + buildtest_system=system, + ) + cmd.build() + + # testing buildtest build --buildspec $BUILDTEST_ROOT/tutorials/python-hello.yml --exclude-tags python + cmd = BuildTest( + configuration=configuration, + buildspecs=[os.path.join(BUILDTEST_ROOT, "tutorials", "python-hello.yml")], + exclude_tags=["python"], + buildtest_system=system, + ) + # no test will be run + with pytest.raises(SystemExit): + cmd.build() + + @pytest.mark.cli def test_build_csh_executor(): if not shutil.which("csh"):