Repository: canonical/operator (language: python)

Issue:
testing storage is somewhat broken
There is a bug in the testing backend:
![image](https://user-images.githubusercontent.com/6230162/186390695-fab55d65-6592-4140-b83b-d55e8af328e0.png)

`include_detached` is namely not used and the code behaves as if it was always True.
I couldn't find any tests for it, in fact.

Attempting to fix it 'the obvious way', i.e.
```python
        return list(index for index in self._storage_list[name]
                    if (include_detached or self._storage_is_attached(name, index)))
```

Breaks the existing unittests, so there are more issues downstream.


Discussion / hints:
I think the issue is that the harness does not drop a snapshot of the newly-added storage on `attach_storage` calls. When the storage-attached event is emitted, it attempts to load a snapshot that doesn't exist.
![image](https://user-images.githubusercontent.com/6230162/186392117-e46ba3f0-5e59-4507-b56b-c1e8f9bf6eb1.png)

A meta comment here - I'm an old guy who prefers text-based code snippets over screenshots :P

Fix patch:
diff --git a/ops/framework.py b/ops/framework.py
index 5ed446f2b..ddfc4bb19 100755
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -1027,6 +1027,7 @@ class BoundStoredState:
         @property
         def _data(self) -> StoredStateData:  # noqa
             pass  # pyright: reportGeneralTypeIssues=false
+
         @property  # noqa
         def _attr_name(self) -> str:  # noqa
             pass  # pyright: reportGeneralTypeIssues=false


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index a01189f0c..be25b1da0 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -524,10 +524,6 @@ def add_storage(self, storage_name: str, count: int = 1,
 
         storage_indices = self._backend.storage_add(storage_name, count)
 
-        # Reset associated cached value in the storage mappings.  If we don't do this,
-        # Model._storages won't return Storage objects for subsequently-added storage.
-        self._model._storages._invalidate(storage_name)
-
         ids = []
         for storage_index in storage_indices:
             s = model.Storage(storage_name, storage_index, self._backend)
@@ -574,6 +570,10 @@ def attach_storage(self, storage_id: str) -> None:
 
         storage_name, storage_index = storage_id.split('/', 1)
 
+        # Reset associated cached value in the storage mappings.  If we don't do this,
+        # Model._storages won't return Storage objects for subsequently-added storage.
+        self._model._storages._invalidate(storage_name)
+
         storage_index = int(storage_index)
         self.charm.on[storage_name].storage_attached.emit(
             model.Storage(storage_name, storage_index, self._backend))
@@ -1433,7 +1433,7 @@ def storage_list(self, name: str, include_detached: bool = False):
             include_detached: True to include unattached storage mounts as well.
         """
         return list(index for index in self._storage_list[name]
-                    if all or self._storage_is_attached(name, index))
+                    if include_detached or self._storage_is_attached(name, index))
 
     def storage_get(self, storage_name_id, attribute):
         name, index = storage_name_id.split("/", 1)
diff --git a/test/test_testing.py b/test/test_testing.py
index 983a81be4..590992cb8 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -1416,6 +1416,23 @@ def test_add_storage_then_harness_begin(self):
         want = str(pathlib.PurePath('test', '0'))
         self.assertEqual(want, harness._backend.storage_get("test/0", "location")[-6:])
 
+    def test_add_storage_not_attached_default(self):
+        harness = Harness(CharmBase, meta='''
+            name: test-app
+            requires:
+                db:
+                    interface: pgsql
+            storage:
+                test:
+                    type: filesystem
+            ''')
+        self.addCleanup(harness.cleanup)
+
+        harness.add_storage('test')
+        harness.begin()
+        assert len(harness.model.storages['test']) == 0, \
+            'storage should start in detached state and be excluded from storage listing'
+
     def test_add_storage_without_metadata_key_fails(self):
         harness = Harness(CharmBase, meta='''
             name: test-app