Merge lp:~sil2100/ubuntu-release-upgrader/snap-size-estimation into lp:ubuntu-release-upgrader

[Łukasz Zemczak]

Modify DistUpgradeQuirks to calculate extra space needed for snaps that need to be installed during upgrade (replacing old debs) and add that to estimates done in checkFreeSpace().

Modify DistUpgradeQuirks to calculate extra space needed for snaps that need to be installed during upgrade (replacing old debs) and add that to estimates done in checkFreeSpace().

This branch is now ready for an initial review. I had a few design bikeshedding issues while working on this, so I'm still not entirely happy with the end result. For instance, I don't like how it looks with the consecutive quirks assuming the previous ones set up everything for them - but that seemed to be optimal.

I added all the estimation as a quirk, alongside of the existing ones for replacing debs with snaps. Seemed to be 'consistent' with the current approach. In cases where quirks are not ran, the extra_snap_space is 0 so nothing funny is happening. I also added some very basic unit testing for the new methods added. They're not very pretty, mostly because a lot of the functionality had to be mocked as it required network access, which we don't want to rely on in unit testing.
For fetching the size, I try to be as accurate as possible - which is why I am performing raw API calls to the snapstore as per their recommendation. I could not use snap_info as we need to get the size of the snap for the specific branch (stable/ubuntu-x.x) - and only a fake snap_refresh allows that (which needs the snap-id specified). At first I wanted to be consistent and just use `snap info`, but as said - the command didn't seem to allow specifying the branch that needed to be checked.

I'm open to any propositions. This is my first contribution to u-r-u so apologies if the coding style is not adequate. I still need to test it in practice, which is why I'd prefer if we didn't merge it yet before more testing.

[Łukasz Zemczak (sil2100)]

From the stylistic nitpicks for instance, I really had a strong urge to just call _prepare_snap_replacement_data() in both _calculateSnapSizeRequirements() and _replaceDebsWithSnaps(), and simply make it to exit early if self._snap_list is already non-empty. But then it would be non-optimal, since we know _replaceDebsWithSnaps will never be called without _calculateSnapSizeRequirements running first. But eh, I somehow couldn't make this look pretty, sorry...

[Brian Murray (brian-murray)]

Looking at DistUpgradeQuirks.py's run function we can see that there are currently 6 different quirk handlers that are supported. Is there a reason you didn't use an existing one and created "PreCalcDistUpgrade"? If there is a good reason then I think PreCalcDistUpgrade should be documented with the other ones.

review: Needs Information

[Brian Murray (brian-murray)]

I think it'd also be good if DistUpgradeCache.py were to log that the not enough free space error in /var was due to the fact that extra_snap_space was size X.

[Brian Murray (brian-murray)]

Thanks for working on this! You'll find some specific comments in line.

review: Needs Fixing

[lp:~sil2100/ubuntu-release-upgrader/snap-size-estimation]

3280. By Łukasz Zemczak on 2019-08-28

Fixes as per Brian's review: do not create a PreCalcDistUpgrade quirk (no needed), fix typos.

[Łukasz Zemczak (sil2100)]

Thanks for the review! I will address all the issues you have outlined, with comments to a few of them (not including those inline as the diff will change and it's always a bit more annoying to switch to the old revision to see the comments):

1) Regarding PreCalcDistUpgrade: actually I see that I can just do the same in PostInitialUpdate, not sure why those weeks ago I didn't find that one appropriate. I just know any of the later ones would not do, as it has to happen early in the upgrade process. One plus side of having a PreCalcDistUpgrade was that it all felt a bit more consistent, as the size calculation was done right before the dist-upgrade was 'calculated'. I changed it to be done as part of PostInitialUpdate now which is also fine, but the only downside is that now very early users might be seeing 'Calculating snap size requirements', which might seem a bit out-of-place. On the other hand, that doesn't actually matter much I'd say!

2) Regarding changing the 'installing ...' strings as you requested - won't that break translations? I mean, I'm usually very reluctant in changing translatable strings without a valid reason. Guess this particular change won't be backported to bionic though, so maybe it's actually safe. Anyway, I guess I'd recommend changing those in a separate MP/commit.

3) Regarding Snap-Device-Series: for now this is a constant that I think can just stay hard-coded. One might think it's related to the core base (16, 18 etc.) - that would be logical, right? But actually it's not. I was confused about that as well, but the snapd team told me this series is actually unrelated to the snap base but is instead like a snap-specific standard version which should be only bumped when the standard changes, which for now is still constant. So this argument is '16' for both core16, core18 and possibly the upcoming core20.

4) Regarding the count of 5 in the test for replacing debs with snaps: I guess it feels correct, because basically currently as well as previously when determining which debs need to be exchanged with snaps, at the stage of when the quirk is running there was never any logic to check if we're replacing a real deb or not. So for instance 'gtk-common-themes', which is not a real deb package, in the case when its snap was not installed on the system, would anyway mark it for installation and put it into forced_obsoletes. The logic here didn't change. What the unit test checks is only if the 'refresh' snaps are refreshed and if the 'install' snaps are installed (and, per the existing logic, added to the forced_obsoletes).
I can only assume that the upgrader actually checks if the packages in forced_obsoletes are actual packages that need acting on somewhere later on.

5) Regarding the new dependencies and adding those to debian/control: I think no new dependencies need to be added. Both the json and urllib modules are part of the python standard library, so they basically come from the libpython3.*-stdlib etc. packages, and those are explicit dependencies of python3.*. So I guess we should be safe here, right?

[Łukasz Zemczak (sil2100)]

Was also looking into making DistUpgradeCache mentioning the not enough space being because of extra_snap_space, but from what I see the way it's written right now feels very generic and not sure if I should change that. But I'll look into it a bit more.

[Brian Murray (brian-murray)]

> Thanks for the review! I will address all the issues you have outlined, with
> comments to a few of them (not including those inline as the diff will change
> and it's always a bit more annoying to switch to the old revision to see the
> comments):
>
> 1) Regarding PreCalcDistUpgrade: actually I see that I can just do the same in
> PostInitialUpdate, not sure why those weeks ago I didn't find that one
> appropriate. I just know any of the later ones would not do, as it has to
> happen early in the upgrade process. One plus side of having a
> PreCalcDistUpgrade was that it all felt a bit more consistent, as the size
> calculation was done right before the dist-upgrade was 'calculated'. I changed
> it to be done as part of PostInitialUpdate now which is also fine, but the
> only downside is that now very early users might be seeing 'Calculating snap
> size requirements', which might seem a bit out-of-place. On the other hand,
> that doesn't actually matter much I'd say!

Great, thanks.

> 2) Regarding changing the 'installing ...' strings as you requested - won't
> that break translations? I mean, I'm usually very reluctant in changing
> translatable strings without a valid reason. Guess this particular change
> won't be backported to bionic though, so maybe it's actually safe. Anyway, I
> guess I'd recommend changing those in a separate MP/commit.

That's fair and not a big deal really.

> 3) Regarding Snap-Device-Series: for now this is a constant that I think can
> just stay hard-coded. One might think it's related to the core base (16, 18
> etc.) - that would be logical, right? But actually it's not. I was confused
> about that as well, but the snapd team told me this series is actually
> unrelated to the snap base but is instead like a snap-specific standard
> version which should be only bumped when the standard changes, which for now
> is still constant. So this argument is '16' for both core16, core18 and
> possibly the upcoming core20.

Alright, thanks for explaining this!

> 4) Regarding the count of 5 in the test for replacing debs with snaps: I guess
> it feels correct, because basically currently as well as previously when
> determining which debs need to be exchanged with snaps, at the stage of when
> the quirk is running there was never any logic to check if we're replacing a
> real deb or not. So for instance 'gtk-common-themes', which is not a real deb
> package, in the case when its snap was not installed on the system, would
> anyway mark it for installation and put it into forced_obsoletes. The logic
> here didn't change. What the unit test checks is only if the 'refresh' snaps
> are refreshed and if the 'install' snaps are installed (and, per the existing
> logic, added to the forced_obsoletes).
> I can only assume that the upgrader actually checks if the packages in
> forced_obsoletes are actual packages that need acting on somewhere later on.

That makes sense, maybe its worth adding a comment about why it is 5 though?

> 5) Regarding the new dependencies and adding those to debian/control: I think
> no new dependencies need to be added. Both the json and urllib modules are
> part of the python standard library, so the...

[Brian Murray (brian-murray)]

This look good to me now, but as I mentioned on irc I'd expect the size calculation to fail for the core18 snap because there is no stable/ubuntu-19.10 channel for core18. It'd be good to test that this is really failing before fixing bug 1841225.

[lp:~sil2100/ubuntu-release-upgrader/snap-size-estimation]

3281. By Łukasz Zemczak on 2019-08-30

Add a better comment regarding the forced_obsoletes check in quirks.

[Łukasz Zemczak (sil2100)]

I did some testing on a VM and real upgrade scenarios and with this branch and the core18-stable follow up one, everything seems to work as expected. Let me merge both in this case.

=== modified file 'DistUpgrade/DistUpgradeCache.py'
--- DistUpgrade/DistUpgradeCache.py	2019-06-21 17:46:27 +0000
+++ DistUpgrade/DistUpgradeCache.py	2019-08-30 14:54:30 +0000
@@ -1157,6 +1157,8 @@
         # sum up space requirements
         for (dir, size) in [(archivedir, self.required_download),
                             ("/usr", self.additional_required_space),
+                            # this is only >0 for the deb-to-snap quirks
+                            ("/var", self.quirks.extra_snap_space),
                             # plus 50M safety buffer in /usr
                             ("/usr", 50*1024*1024),
                             ("/boot", space_in_boot),

=== modified file 'DistUpgrade/DistUpgradeQuirks.py'
--- DistUpgrade/DistUpgradeQuirks.py	2019-06-24 18:16:09 +0000
+++ DistUpgrade/DistUpgradeQuirks.py	2019-08-30 14:54:30 +0000
@@ -49,8 +49,12 @@
         self.uname = Popen(["uname", "-r"], stdout=PIPE,
                            universal_newlines=True).communicate()[0].strip()
         self.arch = get_arch()
+        self.extra_snap_space = 0
         self._poke = None
         self._snapstore_reachable = False
+        self._snap_list = None
+        self._from_version = None
+        self._to_version = None
 
     # the quirk function have the name:
     #  $Name (e.g. PostUpgrade)
@@ -118,6 +122,11 @@
         if cache['ubuntu-desktop'].is_installed and \
                 cache['snapd'].is_installed:
             self._checkStoreConnectivity()
+        # If the snap store is accessible, at the same time calculate the
+        # extra size needed by to-be-installed snaps.  This also prepares
+        # the snaps-to-install list for the actual upgrade.
+        if self._snapstore_reachable:
+            self._calculateSnapSizeRequirements()
 
     def eoanPostUpgrade(self):
         logging.debug("running Quirks.eoanPostUpgrade")
@@ -128,7 +137,7 @@
             return
         if cache['ubuntu-desktop'].is_installed and \
                 cache['snapd'].is_installed and \
-                self._snapstore_reachable:
+                self._snap_list:
             self._replaceDebsWithSnaps()
 
     # individual quirks handler when the dpkg run is finished ---------
@@ -439,51 +448,65 @@
         if not res:
             self.controller.abort()
 
+    def _calculateSnapSizeRequirements(self):
+        import json
+        import urllib.request
+        from urllib.error import URLError
+
+        # first fetch the list of snap-deb replacements that will be needed
+        # and store them for future reference, along with other data we'll
+        # need in the process
+        self._prepare_snap_replacement_data()
+        # now perform direct API calls to the store, requesting size
+        # information for each of the snaps needing installation
+        self._view.updateStatus(_("Calculating snap size requirements"))
+        for snap, snap_object in self._snap_list.items():
+            if snap_object['command'] != 'install':
+                continue
+            action = {
+                "instance-key": "upgrade-size-check",
+                "action": "download",
+                "snap-id": snap_object['snap-id'],
+                "channel": "stable/ubuntu-%s" % self._to_version,
+            }
+            data = {
+                "context": [],
+                "actions": [action],
+            }
+            req = urllib.request.Request(
+                url='https://api.snapcraft.io/v2/snaps/refresh',
+                data=bytes(json.dumps(data), encoding='utf-8'))
+            req.add_header('Snap-Device-Series', '16')
+            req.add_header('Content-type', 'application/json')
+            req.add_header('Snap-Device-Architecture', self.arch)
+            try:
+                response = urllib.request.urlopen(req).read()
+                info = json.loads(response)
+                size = int(info['results'][0]['snap']['download']['size'])
+            except (KeyError, URLError, ValueError):
+                logging.debug("Failed fetching size of snap %s" % snap)
+                continue
+            self.extra_snap_space += size
+
     def _replaceDebsWithSnaps(self):
-        di = distro_info.UbuntuDistroInfo()
-        try:
-            fromVersion = \
-                di.version('%s' % self.controller.fromDist).split()[0]
-            toVersion = di.version('%s' % self.controller.toDist).split()[0]
-        # Ubuntu 18.04's python3-distro-info does not have version
-        except AttributeError:
-            fromVersion = next((r.version for r in di.get_all("object")
-                               if r.series == self.controller.fromDist),
-                               self.controller.fromDist).split()[0]
-            toVersion = next((r.version for r in di.get_all("object")
-                             if r.series == self.controller.toDist),
-                             self.controller.toDist).split()[0]
         """ install a snap and mark its corresponding package for removal """
         # gtk-common-themes isn't a package name but is this risky?
-        snaps = ['core18', 'gnome-3-28-1804', 'gtk-common-themes',
-                 'gnome-calculator', 'gnome-characters', 'gnome-logs',
-                 'gnome-system-monitor']
-        self._view.updateStatus(_("Checking for installed snaps"))
-        for snap in snaps:
-            # check to see if the snap is already installed
-            snap_info = subprocess.Popen(["snap", "info", snap],
-                                         universal_newlines=True,
-                                         stdout=subprocess.PIPE).communicate()
-            self._view.processEvents()
-            if re.search("^installed: ", snap_info[0], re.MULTILINE):
-                logging.debug("Snap %s is installed" % snap)
-                # its not tracking the release channel so don't refresh
-                if not re.search("^tracking:.*ubuntu-%s" % fromVersion,
-                                 snap_info[0], re.MULTILINE):
-                    logging.debug("Snap %s is not tracking the release channel"
-                                  % snap)
-                    continue
-                command = 'refresh'
+        self._view.updateStatus(_("Processing snap replacements"))
+        # _snap_list should be populated by the earlier
+        # _calculateSnapSizeRequirements call.
+        for snap, snap_object in self._snap_list.items():
+            command = snap_object['command']
+            if command == 'refresh':
                 self._view.updateStatus(_("refreshing snap %s" % snap))
             else:
-                command = 'install'
                 self._view.updateStatus(_("installing snap %s" % snap))
             try:
                 self._view.processEvents()
-                proc = subprocess.run(["snap", command, "--channel",
-                                       "stable/ubuntu-%s" % toVersion, snap],
-                                      stdout=subprocess.PIPE,
-                                      check=True)
+                proc = subprocess.run(
+                    ["snap", command, "--channel",
+                     "stable/ubuntu-%s" % self._to_version, snap],
+                    stdout=subprocess.PIPE,
+                    check=True)
                 self._view.processEvents()
             except subprocess.CalledProcessError:
                 logging.debug("%s of snap %s failed" % (command, snap))
@@ -767,3 +790,57 @@
                 msg += " enabling it just for the upgrade"
                 logging.warning(msg)
                 apt.apt_pkg.config.set("Apt::Install-Recommends", "1")
+
+    def _prepare_snap_replacement_data(self):
+        """ Helper function fetching all required info for the deb-to-snap
+            migration: version strings for upgrade (from and to) and the list
+            of snaps (with actions).
+        """
+        di = distro_info.UbuntuDistroInfo()
+        try:
+            self._from_version = \
+                di.version('%s' % self.controller.fromDist).split()[0]
+            self._to_version = \
+                di.version('%s' % self.controller.toDist).split()[0]
+        # Ubuntu 18.04's python3-distro-info does not have version
+        except AttributeError:
+            self._from_version = next(
+                (r.version for r in di.get_all("object")
+                 if r.series == self.controller.fromDist),
+                self.controller.fromDist).split()[0]
+            self._to_version = next(
+                (r.version for r in di.get_all("object")
+                 if r.series == self.controller.toDist),
+                self.controller.toDist).split()[0]
+        self._snap_list = {}
+        # gtk-common-themes isn't a package name but is this risky?
+        snaps = ['core18', 'gnome-3-28-1804', 'gtk-common-themes',
+                 'gnome-calculator', 'gnome-characters', 'gnome-logs',
+                 'gnome-system-monitor']
+        self._view.updateStatus(_("Checking for installed snaps"))
+        for snap in snaps:
+            snap_object = {}
+            # check to see if the snap is already installed
+            snap_info = subprocess.Popen(["snap", "info", snap],
+                                         universal_newlines=True,
+                                         stdout=subprocess.PIPE).communicate()
+            self._view.processEvents()
+            if re.search("^installed: ", snap_info[0], re.MULTILINE):
+                logging.debug("Snap %s is installed" % snap)
+                # its not tracking the release channel so don't refresh
+                if not re.search("^tracking:.*ubuntu-%s" % self._from_version,
+                                 snap_info[0], re.MULTILINE):
+                    logging.debug("Snap %s is not tracking the release channel"
+                                  % snap)
+                    continue
+                snap_object['command'] = 'refresh'
+            else:
+                match = re.search("snap-id:\s*(\w*)", snap_info[0])
+                if not match:
+                    logging.debug("Could not parse snap-id for the %s snap"
+                                  % snap)
+                    continue
+                snap_object['command'] = 'install'
+                snap_object['snap-id'] = match[1]
+            self._snap_list[snap] = snap_object
+        return self._snap_list

=== modified file 'tests/test_quirks.py'
--- tests/test_quirks.py	2018-08-31 22:46:45 +0000
+++ tests/test_quirks.py	2019-08-30 14:54:30 +0000
@@ -9,6 +9,7 @@
 import unittest
 import shutil
 import tempfile
+import json
 
 from DistUpgrade.DistUpgradeQuirks import DistUpgradeQuirks
 
@@ -24,6 +25,104 @@
     pass
 
 
+class MockPopenSnap():
+    def __init__(self, cmd, universal_newlines=True,
+                 stdout=None):
+        self.command = cmd
+
+    def communicate(self):
+        snap_name = self.command[2]
+        if (snap_name == 'gnome-logs' or
+                snap_name == 'gnome-system-monitor'):
+            return ["""
+name:      test-snap
+summary:   Test Snap
+publisher: Canonical
+license:   unset
+description: Some description
+commands:
+  - gnome-calculator
+snap-id:      1234
+tracking:     stable/ubuntu-19.04
+refresh-date: 2019-04-11
+channels:
+  stable:    3.32.1  2019-04-10 (406) 4MB -
+  candidate: 3.32.2  2019-06-26 (433) 4MB -
+  beta:      3.33.89 2019-08-06 (459) 4MB -
+  edge:      3.33.90 2019-08-06 (460) 4MB -
+installed:   3.32.1             (406) 4MB -
+"""]
+        else:
+            return ["""
+name:      test-snap
+summary:   Test Snap
+publisher: Canonical
+license:   unset
+description: Some description
+commands:
+  - gnome-calculator
+snap-id:      1234
+refresh-date: 2019-04-11
+channels:
+  stable:    3.32.1  2019-04-10 (406) 4MB -
+  candidate: 3.32.2  2019-06-26 (433) 4MB -
+  beta:      3.33.89 2019-08-06 (459) 4MB -
+  edge:      3.33.90 2019-08-06 (460) 4MB -
+"""]
+
+
+def mock_urlopen_snap(req):
+    result = """{{
+  "error-list": [],
+  "results": [
+    {{
+      "effective-channel": "stable",
+      "instance-key": "test",
+      "name": "{name}",
+      "released-at": "2019-04-10T18:54:15.717357+00:00",
+      "result": "download",
+      "snap": {{
+        "created-at": "2019-04-09T17:09:29.941588+00:00",
+        "download": {{
+          "deltas": [],
+          "size": {size},
+          "url": "SNAPURL"
+        }},
+        "license": "GPL-3.0+",
+        "name": "{name}",
+        "prices": {{ }},
+        "publisher": {{
+          "display-name": "Canonical",
+          "id": "canonical",
+          "username": "canonical",
+          "validation": "verified"
+        }},
+        "revision": 406,
+        "snap-id": "{snap_id}",
+        "summary": "GNOME Calculator",
+        "title": "GNOME Calculator",
+        "type": "app",
+        "version": "3.32.1"
+      }},
+      "snap-id": "{snap_id}"
+    }}
+  ]
+}}
+"""
+    test_snaps = {
+        '1': ("gnome-calculator", 4218880),
+        '2': ("test-snap", 2000000)
+    }
+    json_data = json.loads(req.data)
+    snap_id = json_data['actions'][0]['snap-id']
+    name = test_snaps[snap_id][0]
+    size = test_snaps[snap_id][1]
+    response_mock = mock.Mock()
+    response_mock.read.return_value = result.format(
+        name=name, snap_id=snap_id, size=size)
+    return response_mock
+
+
 def make_mock_pkg(name, is_installed, candidate_rec):
     mock_pkg = mock.Mock()
     mock_pkg.name = name
@@ -229,6 +328,111 @@
         self.assertEqual(pkgname, "linux-generic-lts-quantal")
 
 
+class TestSnapQuirks(unittest.TestCase):
+
+    @mock.patch("subprocess.Popen", MockPopenSnap)
+    def test_prepare_snap_replacement_data(self):
+        # Prepare the state for testing
+        controller = mock.Mock()
+        controller.fromDist = 'disco'
+        controller.toDist = 'eoan'
+        config = mock.Mock()
+        q = DistUpgradeQuirks(controller, config)
+        # Call method under test
+        q._prepare_snap_replacement_data()
+        self.assertEqual(q._from_version, '19.04')
+        self.assertEqual(q._to_version, '19.10')
+        # Check if the right snaps have been detected as installed and
+        # needing refresh and which ones need installation
+        self.assertDictEqual(
+            q._snap_list,
+            {'core18': {'command': 'install', 'snap-id': '1234'},
+             'gnome-3-28-1804': {'command': 'install', 'snap-id': '1234'},
+             'gtk-common-themes': {'command': 'install', 'snap-id': '1234'},
+             'gnome-calculator': {'command': 'install', 'snap-id': '1234'},
+             'gnome-characters': {'command': 'install', 'snap-id': '1234'},
+             'gnome-logs': {'command': 'refresh'},
+             'gnome-system-monitor': {'command': 'refresh'}})
+
+    @mock.patch("DistUpgrade.DistUpgradeQuirks.get_arch")
+    @mock.patch("urllib.request.urlopen")
+    def test_calculate_snap_size_requirements(self, urlopen, arch):
+        # Prepare the state for testing
+        arch.return_value = 'amd64'
+        controller = mock.Mock()
+        config = mock.Mock()
+        q = DistUpgradeQuirks(controller, config)
+        # We mock out _prepare_snap_replacement_data(), as this is tested
+        # separately.
+        q._prepare_snap_replacement_data = mock.Mock()
+        q._snap_list = {
+            'test-snap': {'command': 'install', 'snap-id': '2'},
+            'gnome-calculator': {'command': 'install', 'snap-id': '1'},
+            'gnome-system-monitor': {'command': 'refresh'}
+        }
+        q._to_version = "19.10"
+        # Mock out urlopen in such a way that we get a mocked response based
+        # on the parameters given but also allow us to check call arguments
+        # etc.
+        urlopen.side_effect = mock_urlopen_snap
+        # Call method under test
+        q._calculateSnapSizeRequirements()
+        # Check if the size was calculated correctly
+        self.assertEqual(q.extra_snap_space, 6218880)
+        # Check if we only sent queries for the two command: install snaps
+        self.assertEqual(urlopen.call_count, 2)
+        # Make sure each call had the right headers and parameters
+        for call in urlopen.call_args_list:
+            req = call[0][0]
+            self.assertIn(b"stable/ubuntu-19.10", req.data)
+            self.assertDictEqual(
+                req.headers,
+                {'Snap-device-series': '16',
+                 'Content-type': 'application/json',
+                 'Snap-device-architecture': 'amd64'})
+
+    @mock.patch("subprocess.run")
+    def test_replace_debs_with_snaps(self, run_mock):
+        controller = mock.Mock()
+        config = mock.Mock()
+        q = DistUpgradeQuirks(controller, config)
+        q._snap_list = {
+            'core18': {'command': 'install', 'snap-id': '1234'},
+            'gnome-3-28-1804': {'command': 'install', 'snap-id': '1234'},
+            'gtk-common-themes': {'command': 'install', 'snap-id': '1234'},
+            'gnome-calculator': {'command': 'install', 'snap-id': '1234'},
+            'gnome-characters': {'command': 'install', 'snap-id': '1234'},
+            'gnome-logs': {'command': 'refresh'},
+            'gnome-system-monitor': {'command': 'refresh'}
+        }
+        q._to_version = "19.10"
+        q._replaceDebsWithSnaps()
+        # Make sure all snaps have been handled
+        self.assertEqual(run_mock.call_count, 7)
+        snaps_refreshed = set()
+        snaps_installed = set()
+        # Check if all the snaps that needed to be installed were installed
+        # and those that needed a refresh - refreshed
+        for call in run_mock.call_args_list:
+            args = call[0][0]
+            if args[1] == 'install':
+                snaps_installed.add(args[4])
+            else:
+                snaps_refreshed.add(args[4])
+        self.assertSetEqual(
+            snaps_refreshed,
+            {'gnome-logs', 'gnome-system-monitor'})
+        self.assertSetEqual(
+            snaps_installed,
+            {'core18', 'gnome-3-28-1804', 'gtk-common-themes',
+             'gnome-calculator', 'gnome-characters'})
+        # Make sure we marked the replaced ones for removal
+        # Here we only check if the right number of 'packages' has been
+        # added to the forced_obsoletes list - not all of those packages are
+        # actual deb packages that will have to be removed during the upgrade
+        self.assertEqual(controller.forced_obsoletes.append.call_count, 5)
+
+
 if __name__ == "__main__":
     import logging
     logging.basicConfig(level=logging.DEBUG)