test(storage-constraints): update tests to reflect new logic

Unit tests have been updated to reflect the fact that parsing of storage
constraints no longer happens before calling Model._deploy. Due to the
difficulty in mocking the _deploy internals, the bundle test of parsing
many storage variations has been moved to test_parse_storage_constraints
in test_constraints.

Two integration tests have been added to test deployment with parsed and
unparsed storage arguments respectively.

Author: james-garner-canonical

tests/integration/test_model.py

@@ -45,6 +45,42 @@ async def test_model_name():
         await model.disconnect()
 
 
+@base.bootstrapped
+async def test_deploy_with_storage_unparsed():
+    async with base.CleanModel() as model:
+        await model.deploy(
+            "postgresql",
+            storage={"pgdata": "1G"},
+        )
+        await model.wait_for_idle(status="active")
+        storages = await model.list_storage()
+        assert len(storages) == 1
+        [storage] = storages
+        # size information isn't exposed, so can't assert on that
+        assert storage["owner-tag"].startswith(tag.unit("postgresql"))
+        assert storage["storage-tag"].startswith(tag.storage("pgdata"))
+        assert storage["life"] == "alive"
+        assert storage["status"].status == "attached"
+
+
+@base.bootstrapped
+async def test_deploy_with_storage_preparsed():
+    async with base.CleanModel() as model:
+        await model.deploy(
+            "postgresql",
+            storage={"pgdata": {"size": 1024, "count": 1}},
+        )
+        await model.wait_for_idle(status="active")
+        storages = await model.list_storage()
+        assert len(storages) == 1
+        [storage] = storages
+        # size information isn't exposed, so can't assert on that
+        assert storage["owner-tag"].startswith(tag.unit("postgresql"))
+        assert storage["storage-tag"].startswith(tag.storage("pgdata"))
+        assert storage["life"] == "alive"
+        assert storage["status"].status == "attached"
+
+
 @base.bootstrapped
 @pytest.mark.bundle
 async def test_deploy_local_bundle_dir():

tests/unit/test_bundle.py

@@ -3,14 +3,13 @@
 
 import unittest
 from pathlib import Path
-from typing import Dict, List, Tuple
 from unittest import mock
 from unittest.mock import ANY, Mock, patch
 
 import yaml
 from toposort import CircularDependencyError
 
-from juju import charmhub, constraints
+from juju import charmhub
 from juju.bundle import (
     AddApplicationChange,
     AddCharmChange,
@@ -189,113 +188,13 @@ async def test_run_with_charmhub_charm(self):
             constraints="constraints",
             endpoint_bindings="endpoint_bindings",
             resources=["resource1"],
-            storage={
-                storage_label: constraints.parse_storage_constraint(storage_constraint)
-            },
+            storage={storage_label: storage_constraint},
             devices="devices",
             channel="channel",
             charm_origin=ANY,
             num_units="num_units",
         )
 
-    async def test_run_with_storage_variations(self):
-        """Test that various valid storage constraints are parsed as expected
-        before model._deploy is called.
-
-        Uses the mock call logic from test_run_with_charmhub_charm,
-        which will run before this test.
-        """
-        storage_arg_pairs: List[
-            Tuple[Dict[str, str], Dict[str, constraints.StorageConstraintDict]]
-        ] = [
-            # (storage_arg_for_change, storage_arg_for_deploy)
-            (
-                {"some-label": "ebs,100G,1"},
-                {"some-label": {"count": 1, "pool": "ebs", "size": 102400}},
-            ),
-            (
-                {"some-label": "ebs,2.1G,3"},
-                {"some-label": {"count": 3, "pool": "ebs", "size": 2150}},
-            ),
-            (
-                {"some-label": "ebs,100G"},
-                {"some-label": {"count": 1, "pool": "ebs", "size": 102400}},
-            ),
-            ({"some-label": "ebs,2"}, {"some-label": {"count": 2, "pool": "ebs"}}),
-            ({"some-label": "200G,7"}, {"some-label": {"count": 7, "size": 204800}}),
-            ({"some-label": "ebs"}, {"some-label": {"count": 1, "pool": "ebs"}}),
-            (
-                {"some-label": "10YB"},
-                {"some-label": {"count": 1, "size": 11529215046068469760}},
-            ),
-            ({"some-label": "1"}, {"some-label": {"count": 1}}),
-            ({"some-label": "-1"}, {"some-label": {"count": 1}}),
-            ({"some-label": ""}, {"some-label": {"count": 1}}),
-            (
-                {
-                    "some-label": "2.1G,3",
-                    "data": "1MiB,70",
-                    "logs": "ebs,-1",
-                },
-                {
-                    "some-label": {"count": 3, "size": 2150},
-                    "data": {"count": 70, "size": 1},
-                    "logs": {"count": 1, "pool": "ebs"},
-                },
-            ),
-        ]
-        for storage_arg_for_change, storage_arg_for_deploy in storage_arg_pairs:
-            change = AddApplicationChange(
-                1,
-                [],
-                params={
-                    "charm": "charm",
-                    "series": "series",
-                    "application": "application",
-                    "options": "options",
-                    "constraints": "constraints",
-                    "storage": storage_arg_for_change,
-                    "endpoint-bindings": "endpoint_bindings",
-                    "resources": "resources",
-                    "devices": "devices",
-                    "num-units": "num_units",
-                    "channel": "channel",
-                },
-            )
-            # mock model
-            model = Mock()
-            model._deploy = mock.AsyncMock(return_value=None)
-            model._add_charmhub_resources = mock.AsyncMock(return_value=["resource1"])
-            model.applications = {}
-            # mock context
-            context = Mock()
-            context.resolve.return_value = "ch:charm1"
-            context.origins = {"ch:charm1": Mock()}
-            context.trusted = False
-            context.model = model
-            # mock info_func
-            info_func = mock.AsyncMock(return_value=["12345", "name"])
-            # patch and call
-            with patch.object(charmhub.CharmHub, "get_charm_id", info_func):
-                result = await change.run(context)
-            assert result == "application"
-            # asserts
-            model._deploy.assert_called_once()
-            model._deploy.assert_called_with(
-                charm_url="ch:charm1",
-                application="application",
-                series="series",
-                config="options",
-                constraints="constraints",
-                endpoint_bindings="endpoint_bindings",
-                resources=["resource1"],
-                storage=storage_arg_for_deploy,  # we're testing this
-                devices="devices",
-                channel="channel",
-                charm_origin=ANY,
-                num_units="num_units",
-            )
-
     async def test_run_with_charmhub_charm_no_channel(self):
         """Test to verify if when the given channel is None, the channel
         defaults to "local/stable", which is the default channel value for the
@@ -347,9 +246,7 @@ async def test_run_with_charmhub_charm_no_channel(self):
             constraints="constraints",
             endpoint_bindings="endpoint_bindings",
             resources=["resource1"],
-            storage={
-                storage_label: constraints.parse_storage_constraint(storage_constraint)
-            },
+            storage={storage_label: storage_constraint},
             devices="devices",
             channel="latest/stable",
             charm_origin=ANY,
@@ -397,9 +294,7 @@ async def test_run_local(self):
             constraints="constraints",
             endpoint_bindings="endpoint_bindings",
             resources={},
-            storage={
-                storage_label: constraints.parse_storage_constraint(storage_constraint)
-            },
+            storage={storage_label: storage_constraint},
             devices="devices",
             num_units="num_units",
             channel="",
@@ -452,9 +347,7 @@ async def test_run_no_series(self):
             constraints="constraints",
             endpoint_bindings="endpoint_bindings",
             resources=["resource1"],
-            storage={
-                storage_label: constraints.parse_storage_constraint(storage_constraint)
-            },
+            storage={storage_label: storage_constraint},
             devices="devices",
             channel="channel",
             charm_origin=ANY,

tests/unit/test_constraints.py

@@ -4,6 +4,7 @@
 #
 # Test our constraints parser
 #
+from __future__ import annotations
 
 import unittest
 
@@ -79,6 +80,57 @@ def test_parse_storage_constraint(self):
         self.assertEqual(_("3,0.5T"), {"count": 3, "size": 512 * 1024**1})
         self.assertEqual(_("0.5T,3"), {"count": 3, "size": 512 * 1024**1})
 
+    def test_parse_storage_constraints(self):
+        """Test that various valid storage constraints are parsed as expected."""
+        storage_arg_pairs: list[
+            tuple[dict[str, str], dict[str, constraints.StorageConstraintDict]]
+        ] = [
+            # (storage_arg, parsed_storage_arg)
+            (
+                {"some-label": "ebs,100G,1"},
+                {"some-label": {"count": 1, "pool": "ebs", "size": 102400}},
+            ),
+            (
+                {"some-label": "ebs,2.1G,3"},
+                {"some-label": {"count": 3, "pool": "ebs", "size": 2150}},
+            ),
+            (
+                {"some-label": "ebs,100G"},
+                {"some-label": {"count": 1, "pool": "ebs", "size": 102400}},
+            ),
+            ({"some-label": "ebs,2"}, {"some-label": {"count": 2, "pool": "ebs"}}),
+            ({"some-label": "200G,7"}, {"some-label": {"count": 7, "size": 204800}}),
+            ({"some-label": "ebs"}, {"some-label": {"count": 1, "pool": "ebs"}}),
+            (
+                {"some-label": "10YB"},
+                {"some-label": {"count": 1, "size": 11529215046068469760}},
+            ),
+            ({"some-label": "1"}, {"some-label": {"count": 1}}),
+            ({"some-label": "-1"}, {"some-label": {"count": 1}}),
+            ({"some-label": ""}, {"some-label": {"count": 1}}),
+            (
+                {
+                    "some-label": "2.1G,3",
+                    "data": "1MiB,70",
+                    "logs": "ebs,-1",
+                },
+                {
+                    "some-label": {"count": 3, "size": 2150},
+                    "data": {"count": 70, "size": 1},
+                    "logs": {"count": 1, "pool": "ebs"},
+                },
+            ),
+        ]
+        for storage_arg, parsed_storage_constraint in storage_arg_pairs:
+            self.assertEqual(
+                constraints.parse_storage_constraints(storage_arg),
+                parsed_storage_constraint,
+            )
+            self.assertEqual(
+                constraints.parse_storage_constraints(parsed_storage_constraint),
+                parsed_storage_constraint,
+            )
+
     def test_parse_device_constraint(self):
         _ = constraints.parse_device_constraint