From dbb9b0f2e78dfb9a428e9e29abeaba96141ff499 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 15:49:51 +1300 Subject: [PATCH 01/15] perf: cache yaml load when autoloading charm spec --- testing/src/scenario/state.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index a5274ebf6..16e4b5894 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -8,6 +8,7 @@ import copy import dataclasses import datetime +import functools import inspect import pathlib import random @@ -1878,25 +1879,15 @@ def _load_metadata_legacy(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft < 2.5.""" # back in the days, we used to have separate metadata.yaml, config.yaml and actions.yaml # files for charm metadata. - metadata_path = charm_root / 'metadata.yaml' - meta: dict[str, Any] = ( - yaml.safe_load(metadata_path.open()) if metadata_path.exists() else {} - ) - - config_path = charm_root / 'config.yaml' - config = yaml.safe_load(config_path.open()) if config_path.exists() else None - - actions_path = charm_root / 'actions.yaml' - actions = yaml.safe_load(actions_path.open()) if actions_path.exists() else None + meta: dict[str, Any] = _load_yaml(charm_root / 'metadata.yaml') or {} + config = _load_yaml(charm_root / 'config.yaml') + actions = _load_yaml(charm_root / 'actions.yaml') return meta, config, actions @staticmethod def _load_metadata(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft >= 2.5.""" - metadata_path = charm_root / 'charmcraft.yaml' - meta: dict[str, Any] = ( - yaml.safe_load(metadata_path.open()) if metadata_path.exists() else {} - ) + meta: dict[str, Any] = _load_yaml(charm_root / 'charmcraft.yaml') or {} if not _is_valid_charmcraft_25_metadata(meta): meta = {} config = meta.pop('config', None) @@ -2356,3 +2347,10 @@ def test_backup_action(): Every action invocation is automatically assigned a new one. Override in the rare cases where a specific ID is required.""" + + +@functools.lru_cache +def _load_yaml(path: pathlib.Path) -> Any | None: + if path.exists(): + return yaml.safe_load(path.read_text()) + return None From 3a56a787da877e02e29aa702bacfb431568704dd Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 15:56:13 +1300 Subject: [PATCH 02/15] fix: use absolute path to charm root for caching load --- testing/src/scenario/state.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index 16e4b5894..f70a2d083 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -1879,6 +1879,7 @@ def _load_metadata_legacy(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft < 2.5.""" # back in the days, we used to have separate metadata.yaml, config.yaml and actions.yaml # files for charm metadata. + charm_root = charm_root.absolute() meta: dict[str, Any] = _load_yaml(charm_root / 'metadata.yaml') or {} config = _load_yaml(charm_root / 'config.yaml') actions = _load_yaml(charm_root / 'actions.yaml') @@ -1887,7 +1888,7 @@ def _load_metadata_legacy(charm_root: pathlib.Path): @staticmethod def _load_metadata(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft >= 2.5.""" - meta: dict[str, Any] = _load_yaml(charm_root / 'charmcraft.yaml') or {} + meta: dict[str, Any] = _load_yaml(charm_root.absolute() / 'charmcraft.yaml') or {} if not _is_valid_charmcraft_25_metadata(meta): meta = {} config = meta.pop('config', None) From b604b0321e6c1a448c8e0251c6da100fabd44810 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:14:58 +1300 Subject: [PATCH 03/15] test: add debugging asserts to _load_yaml --- testing/src/scenario/state.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index f70a2d083..b32546c6d 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -2350,8 +2350,13 @@ def test_backup_action(): the rare cases where a specific ID is required.""" -@functools.lru_cache +_CACHE = {} def _load_yaml(path: pathlib.Path) -> Any | None: if path.exists(): - return yaml.safe_load(path.read_text()) + loaded = yaml.safe_load(path.read_text()) + if path in _CACHE: + assert loaded == _CACHE[path] + else: + _CACHE[path] = loaded + return loaded return None From 95c07ddb4f577fb4d526e62979128629a78c5d1c Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:18:41 +1300 Subject: [PATCH 04/15] test: make unit tests verbose --- .github/workflows/observability-charm-tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/observability-charm-tests.yaml b/.github/workflows/observability-charm-tests.yaml index 5f2d665ef..c5632c88f 100644 --- a/.github/workflows/observability-charm-tests.yaml +++ b/.github/workflows/observability-charm-tests.yaml @@ -78,7 +78,7 @@ jobs: - name: Run the charm's unit tests if: ${{ !(matrix.disabled) }} - run: tox -vve unit + run: tox -vve unit -- -vv - name: Check if 'scenario' tox environment exists id: check-tox-env-scenario From 1bdfd195738a6eb936f939e58390101a211d45ea Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:22:48 +1300 Subject: [PATCH 05/15] test: add logging --- testing/src/scenario/state.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index b32546c6d..cd969e88a 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -2352,10 +2352,14 @@ def test_backup_action(): _CACHE = {} def _load_yaml(path: pathlib.Path) -> Any | None: + logger.warning(path) if path.exists(): loaded = yaml.safe_load(path.read_text()) if path in _CACHE: - assert loaded == _CACHE[path] + if loaded != _CACHE[path]: + logger.error(loaded) + logger.error(_CACHE[path]) + assert False else: _CACHE[path] = loaded return loaded From 2262cfcb46f00d6be6c3b0e58b4dbfb6c4b93448 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:28:43 +1300 Subject: [PATCH 06/15] fix: avoid accidental mutation --- testing/src/scenario/state.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index cd969e88a..19452c9a8 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -1891,8 +1891,10 @@ def _load_metadata(charm_root: pathlib.Path): meta: dict[str, Any] = _load_yaml(charm_root.absolute() / 'charmcraft.yaml') or {} if not _is_valid_charmcraft_25_metadata(meta): meta = {} - config = meta.pop('config', None) - actions = meta.pop('actions', None) + if 'config' in meta or 'actions' in meta: + meta = {**meta} + config = meta.pop('config', None) + actions = meta.pop('actions', None) return meta, config, actions @staticmethod From b841ae4fb78344050ae50ab0424c668552b1b5b2 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:31:17 +1300 Subject: [PATCH 07/15] fix: handle else case --- testing/src/scenario/state.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index 19452c9a8..f7a6d721f 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -1895,6 +1895,9 @@ def _load_metadata(charm_root: pathlib.Path): meta = {**meta} config = meta.pop('config', None) actions = meta.pop('actions', None) + else: + config = None + actions = None return meta, config, actions @staticmethod From d895b8183d5c11f88413de52bb32de3b0acce84f Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:31:59 +1300 Subject: [PATCH 08/15] chore: restore lru_cache use now that we've debugged things --- testing/src/scenario/state.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index f7a6d721f..f18b25470 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -2355,17 +2355,8 @@ def test_backup_action(): the rare cases where a specific ID is required.""" -_CACHE = {} +@functools.lru_cache def _load_yaml(path: pathlib.Path) -> Any | None: - logger.warning(path) if path.exists(): - loaded = yaml.safe_load(path.read_text()) - if path in _CACHE: - if loaded != _CACHE[path]: - logger.error(loaded) - logger.error(_CACHE[path]) - assert False - else: - _CACHE[path] = loaded - return loaded + return yaml.safe_load(path.read_text()) return None From 04adbceebf06146dae79a31f4cabbeed1ab1d00a Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:32:29 +1300 Subject: [PATCH 09/15] style: format --- testing/src/scenario/state.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index f18b25470..10a6b80f0 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -1880,15 +1880,15 @@ def _load_metadata_legacy(charm_root: pathlib.Path): # back in the days, we used to have separate metadata.yaml, config.yaml and actions.yaml # files for charm metadata. charm_root = charm_root.absolute() - meta: dict[str, Any] = _load_yaml(charm_root / 'metadata.yaml') or {} - config = _load_yaml(charm_root / 'config.yaml') - actions = _load_yaml(charm_root / 'actions.yaml') + meta: dict[str, Any] = _load_yaml(charm_root / 'metadata.yaml') or {} + config = _load_yaml(charm_root / 'config.yaml') + actions = _load_yaml(charm_root / 'actions.yaml') return meta, config, actions @staticmethod def _load_metadata(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft >= 2.5.""" - meta: dict[str, Any] = _load_yaml(charm_root.absolute() / 'charmcraft.yaml') or {} + meta: dict[str, Any] = _load_yaml(charm_root.absolute() / 'charmcraft.yaml') or {} if not _is_valid_charmcraft_25_metadata(meta): meta = {} if 'config' in meta or 'actions' in meta: From 55c9f66bff2fbc9e1e7182eae3cba8ebd4172358 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 16:48:21 +1300 Subject: [PATCH 10/15] perf: use already loaded _CharmSpec to create CharmMeta --- testing/src/scenario/_ops_main_mock.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/testing/src/scenario/_ops_main_mock.py b/testing/src/scenario/_ops_main_mock.py index c0c3e8d1d..31c406567 100644 --- a/testing/src/scenario/_ops_main_mock.py +++ b/testing/src/scenario/_ops_main_mock.py @@ -148,13 +148,11 @@ def __init__( super().__init__(self.charm_spec.charm_type, model_backend, juju_context=juju_context) def _load_charm_meta(self): - metadata = (self._charm_root / 'metadata.yaml').read_text() - actions_meta = self._charm_root / 'actions.yaml' - actions_metadata = actions_meta.read_text() if actions_meta.exists() else None - config_meta = self._charm_root / 'config.yaml' - config_metadata = config_meta.read_text() if config_meta.exists() else None - - return ops.CharmMeta.from_yaml(metadata, actions_metadata, config_metadata) + return ops.CharmMeta( + self.charm_spec.meta, + self.charm_spec.actions or {}, + self.charm_spec.config or {}, + ) @property def _framework_class(self): From 5585982e6a4fbf7ad9137f946c3393a2fdeeb6a0 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 17:09:39 +1300 Subject: [PATCH 11/15] chore: revert workflow change --- .github/workflows/observability-charm-tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/observability-charm-tests.yaml b/.github/workflows/observability-charm-tests.yaml index c5632c88f..5f2d665ef 100644 --- a/.github/workflows/observability-charm-tests.yaml +++ b/.github/workflows/observability-charm-tests.yaml @@ -78,7 +78,7 @@ jobs: - name: Run the charm's unit tests if: ${{ !(matrix.disabled) }} - run: tox -vve unit -- -vv + run: tox -vve unit - name: Check if 'scenario' tox environment exists id: check-tox-env-scenario From 4c49ceb210892b54382d47c5e42752712188b827 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 17:27:10 +1300 Subject: [PATCH 12/15] perf: don't write metadata files out at all --- testing/src/scenario/_runtime.py | 7 +++++++ testing/tests/test_e2e/test_vroot.py | 9 ++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/testing/src/scenario/_runtime.py b/testing/src/scenario/_runtime.py index fad5ffe90..7c92c35ab 100644 --- a/testing/src/scenario/_runtime.py +++ b/testing/src/scenario/_runtime.py @@ -221,6 +221,13 @@ def _virtual_charm_root(self): virtual_charm_root = Path(charm_virtual_root.name) charm_virtual_root_is_custom = False + yield virtual_charm_root + + if not charm_virtual_root_is_custom: + # charm_virtual_root is a tempdir + typing.cast('tempfile.TemporaryDirectory', charm_virtual_root).cleanup() # type: ignore + return + metadata_yaml = virtual_charm_root / 'metadata.yaml' config_yaml = virtual_charm_root / 'config.yaml' actions_yaml = virtual_charm_root / 'actions.yaml' diff --git a/testing/tests/test_e2e/test_vroot.py b/testing/tests/test_e2e/test_vroot.py index e5ee12a96..4657c9ab2 100644 --- a/testing/tests/test_e2e/test_vroot.py +++ b/testing/tests/test_e2e/test_vroot.py @@ -69,15 +69,11 @@ def test_charm_virtual_root_cleanup_if_exists(charm_virtual_root): State(), ) as mgr: assert meta_file.exists() - assert meta_file.read_text() == yaml.safe_dump({'name': 'my-charm'}) + assert meta_file.read_text() == raw_ori_meta # we don't write metadata to temp dir assert mgr.charm.meta.name == 'my-charm' # not karl! Context.meta takes precedence mgr.run() assert meta_file.exists() - # meta file was restored to its previous contents - assert meta_file.read_text() == raw_ori_meta - assert meta_file.exists() - def test_charm_virtual_root_cleanup_if_not_exists(charm_virtual_root): meta_file = charm_virtual_root / 'metadata.yaml' @@ -89,8 +85,7 @@ def test_charm_virtual_root_cleanup_if_not_exists(charm_virtual_root): ctx.on.start(), State(), ) as mgr: - assert meta_file.exists() - assert meta_file.read_text() == yaml.safe_dump({'name': 'my-charm'}) + assert not meta_file.exists() # we don't write metadata to temp dir mgr.run() assert not meta_file.exists() From 455c2b4ec07c83b2c16beb7743dfe8568a9cae49 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 17:52:20 +1300 Subject: [PATCH 13/15] refactor: clean up unused code to make diff clearer --- testing/src/scenario/_runtime.py | 78 +++----------------------------- 1 file changed, 7 insertions(+), 71 deletions(-) diff --git a/testing/src/scenario/_runtime.py b/testing/src/scenario/_runtime.py index 7c92c35ab..335a0fc48 100644 --- a/testing/src/scenario/_runtime.py +++ b/testing/src/scenario/_runtime.py @@ -14,8 +14,6 @@ from pathlib import Path from typing import TYPE_CHECKING -import yaml - from ops import JujuContext, pebble from ops._main import _Abort from ops._private.harness import ActionFailed @@ -206,79 +204,17 @@ class WrappedCharm(charm_type): @contextmanager def _virtual_charm_root(self): - # If we are using runtime on a real charm, we can make some assumptions about the - # directory structure we are going to find. - # If we're, say, dynamically defining charm types and doing tests on them, we'll have to - # generate the metadata files ourselves. To be sure, we ALWAYS use a tempdir. Ground truth - # is what the user passed via the CharmSpec - spec = self._charm_spec - - if charm_virtual_root := self._charm_root: - charm_virtual_root_is_custom = True - virtual_charm_root = Path(charm_virtual_root) + if self._charm_root: + tmp_dir = None + virtual_charm_root = Path(self._charm_root) else: - charm_virtual_root = tempfile.TemporaryDirectory() - virtual_charm_root = Path(charm_virtual_root.name) - charm_virtual_root_is_custom = False + tmp_dir = tempfile.TemporaryDirectory() + virtual_charm_root = Path(tmp_dir.name) yield virtual_charm_root - if not charm_virtual_root_is_custom: - # charm_virtual_root is a tempdir - typing.cast('tempfile.TemporaryDirectory', charm_virtual_root).cleanup() # type: ignore - return - - metadata_yaml = virtual_charm_root / 'metadata.yaml' - config_yaml = virtual_charm_root / 'config.yaml' - actions_yaml = virtual_charm_root / 'actions.yaml' - - metadata_files_present: dict[Path, str | None] = { - file: file.read_text() if charm_virtual_root_is_custom and file.exists() else None - for file in (metadata_yaml, config_yaml, actions_yaml) - } - - any_metadata_files_present_in_charm_virtual_root = any( - v is not None for v in metadata_files_present.values() - ) - - if spec.is_autoloaded and charm_virtual_root_is_custom: - # since the spec is autoloaded, in theory the metadata contents won't differ, so we can - # overwrite away even if the custom vroot is the real charm root (the local repo). - # Still, log it for clarity. - if any_metadata_files_present_in_charm_virtual_root: - logger.debug( - f'metadata files found in custom charm_root {charm_virtual_root}. ' - f'The spec was autoloaded so the contents should be identical. ' - f'Proceeding...', - ) - - elif not spec.is_autoloaded and any_metadata_files_present_in_charm_virtual_root: - logger.warning( - f'Some metadata files found in custom user-provided charm_root ' - f'{charm_virtual_root} while you have passed meta, config or actions to ' - f'Context.run(). ' - 'Single source of truth are the arguments passed to Context.run(). ' - 'charm_root metadata files will be overwritten for the ' - 'duration of this test, and restored afterwards. ' - 'To avoid this, clean any metadata files from the charm_root before calling run.', - ) - - metadata_yaml.write_text(yaml.safe_dump(spec.meta)) - config_yaml.write_text(yaml.safe_dump(spec.config or {})) - actions_yaml.write_text(yaml.safe_dump(spec.actions or {})) - - yield virtual_charm_root - - if charm_virtual_root_is_custom: - for file, previous_content in metadata_files_present.items(): - if previous_content is None: # None == file did not exist before - file.unlink() - else: - file.write_text(previous_content) - - else: - # charm_virtual_root is a tempdir - typing.cast('tempfile.TemporaryDirectory', charm_virtual_root).cleanup() # type: ignore + if tmp_dir is not None: + tmp_dir.cleanup() @contextmanager def exec( From f848e1adc441d14608486c68a273de5052c5fada Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 18:14:19 +1300 Subject: [PATCH 14/15] refactor: tidy up since we have less to do now --- testing/src/scenario/_runtime.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/testing/src/scenario/_runtime.py b/testing/src/scenario/_runtime.py index 335a0fc48..7670cb922 100644 --- a/testing/src/scenario/_runtime.py +++ b/testing/src/scenario/_runtime.py @@ -205,16 +205,10 @@ class WrappedCharm(charm_type): @contextmanager def _virtual_charm_root(self): if self._charm_root: - tmp_dir = None - virtual_charm_root = Path(self._charm_root) - else: - tmp_dir = tempfile.TemporaryDirectory() - virtual_charm_root = Path(tmp_dir.name) - - yield virtual_charm_root - - if tmp_dir is not None: - tmp_dir.cleanup() + yield Path(self._charm_root) + return + with tempfile.TemporaryDirectory() as tmp_dir: + yield Path(tmp_dir.name) @contextmanager def exec( From 547765d92da45dc2d171c5040c7230b1a6d988d8 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 2 Mar 2026 18:20:01 +1300 Subject: [PATCH 15/15] fix: don't use a context manager, keep using cleanup explicitly --- testing/src/scenario/_runtime.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/testing/src/scenario/_runtime.py b/testing/src/scenario/_runtime.py index 7670cb922..3df87de7a 100644 --- a/testing/src/scenario/_runtime.py +++ b/testing/src/scenario/_runtime.py @@ -207,8 +207,9 @@ def _virtual_charm_root(self): if self._charm_root: yield Path(self._charm_root) return - with tempfile.TemporaryDirectory() as tmp_dir: - yield Path(tmp_dir.name) + tmp_dir = tempfile.TemporaryDirectory() + yield Path(tmp_dir.name) + tmp_dir.cleanup() @contextmanager def exec(