From d303489ee7b7702895b2a821d2f97a2f16957bbf Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 26 Jan 2024 09:23:30 +0100 Subject: [PATCH 01/15] bumped pydantic v2 dep --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index c52f0b3..1dd68f3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,7 +20,7 @@ license.text = "Apache-2.0" keywords = ["juju", "relation interfaces"] dependencies = [ - "pydantic>= 1.10.7, <2", + "pydantic>2", "typer==0.7.0", "ops-scenario>=5.2", "pytest" From e7edf37444b52974b6e84affdacdf909be13bf47 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 12:41:52 +0100 Subject: [PATCH 02/15] fixed endpoint name determination from input_state --- interface_tester/collector.py | 4 +-- interface_tester/interface_test.py | 58 +++++++++++++++++++----------- tests/unit/test_collect_schemas.py | 8 ++--- 3 files changed, 43 insertions(+), 27 deletions(-) diff --git a/interface_tester/collector.py b/interface_tester/collector.py index ba8f893..52657b3 100644 --- a/interface_tester/collector.py +++ b/interface_tester/collector.py @@ -107,8 +107,8 @@ def load_schema_module(schema_path: Path) -> types.ModuleType: # Otherwise we'll get an error when we re-run @validator # fixme: is there a better way to do this? - logger.debug("Clearing pydantic.class_validators._FUNCS") - pydantic.class_validators._FUNCS.clear() # noqa + # logger.debug("Clearing pydantic.class_validators._FUNCS") + # pydantic.class_validators._FUNCS.clear() # noqa try: module = importlib.import_module(module_name) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index 27c0545..d23c2f5 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -290,7 +290,7 @@ def assert_schema_valid(self, schema: Optional["DataBagSchema"] = None): } ) except ValidationError as e: - errors.append(e.args[0]) + errors.append(str(e)) if errors: raise SchemaValidationError(errors) @@ -426,6 +426,20 @@ def _coerce_event(self, raw_event: Union[str, Event], relation: Relation) -> Eve f"Invalid test case: {self} cannot cast {raw_event} to Event." ) + @staticmethod + def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): + endpoints_for_interface = supported_endpoints[role] + + if len(endpoints_for_interface) < 1: + raise ValueError(f"no endpoint found for {role}/{interface_name}.") + elif len(endpoints_for_interface) > 1: + raise ValueError( + f"Multiple endpoints found for {role}/{interface_name}: " + f"{endpoints_for_interface}: cannot guess which one it is " + f"we're supposed to be testing" + ) + return endpoints_for_interface[0] + def _generate_relations_state( self, state_template: State, input_state: State, supported_endpoints, role: Role ) -> List[Relation]: @@ -437,6 +451,9 @@ def _generate_relations_state( """ interface_name = self.ctx.interface_name + # determine what charm endpoint we're testing. + endpoint = self._get_endpoint(supported_endpoints, role, interface_name=interface_name) + for rel in state_template.relations: if rel.interface == interface_name: logger.warning( @@ -448,18 +465,29 @@ def _generate_relations_state( def filter_relations(rels: List[Relation], op: Callable): return [r for r in rels if op(r.interface, interface_name)] - # the baseline is: all relations whose interface IS NOT the interface we're testing. + # the baseline is: all relations provided by the charm in the state_template, + # whose interface IS NOT the interface we're testing. We assume the test (input_state) is + # the ultimate owner of the state when it comes to the interface we're testing. + # We don't allow the charm to mess with it. relations = filter_relations(state_template.relations, op=operator.ne) if input_state: - # if the charm we're testing specified some relations in its input state, we add those - # whose interface IS the same as the one we're testing. If other relation interfaces - # were specified, they will be ignored. - relations.extend(filter_relations(input_state.relations, op=operator.eq)) + # if the interface test we're running specified some relations in its input_state, + # we add those whose interface IS the same as the one we're testing. + # If other relation interfaces were specified (for whatever reason?), + # they will be ignored. + relations_from_input_state = filter_relations(input_state.relations, op=operator.eq) + + # relations that come from the state_template presumably have the right endpoint, + # but those that we get from interface tests cannot. + relations_with_endpoint = [r.replace(endpoint=endpoint) for r in relations_from_input_state] + + relations.extend(relations_with_endpoint) - if ignored := filter_relations(input_state.relations, op=operator.eq): + if ignored := filter_relations(input_state.relations, op=operator.ne): + # this is a sign of a bad test. logger.warning( - "irrelevant relations specified in input state for %s/%s." + "irrelevant relations specified in input_state for %s/%s." "These will be ignored. details: %s" % (interface_name, role, ignored) ) @@ -468,19 +496,6 @@ def filter_relations(rels: List[Relation], op: Callable): if not filter_relations(relations, op=operator.eq): # if neither the charm nor the interface specified any custom relation spec for # the interface we're testing, we will provide one. - endpoints_for_interface = supported_endpoints[role] - - if len(endpoints_for_interface) < 1: - raise ValueError(f"no endpoint found for {role}/{interface_name}.") - elif len(endpoints_for_interface) > 1: - raise ValueError( - f"Multiple endpoints found for {role}/{interface_name}: " - f"{endpoints_for_interface}: cannot guess which one it is " - f"we're supposed to be testing" - ) - else: - endpoint = endpoints_for_interface[0] - relations.append( Relation( interface=interface_name, @@ -491,4 +506,5 @@ def filter_relations(rels: List[Relation], op: Callable): "%s: merged %s and %s --> relations=%s" % (self, input_state, state_template, relations) ) + return relations diff --git a/tests/unit/test_collect_schemas.py b/tests/unit/test_collect_schemas.py index 5190da4..8fd7f69 100644 --- a/tests/unit/test_collect_schemas.py +++ b/tests/unit/test_collect_schemas.py @@ -35,7 +35,7 @@ def test_collect_schemas(tmp_path): """from interface_tester.schema_base import DataBagSchema class RequirerSchema(DataBagSchema): - foo = 1""" + foo: int = 1""" ) ) @@ -54,7 +54,7 @@ def test_collect_schemas_multiple(tmp_path): """from interface_tester.schema_base import DataBagSchema class RequirerSchema(DataBagSchema): - foo = 1""" + foo:int = 1""" ) ) @@ -65,7 +65,7 @@ class RequirerSchema(DataBagSchema): """from interface_tester.schema_base import DataBagSchema class RequirerSchema(DataBagSchema): - foo = 2""" + foo:int = 2""" ) ) @@ -84,7 +84,7 @@ def test_collect_invalid_schemas(tmp_path): dedent( """from interface_tester.schema_base import DataBagSchema class ProviderSchema(DataBagSchema): - foo = 2""" + foo:int = 2""" ) ) From 9f5e810a3a655a433643734fd5900276247e3283 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 12:43:49 +0100 Subject: [PATCH 03/15] tests --- interface_tester/interface_test.py | 4 +++- tests/unit/test_e2e.py | 22 +++++++++++----------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index d23c2f5..efb5930 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -480,7 +480,9 @@ def filter_relations(rels: List[Relation], op: Callable): # relations that come from the state_template presumably have the right endpoint, # but those that we get from interface tests cannot. - relations_with_endpoint = [r.replace(endpoint=endpoint) for r in relations_from_input_state] + relations_with_endpoint = [ + r.replace(endpoint=endpoint) for r in relations_from_input_state + ] relations.extend(relations_with_endpoint) diff --git a/tests/unit/test_e2e.py b/tests/unit/test_e2e.py index 5a021ed..b5664ba 100644 --- a/tests/unit/test_e2e.py +++ b/tests/unit/test_e2e.py @@ -107,7 +107,7 @@ def test_error_if_skip_schema_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -133,7 +133,7 @@ def test_error_if_assert_relation_data_empty_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -160,7 +160,7 @@ def test_error_if_assert_schema_valid_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -186,7 +186,7 @@ def test_error_if_assert_schema_without_schema(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -213,7 +213,7 @@ def test_error_if_return_before_schema_call(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -239,7 +239,7 @@ def test_error_if_return_without_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -285,7 +285,7 @@ def test_valid_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={} @@ -312,7 +312,7 @@ def test_valid_run_default_schema(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={"foo":"1"}, @@ -355,7 +355,7 @@ def test_default_schema_validation_failure(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={"foo":"abc"}, @@ -408,7 +408,7 @@ class FooBarSchema(DataBagSchema): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={"foo":"1"}, @@ -445,7 +445,7 @@ class FooBarSchema(DataBagSchema): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='tracing', + endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' interface='tracing', remote_app_name='remote', local_app_data={"foo":"abc"}, From b1e76e13e0731fa5f058c35f7a78bc15e97dc8ed Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 12:47:08 +0100 Subject: [PATCH 04/15] pydantic v1 BC --- interface_tester/collector.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/interface_tester/collector.py b/interface_tester/collector.py index 52657b3..47beaf5 100644 --- a/interface_tester/collector.py +++ b/interface_tester/collector.py @@ -105,10 +105,13 @@ def load_schema_module(schema_path: Path) -> types.ModuleType: if module_name in sys.modules: del sys.modules[module_name] - # Otherwise we'll get an error when we re-run @validator - # fixme: is there a better way to do this? - # logger.debug("Clearing pydantic.class_validators._FUNCS") - # pydantic.class_validators._FUNCS.clear() # noqa + if pydantic.version.VERSION.split(".") <= ["2"]: + # in pydantic v1 it's necessary; in v2 it isn't. + + # Otherwise we'll get an error when we re-run @validator + logger.debug("Clearing pydantic.class_validators._FUNCS") + pydantic.class_validators._FUNCS.clear() # noqa + try: module = importlib.import_module(module_name) From a62e58c2febcaeb6aeb0361b0e433b3ab5735b2b Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 13:18:51 +0100 Subject: [PATCH 05/15] error logging on test failure --- interface_tester/plugin.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/interface_tester/plugin.py b/interface_tester/plugin.py index 4dbb598..e0db9e2 100644 --- a/interface_tester/plugin.py +++ b/interface_tester/plugin.py @@ -328,8 +328,11 @@ def run(self) -> bool: with tester_context(ctx): test_fn() except Exception as e: + logger.error(f'Interface tester plugin failed with {e}', exc_info=True) + if self._RAISE_IMMEDIATELY: raise e + errors.append((ctx, e)) ran_some = True From 91e7fbb7b607e301d367f9dd9fdb8d2dc13d3edd Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 13:32:31 +0100 Subject: [PATCH 06/15] lint --- interface_tester/collector.py | 1 - interface_tester/plugin.py | 2 +- tests/unit/test_e2e.py | 22 +++++++++++----------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/interface_tester/collector.py b/interface_tester/collector.py index 47beaf5..d297455 100644 --- a/interface_tester/collector.py +++ b/interface_tester/collector.py @@ -112,7 +112,6 @@ def load_schema_module(schema_path: Path) -> types.ModuleType: logger.debug("Clearing pydantic.class_validators._FUNCS") pydantic.class_validators._FUNCS.clear() # noqa - try: module = importlib.import_module(module_name) except ImportError: diff --git a/interface_tester/plugin.py b/interface_tester/plugin.py index e0db9e2..0192b09 100644 --- a/interface_tester/plugin.py +++ b/interface_tester/plugin.py @@ -328,7 +328,7 @@ def run(self) -> bool: with tester_context(ctx): test_fn() except Exception as e: - logger.error(f'Interface tester plugin failed with {e}', exc_info=True) + logger.error(f"Interface tester plugin failed with {e}", exc_info=True) if self._RAISE_IMMEDIATELY: raise e diff --git a/tests/unit/test_e2e.py b/tests/unit/test_e2e.py index b5664ba..aa72814 100644 --- a/tests/unit/test_e2e.py +++ b/tests/unit/test_e2e.py @@ -107,7 +107,7 @@ def test_error_if_skip_schema_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -133,7 +133,7 @@ def test_error_if_assert_relation_data_empty_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -160,7 +160,7 @@ def test_error_if_assert_schema_valid_before_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -186,7 +186,7 @@ def test_error_if_assert_schema_without_schema(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -213,7 +213,7 @@ def test_error_if_return_before_schema_call(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -239,7 +239,7 @@ def test_error_if_return_without_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -285,7 +285,7 @@ def test_valid_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={} @@ -312,7 +312,7 @@ def test_valid_run_default_schema(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={"foo":"1"}, @@ -355,7 +355,7 @@ def test_default_schema_validation_failure(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={"foo":"abc"}, @@ -408,7 +408,7 @@ class FooBarSchema(DataBagSchema): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={"foo":"1"}, @@ -445,7 +445,7 @@ class FooBarSchema(DataBagSchema): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter: the charm says on what endpoint it uses 'tracing' + endpoint='foobadooble', # should not matter interface='tracing', remote_app_name='remote', local_app_data={"foo":"abc"}, From 8543e1671a950ac779045b93ff2f1695199c6a84 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 14:50:48 +0100 Subject: [PATCH 07/15] BC for pydantic v1 --- README.md | 3 ++- interface_tester/interface_test.py | 14 ++++++++++++-- tests/unit/test_collect_schemas.py | 10 ++++++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 7374b7e..5bdc9d4 100644 --- a/README.md +++ b/README.md @@ -84,4 +84,5 @@ You can customize name and location of the fixture, but you will need to include ``` ## Upgrading from v1 -As `pytest-interface-tester` v2 is using `pydantic` v2 that introduces breaking changes to their API, you might need to adjust your tested charm to also support v2. See [migration guide](https://docs.pydantic.dev/latest/migration/) for more information. \ No newline at end of file +`pytest-interface-tester` supports both pydantic v1 and v2, but using v2 is recommended. +You might need to adjust your tested charm to also support v2. See [migration guide](https://docs.pydantic.dev/latest/migration/) for more information. \ No newline at end of file diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index b9d1f5b..86144c2 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -10,6 +10,7 @@ from enum import Enum from typing import Any, Callable, List, Literal, Optional, Union +import pydantic from ops.testing import CharmType from pydantic import ValidationError from scenario import Context, Event, Relation, State @@ -28,6 +29,14 @@ logger = logging.getLogger(__name__) +_has_pydantic_v1 = pydantic.version.VERSION.split(".") <= ["2"] + + +def _validate(model: pydantic.BaseModel, obj: dict): + if _has_pydantic_v1: + return model.validate(obj) + return model.model_validate(obj) + class InvalidTestCase(RuntimeError): """Raised if a function decorated with interface_test_case is invalid.""" @@ -283,7 +292,8 @@ def assert_schema_valid(self, schema: Optional["DataBagSchema"] = None): errors = [] for relation in self._relations: try: - databag_schema.model_validate( + _validate( + databag_schema, { "unit": relation.local_unit_data, "app": relation.local_app_data, @@ -441,7 +451,7 @@ def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): return endpoints_for_interface[0] def _generate_relations_state( - self, state_template: State, input_state: State, supported_endpoints, role: Role + self, state_template: State, input_state: State, supported_endpoints, role: Role ) -> List[Relation]: """Merge the relations from the input state and the state template into one. diff --git a/tests/unit/test_collect_schemas.py b/tests/unit/test_collect_schemas.py index 9500021..5e97a1d 100644 --- a/tests/unit/test_collect_schemas.py +++ b/tests/unit/test_collect_schemas.py @@ -3,6 +3,7 @@ import pytest +from interface_tester.interface_test import _has_pydantic_v1 from interface_tester.collector import ( collect_tests, get_schema_from_module, @@ -70,8 +71,13 @@ class RequirerSchema(DataBagSchema): ) tests = collect_tests(root) - assert tests["mytestinterfacea"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 1 - assert tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 2 + if _has_pydantic_v1: + assert tests["mytestinterfacea"]["v0"]["requirer"]["schema"].__fields__["foo"].default == 1 + assert tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].__fields__["foo"].default == 2 + + else: + assert tests["mytestinterfacea"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 1 + assert tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 2 def test_collect_invalid_schemas(tmp_path): From 6a7727f6fc3e1ce0f5bd273aa92720981840da06 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 14:50:58 +0100 Subject: [PATCH 08/15] lint --- interface_tester/interface_test.py | 4 ++-- tests/unit/test_collect_schemas.py | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index 86144c2..4c013a1 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -297,7 +297,7 @@ def assert_schema_valid(self, schema: Optional["DataBagSchema"] = None): { "unit": relation.local_unit_data, "app": relation.local_app_data, - } + }, ) except ValidationError as e: errors.append(str(e)) @@ -451,7 +451,7 @@ def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): return endpoints_for_interface[0] def _generate_relations_state( - self, state_template: State, input_state: State, supported_endpoints, role: Role + self, state_template: State, input_state: State, supported_endpoints, role: Role ) -> List[Relation]: """Merge the relations from the input state and the state template into one. diff --git a/tests/unit/test_collect_schemas.py b/tests/unit/test_collect_schemas.py index 5e97a1d..7357da4 100644 --- a/tests/unit/test_collect_schemas.py +++ b/tests/unit/test_collect_schemas.py @@ -3,12 +3,12 @@ import pytest -from interface_tester.interface_test import _has_pydantic_v1 from interface_tester.collector import ( collect_tests, get_schema_from_module, load_schema_module, ) +from interface_tester.interface_test import _has_pydantic_v1 def test_load_schema_module(tmp_path): @@ -76,8 +76,12 @@ class RequirerSchema(DataBagSchema): assert tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].__fields__["foo"].default == 2 else: - assert tests["mytestinterfacea"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 1 - assert tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 2 + assert ( + tests["mytestinterfacea"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 1 + ) + assert ( + tests["mytestinterfaceb"]["v0"]["requirer"]["schema"].model_fields["foo"].default == 2 + ) def test_collect_invalid_schemas(tmp_path): From b8a635972faaf1d2eeea0991b8f095a12d2265b9 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 14:52:55 +0100 Subject: [PATCH 09/15] deprecation warning --- interface_tester/interface_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index 4c013a1..6616b2b 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -30,6 +30,11 @@ logger = logging.getLogger(__name__) _has_pydantic_v1 = pydantic.version.VERSION.split(".") <= ["2"] +if _has_pydantic_v1: + logger.warning( + "You seem to be using pydantic v1. " + "Please upgrade to v2, as compatibility may be dropped in a future version of pytest-interface-tester." + ) def _validate(model: pydantic.BaseModel, obj: dict): From 2faa3ecce9a830bc18150f1ec1e3c8c44caaa34c Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 14:57:02 +0100 Subject: [PATCH 10/15] lint --- interface_tester/interface_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index 6616b2b..65c09ee 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -33,7 +33,8 @@ if _has_pydantic_v1: logger.warning( "You seem to be using pydantic v1. " - "Please upgrade to v2, as compatibility may be dropped in a future version of pytest-interface-tester." + "Please upgrade to v2, as compatibility may be dropped in a future version of " + "pytest-interface-tester." ) From eb2feee35179aed52fde98097757ac289c818c16 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Wed, 7 Feb 2024 15:27:17 +0100 Subject: [PATCH 11/15] exception logging --- interface_tester/plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface_tester/plugin.py b/interface_tester/plugin.py index 0192b09..346fad3 100644 --- a/interface_tester/plugin.py +++ b/interface_tester/plugin.py @@ -328,7 +328,7 @@ def run(self) -> bool: with tester_context(ctx): test_fn() except Exception as e: - logger.error(f"Interface tester plugin failed with {e}", exc_info=True) + logger.exception(f"Interface tester plugin failed with {e}") if self._RAISE_IMMEDIATELY: raise e From ce32b29f041d7eb53fcabf7f6a6423f5e63871d2 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 9 Feb 2024 08:39:36 +0100 Subject: [PATCH 12/15] event path fix --- interface_tester/interface_test.py | 62 +++++++++++++----------------- interface_tester/plugin.py | 49 ++++++++++++----------- tests/unit/test_e2e.py | 24 ++++++------ 3 files changed, 63 insertions(+), 72 deletions(-) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index 65c09ee..fa34299 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -14,6 +14,7 @@ from ops.testing import CharmType from pydantic import ValidationError from scenario import Context, Event, Relation, State +from scenario.state import _EventPath from interface_tester.errors import InvalidTestCaseError, SchemaValidationError @@ -381,8 +382,7 @@ def _run(self, event: Union[str, Event]): # the Relation instance this test is about: relation = next(filter(lambda r: r.interface == self.ctx.interface_name, relations)) - # test.EVENT might be a string or an Event. Cast to Event. - evt: Event = self._coerce_event(event, relation) + evt: Event = self._cast_event(event, relation) logger.info("collected test for %s with %s" % (self.ctx.interface_name, evt.name)) return self._run_scenario(evt, modified_state) @@ -403,45 +403,35 @@ def _run_scenario(self, event: Event, state: State): ) return ctx.run(event, state) - def _coerce_event(self, raw_event: Union[str, Event], relation: Relation) -> Event: - # if the event being tested is a relation event, we need to inject some metadata - # or scenario.Runtime won't be able to guess what envvars need setting before ops.main - # takes over - if isinstance(raw_event, str): - ep_name, _, evt_kind = raw_event.rpartition("-relation-") - if ep_name and evt_kind: - # this is a relation event. - # we inject the relation metadata - # todo: if the user passes a relation event that is NOT about the relation - # interface that this test is about, at this point we are injecting the wrong - # Relation instance. - # e.g. if in interfaces/foo one wants to test that if 'bar-relation-joined' is - # fired... then one would have to pass an Event instance already with its - # own Relation. - return Event( - raw_event, - relation=relation.replace(endpoint=ep_name), - ) - - else: - return Event(raw_event) - - elif isinstance(raw_event, Event): - if raw_event._is_relation_event and not raw_event.relation: - raise InvalidTestCaseError( - "This test case was passed an Event representing a relation event." - "However it does not have a Relation. Please pass it to the Event like so: " - "evt = Event('my_relation_changed', relation=Relation(...))" - ) - - return raw_event + def _cast_event(self, raw_event: Union[str, Event], relation: Relation): + # test.EVENT might be a string or an Event. Cast to Event. + event = Event(raw_event) if isinstance(raw_event, str) else raw_event - else: + if not isinstance(event, Event): raise InvalidTestCaseError( f"Expected Event or str, not {type(raw_event)}. " f"Invalid test case: {self} cannot cast {raw_event} to Event." ) + if not event._is_relation_event: + raise InvalidTestCaseError(f"Bad interface test specification: event {raw_event} " + "is not a relation event.") + + # todo: if the user passes a relation event that is NOT about the relation + # interface that this test is about, at this point we are injecting the wrong + # Relation instance. + # e.g. if in interfaces/foo one wants to test that if 'bar-relation-joined' is + # fired... then one would have to pass an Event instance already with its + # own Relation. + + # next we need to ensure that the event's .relation is our relation, and that the endpoint + # in the relation and the event path match that of the charm we're testing. + charm_event = event.replace( + relation=relation, + path=relation.endpoint + typing.cast(_EventPath, event.path).suffix) + + return charm_event + @staticmethod def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): endpoints_for_interface = supported_endpoints[role] @@ -457,7 +447,7 @@ def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): return endpoints_for_interface[0] def _generate_relations_state( - self, state_template: State, input_state: State, supported_endpoints, role: Role + self, state_template: State, input_state: State, supported_endpoints, role: Role ) -> List[Relation]: """Merge the relations from the input state and the state template into one. diff --git a/interface_tester/plugin.py b/interface_tester/plugin.py index 346fad3..1288ba3 100644 --- a/interface_tester/plugin.py +++ b/interface_tester/plugin.py @@ -7,7 +7,7 @@ from typing import Any, Callable, Dict, Generator, List, Optional, Tuple, Type from ops.testing import CharmType -from scenario.state import Event, MetadataNotFoundError, State, _CharmSpec +from scenario.state import MetadataNotFoundError, State, _CharmSpec from interface_tester.collector import InterfaceTestSpec, gather_test_spec_for_version from interface_tester.errors import ( @@ -22,7 +22,6 @@ ) from interface_tester.schema_base import DataBagSchema -Callback = Callable[[State, Event], None] ROLE_TO_ROLE_META = {"provider": "provides", "requirer": "requires"} logger = logging.getLogger("pytest_interface_tester") @@ -32,10 +31,10 @@ class InterfaceTester: _RAISE_IMMEDIATELY = False def __init__( - self, - repo: str = "https://github.com/canonical/charm-relation-interfaces", - branch: str = "main", - base_path: str = "interfaces", + self, + repo: str = "https://github.com/canonical/charm-relation-interfaces", + branch: str = "main", + base_path: str = "interfaces", ): self._repo = repo self._branch = branch @@ -54,19 +53,19 @@ def __init__( self._charm_spec_cache = None def configure( - self, - *, - charm_type: Optional[Type[CharmType]] = None, - repo: Optional[str] = None, - branch: Optional[str] = None, - base_path: Optional[str] = None, - interface_name: Optional[str] = None, - interface_version: Optional[int] = None, - state_template: Optional[State] = None, - juju_version: Optional[str] = None, - meta: Optional[Dict[str, Any]] = None, - actions: Optional[Dict[str, Any]] = None, - config: Optional[Dict[str, Any]] = None, + self, + *, + charm_type: Optional[Type[CharmType]] = None, + repo: Optional[str] = None, + branch: Optional[str] = None, + base_path: Optional[str] = None, + interface_name: Optional[str] = None, + interface_version: Optional[int] = None, + state_template: Optional[State] = None, + juju_version: Optional[str] = None, + meta: Optional[Dict[str, Any]] = None, + actions: Optional[Dict[str, Any]] = None, + config: Optional[Dict[str, Any]] = None, ): """ @@ -198,11 +197,11 @@ def _collect_interface_test_specs(self) -> InterfaceTestSpec: repo_name = repo_name.rsplit(".", maxsplit=1)[0] intf_spec_path = ( - Path(tempdir) - / repo_name - / self._base_path - / self._interface_name.replace("-", "_") - / f"v{self._interface_version}" + Path(tempdir) + / repo_name + / self._base_path + / self._interface_name.replace("-", "_") + / f"v{self._interface_version}" ) if not intf_spec_path.exists(): raise RuntimeError( @@ -248,7 +247,7 @@ def _gather_supported_endpoints(self) -> Dict[RoleLiteral, List[str]]: return supported_endpoints def _yield_tests( - self, + self, ) -> Generator[Tuple[Callable, RoleLiteral, DataBagSchema], None, None]: """Yield all test cases applicable to this charm and interface. diff --git a/tests/unit/test_e2e.py b/tests/unit/test_e2e.py index aa72814..cdce24b 100644 --- a/tests/unit/test_e2e.py +++ b/tests/unit/test_e2e.py @@ -40,8 +40,9 @@ def interface_tester(): charm_type=DummiCharm, meta={ "name": "dummi", - "provides": {"tracing": {"interface": "tracing"}}, - "requires": {"tracing-req": {"interface": "tracing"}}, + # interface tests should be agnostic to endpoint names + "provides": {"dead": {"interface": "tracing"}}, + "requires": {"beef-req": {"interface": "tracing"}}, }, state_template=State(leader=True), ) @@ -87,8 +88,9 @@ def _collect_interface_test_specs(self): charm_type=DummiCharm, meta={ "name": "dummi", - "provides": {"tracing": {"interface": "tracing"}}, - "requires": {"tracing-req": {"interface": "tracing"}}, + # interface tests should be agnostic to endpoint names + "provides": {"dead": {"interface": "tracing"}}, + "requires": {"beef-req": {"interface": "tracing"}}, }, state_template=State(leader=True), ) @@ -192,7 +194,7 @@ def test_data_on_changed(): local_app_data={} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid() """ ) @@ -219,7 +221,7 @@ def test_data_on_changed(): local_app_data={} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") """ ) ) @@ -291,7 +293,7 @@ def test_data_on_changed(): local_app_data={} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid(schema=DataBagSchema()) """ ) @@ -319,7 +321,7 @@ def test_data_on_changed(): local_unit_data={"bar": "smackbeef"} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid() """ ), @@ -362,7 +364,7 @@ def test_data_on_changed(): local_unit_data={"bar": "smackbeef"} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid() """ ), @@ -415,7 +417,7 @@ def test_data_on_changed(): local_unit_data={"bar": "smackbeef"} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid(schema=FooBarSchema) """ ) @@ -452,7 +454,7 @@ def test_data_on_changed(): local_unit_data={"bar": "smackbeef"} )] )) - state_out = t.run("tracing-relation-changed") + state_out = t.run("axolotl-relation-changed") t.assert_schema_valid(schema=FooBarSchema) """ ) From b352f016bf49e6bda60a8b9c2271e3a5f49edf47 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 9 Feb 2024 08:46:11 +0100 Subject: [PATCH 13/15] adapted to scenario 6.0.1 --- interface_tester/interface_test.py | 24 ++++++++++------ interface_tester/plugin.py | 46 +++++++++++++++--------------- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/interface_tester/interface_test.py b/interface_tester/interface_test.py index fa34299..daa008c 100644 --- a/interface_tester/interface_test.py +++ b/interface_tester/interface_test.py @@ -13,7 +13,7 @@ import pydantic from ops.testing import CharmType from pydantic import ValidationError -from scenario import Context, Event, Relation, State +from scenario import Context, Event, Relation, State, state from scenario.state import _EventPath from interface_tester.errors import InvalidTestCaseError, SchemaValidationError @@ -323,9 +323,16 @@ def assert_relation_data_empty(self): raise SchemaValidationError( f"test {self._test_id}: local app databag not empty for {relation}" ) - if relation.local_unit_data: + + # remove the default unit databag keys or we'll get false positives. + local_unit_data_keys = set(relation.local_unit_data).difference( + set(state.DEFAULT_JUJU_DATABAG.keys()) + ) + + if local_unit_data_keys: raise SchemaValidationError( - f"test {self._test_id}: local unit databag not empty for {relation}" + f"test {self._test_id}: local unit databag not empty for {relation}: " + f"found {local_unit_data_keys!r} keys set" ) self._has_checked_schema = True @@ -414,8 +421,9 @@ def _cast_event(self, raw_event: Union[str, Event], relation: Relation): ) if not event._is_relation_event: - raise InvalidTestCaseError(f"Bad interface test specification: event {raw_event} " - "is not a relation event.") + raise InvalidTestCaseError( + f"Bad interface test specification: event {raw_event} " "is not a relation event." + ) # todo: if the user passes a relation event that is NOT about the relation # interface that this test is about, at this point we are injecting the wrong @@ -427,8 +435,8 @@ def _cast_event(self, raw_event: Union[str, Event], relation: Relation): # next we need to ensure that the event's .relation is our relation, and that the endpoint # in the relation and the event path match that of the charm we're testing. charm_event = event.replace( - relation=relation, - path=relation.endpoint + typing.cast(_EventPath, event.path).suffix) + relation=relation, path=relation.endpoint + typing.cast(_EventPath, event.path).suffix + ) return charm_event @@ -447,7 +455,7 @@ def _get_endpoint(supported_endpoints: dict, role: Role, interface_name: str): return endpoints_for_interface[0] def _generate_relations_state( - self, state_template: State, input_state: State, supported_endpoints, role: Role + self, state_template: State, input_state: State, supported_endpoints, role: Role ) -> List[Relation]: """Merge the relations from the input state and the state template into one. diff --git a/interface_tester/plugin.py b/interface_tester/plugin.py index 1288ba3..26331cd 100644 --- a/interface_tester/plugin.py +++ b/interface_tester/plugin.py @@ -31,10 +31,10 @@ class InterfaceTester: _RAISE_IMMEDIATELY = False def __init__( - self, - repo: str = "https://github.com/canonical/charm-relation-interfaces", - branch: str = "main", - base_path: str = "interfaces", + self, + repo: str = "https://github.com/canonical/charm-relation-interfaces", + branch: str = "main", + base_path: str = "interfaces", ): self._repo = repo self._branch = branch @@ -53,19 +53,19 @@ def __init__( self._charm_spec_cache = None def configure( - self, - *, - charm_type: Optional[Type[CharmType]] = None, - repo: Optional[str] = None, - branch: Optional[str] = None, - base_path: Optional[str] = None, - interface_name: Optional[str] = None, - interface_version: Optional[int] = None, - state_template: Optional[State] = None, - juju_version: Optional[str] = None, - meta: Optional[Dict[str, Any]] = None, - actions: Optional[Dict[str, Any]] = None, - config: Optional[Dict[str, Any]] = None, + self, + *, + charm_type: Optional[Type[CharmType]] = None, + repo: Optional[str] = None, + branch: Optional[str] = None, + base_path: Optional[str] = None, + interface_name: Optional[str] = None, + interface_version: Optional[int] = None, + state_template: Optional[State] = None, + juju_version: Optional[str] = None, + meta: Optional[Dict[str, Any]] = None, + actions: Optional[Dict[str, Any]] = None, + config: Optional[Dict[str, Any]] = None, ): """ @@ -197,11 +197,11 @@ def _collect_interface_test_specs(self) -> InterfaceTestSpec: repo_name = repo_name.rsplit(".", maxsplit=1)[0] intf_spec_path = ( - Path(tempdir) - / repo_name - / self._base_path - / self._interface_name.replace("-", "_") - / f"v{self._interface_version}" + Path(tempdir) + / repo_name + / self._base_path + / self._interface_name.replace("-", "_") + / f"v{self._interface_version}" ) if not intf_spec_path.exists(): raise RuntimeError( @@ -247,7 +247,7 @@ def _gather_supported_endpoints(self) -> Dict[RoleLiteral, List[str]]: return supported_endpoints def _yield_tests( - self, + self, ) -> Generator[Tuple[Callable, RoleLiteral, DataBagSchema], None, None]: """Yield all test cases applicable to this charm and interface. From 33c0362b44009e93376134762025c790a7b48f18 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 9 Feb 2024 12:29:13 +0100 Subject: [PATCH 14/15] broad test on e2e endpoint names --- tests/unit/test_e2e.py | 54 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_e2e.py b/tests/unit/test_e2e.py index cdce24b..3c3c550 100644 --- a/tests/unit/test_e2e.py +++ b/tests/unit/test_e2e.py @@ -5,17 +5,17 @@ import pytest from ops import CharmBase from scenario import State -from utils import CRI_LIKE_PATH from interface_tester import InterfaceTester from interface_tester.collector import gather_test_spec_for_version -from interface_tester.errors import SchemaValidationError +from interface_tester.errors import SchemaValidationError, InvalidTestCaseError from interface_tester.interface_test import ( InvalidTesterRunError, NoSchemaError, NoTesterInstanceError, Tester, ) +from utils import CRI_LIKE_PATH class LocalTester(InterfaceTester): @@ -124,6 +124,35 @@ def test_data_on_changed(): tester.run() +def test_error_if_not_relation_event(): + tester = _setup_with_test_file( + dedent( + """ +from scenario import State, Relation + +from interface_tester.interface_test import Tester + +def test_data_on_changed(): + t = Tester(State( + relations=[Relation( + endpoint='foobadooble', # should not matter + interface='tracing', + remote_app_name='remote', + local_app_data={} + )] + )) + t.run("foobadooble-changed") + t.skip_schema_validation() +""" + ) + ) + + with pytest.raises(InvalidTestCaseError) as e: + tester.run() + + assert e.match("Bad interface test specification: event foobadooble-changed is not a relation event.") + + def test_error_if_assert_relation_data_empty_before_run(): tester = _setup_with_test_file( dedent( @@ -275,10 +304,21 @@ def test_data_on_changed(): tester.run() -def test_valid_run(): +@pytest.mark.parametrize("endpoint", ( + "foo-one", + "prometheus-scrape", + "foobadoodle", + "foo-one-two")) +@pytest.mark.parametrize("evt_type", ( + "changed", + "created", + "joined", + "departed", + "broken")) +def test_valid_run(endpoint, evt_type): tester = _setup_with_test_file( dedent( - """ + f""" from scenario import State, Relation from interface_tester.interface_test import Tester @@ -287,13 +327,13 @@ def test_valid_run(): def test_data_on_changed(): t = Tester(State( relations=[Relation( - endpoint='foobadooble', # should not matter + endpoint='{endpoint}', # should not matter interface='tracing', remote_app_name='remote', - local_app_data={} + local_app_data={{}} )] )) - state_out = t.run("axolotl-relation-changed") + state_out = t.run("{endpoint}-relation-{evt_type}") t.assert_schema_valid(schema=DataBagSchema()) """ ) From 8f61287f980cc9c27e14c4aaf905ea74d7745797 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 9 Feb 2024 13:50:00 +0100 Subject: [PATCH 15/15] lint --- tests/unit/test_e2e.py | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/tests/unit/test_e2e.py b/tests/unit/test_e2e.py index 3c3c550..e6f9f12 100644 --- a/tests/unit/test_e2e.py +++ b/tests/unit/test_e2e.py @@ -5,17 +5,17 @@ import pytest from ops import CharmBase from scenario import State +from utils import CRI_LIKE_PATH from interface_tester import InterfaceTester from interface_tester.collector import gather_test_spec_for_version -from interface_tester.errors import SchemaValidationError, InvalidTestCaseError +from interface_tester.errors import InvalidTestCaseError, SchemaValidationError from interface_tester.interface_test import ( InvalidTesterRunError, NoSchemaError, NoTesterInstanceError, Tester, ) -from utils import CRI_LIKE_PATH class LocalTester(InterfaceTester): @@ -150,7 +150,9 @@ def test_data_on_changed(): with pytest.raises(InvalidTestCaseError) as e: tester.run() - assert e.match("Bad interface test specification: event foobadooble-changed is not a relation event.") + assert e.match( + "Bad interface test specification: event foobadooble-changed is not a relation event." + ) def test_error_if_assert_relation_data_empty_before_run(): @@ -304,17 +306,10 @@ def test_data_on_changed(): tester.run() -@pytest.mark.parametrize("endpoint", ( - "foo-one", - "prometheus-scrape", - "foobadoodle", - "foo-one-two")) -@pytest.mark.parametrize("evt_type", ( - "changed", - "created", - "joined", - "departed", - "broken")) +@pytest.mark.parametrize( + "endpoint", ("foo-one", "prometheus-scrape", "foobadoodle", "foo-one-two") +) +@pytest.mark.parametrize("evt_type", ("changed", "created", "joined", "departed", "broken")) def test_valid_run(endpoint, evt_type): tester = _setup_with_test_file( dedent(