Update src/olympia/blocklist/tests/test_mlbf.py

Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Update src/olympia/blocklist/tests/test_mlbf.py Co-authored-by: William Durand <will+git@drnd.me> Apply suggestions from code review Co-authored-by: William Durand <will+git@drnd.me>
mozilla · Nov 12, 2024 · b0179cc · b0179cc
1 parent 7aa74ae
commit b0179cc
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 19 deletions.
diff --git a/src/olympia/blocklist/mlbf.py b/src/olympia/blocklist/mlbf.py
@@ -251,7 +251,8 @@ def generate_and_write_stash(self, previous_mlbf: 'MLBF' = None):
         changes between the previous and current MLBF Filters.
 
         In order to support FX clients that don't support soft blocking,
-        unblocked is a union of deletions from blocked and additions to soft_blocked.
+        unblocked is a union of deletions from blocked and deletions from soft_blocked
+        filtering out any items that are in the newly blocked list.
 
         Versions that move from hard to soft blocked will be picked up by old clients
         as no longer hard blocked by being in the unblocked list.

diff --git a/src/olympia/blocklist/tests/test_mlbf.py b/src/olympia/blocklist/tests/test_mlbf.py
@@ -467,48 +467,52 @@ def test_diff_all_possible_changes(self):
         6. Blocked -> Not blocked
 
         """
-        # 1. (Not blocked) -> Soft blocked
+        # 1. Create a version that isn't blocked yet.
         one = addon_factory(guid='1', file_kw={'is_signed': True})
         (one_hash,) = MLBF.hash_filter_inputs([(one.guid, one.current_version.version)])
-        # 2. (Not blocked) -> Blocked
+        # 2. Create a second version not blocked yet.
         two = addon_factory(guid='2', file_kw={'is_signed': True})
         (two_hash,) = MLBF.hash_filter_inputs([(two.guid, two.current_version.version)])
-        # 3. (Soft blocked) -> (Blocked)
+        # Create a soft blocked version.
         three, three_block = self._blocked_addon(
             guid='3', block_type=BlockType.SOFT_BLOCKED, file_kw={'is_signed': True}
         )
         (three_hash,) = MLBF.hash_filter_inputs(
             [(three.guid, three.current_version.version)]
         )
-        # 4. (Soft blocked) -> (Not blocked)
+        # Create another soft blocked version.
         four, four_block = self._blocked_addon(
             guid='4', block_type=BlockType.SOFT_BLOCKED, file_kw={'is_signed': True}
         )
         (four_hash,) = MLBF.hash_filter_inputs(
             [(four.guid, four.current_version.version)]
         )
-        # 5. (Blocked) -> (Soft blocked)
+        # Create a hard blocked version.
         five, five_block = self._blocked_addon(
             guid='5', block_type=BlockType.BLOCKED, file_kw={'is_signed': True}
         )
         (five_hash,) = MLBF.hash_filter_inputs(
             [(five.guid, five.current_version.version)]
         )
-        # 6. (Blocked) -> (Not blocked)
+        # And finally, create another hard blocked version.
         six, six_block = self._blocked_addon(
             guid='6', block_type=BlockType.BLOCKED, file_kw={'is_signed': True}
         )
         (six_hash,) = MLBF.hash_filter_inputs([(six.guid, six.current_version.version)])
 
+        # At this point, we have 2 versions not blocked,
+        # and 4 versions blocked. We're going
+        # to generate a first MLBF from that set of versions.
         first_mlbf = MLBF.generate_from_db('first')
 
+        # We expect the 4 blocked versions to be in the diff, sorted by block type.
         assert first_mlbf.generate_diffs() == {
             BlockType.BLOCKED: ([five_hash, six_hash], [], 2),
             BlockType.SOFT_BLOCKED: ([three_hash, four_hash], [], 2),
         }
 
-        # The first time we generate the stash, we expect 3-6 to be in the stash
-        # as they have some kind of block applied
+        # The first time we generate the stash, we expect 3 to 6 to be in the stash
+        # as they have some kind of block applied.
         assert first_mlbf.generate_and_write_stash() == {
             'blocked': [five_hash, six_hash],
             'unblocked': [],
@@ -521,35 +525,38 @@ def test_diff_all_possible_changes(self):
                 'unblocked': [],
             }
 
-        # Transform the addons to the next state
+        # Update the existing blocks, and create new ones for
+        # the versions "one" and "two".
 
-        # 1. Not blocked -> (Soft blocked)
+        # The first version gets soft blocked now.
         block_factory(
             guid=one.guid, updated_by=self.user, block_type=BlockType.SOFT_BLOCKED
         )
-        # 2. Not blocked -> (Blocked)
+        # The second version is hard blocked.
         block_factory(guid=two.guid, updated_by=self.user, block_type=BlockType.BLOCKED)
-        # 3. Soft blocked -> (Blocked)
+        # 3 was soft-blocked and is now hard blocked.
         three_block.blockversion_set.first().update(block_type=BlockType.BLOCKED)
-        # 4. Soft blocked -> (Not blocked)
+        # 4 was soft blocked and is now unblocked.
         four_block.delete()
-        # 5. Blocked -> (Soft blocked)
+        # 5 was hard blocked and is now soft blocked.
         five_block.blockversion_set.first().update(block_type=BlockType.SOFT_BLOCKED)
-        # 6. Blocked -> (Not blocked)
+        # 6 was hard blocked and is now unblocked.
         six_block.delete()
 
+        # We regenerate another MLBF based on the updates we've just done
+        # to verify the final state of each version.
         second_mlbf = MLBF.generate_from_db('second')
 
         # The order is based on the ID (i.e. creation time) of the block,
-        # not the version so we expect two after three
-        # since two was blocked after three
+        # not the version so we expect two after three since two was
+        # blocked after three.
         assert second_mlbf.generate_diffs(previous_mlbf=first_mlbf) == {
             BlockType.BLOCKED: (
                 [three_hash, two_hash],
                 [five_hash, six_hash],
                 4,
             ),
-            # Same as above, one had a block created after five so it comes second
+            # Same as above, one had a block created after five so it comes second.
             BlockType.SOFT_BLOCKED: (
                 [five_hash, one_hash],
                 [three_hash, four_hash],