test: attempt fixing another edge case

src/bidiMapper/modules/session/SubscriptionManager.spec.ts

@@ -181,14 +181,18 @@ describe('SubscriptionManager', () => {
         ).to.deep.equal([]);
       });
 
-      xit('happy path 2', () => {
-        subscriptionManager.subscribe([SOME_EVENT, ANOTHER_EVENT], [], SOME_CHANNEL);
-        subscriptionManager.subscribe([SOME_EVENT, YET_ANOTHER_EVENT], [], SOME_CHANNEL);
-        subscriptionManager.unsubscribe(
-          [SOME_EVENT],
+      it('happy path 2', () => {
+        subscriptionManager.subscribe(
+          [SOME_EVENT, ANOTHER_EVENT],
+          [],
+          SOME_CHANNEL,
+        );
+        subscriptionManager.subscribe(
+          [SOME_EVENT, YET_ANOTHER_EVENT],
           [],
           SOME_CHANNEL,
         );
+        subscriptionManager.unsubscribe([SOME_EVENT], [], SOME_CHANNEL);
         expect(
           subscriptionManager.getChannelsSubscribedToEvent(
             SOME_EVENT,
@@ -515,27 +519,6 @@ describe('SubscriptionManager', () => {
         ),
       ).to.deep.equal([SOME_CHANNEL]);
     });
-
-    it('should not unsubscribe from top-level context when event lists do not match', () => {
-      subscriptionManager.subscribe(
-        [SOME_EVENT],
-        [SOME_NESTED_CONTEXT],
-        SOME_CHANNEL,
-      );
-      expect(() => {
-        subscriptionManager.unsubscribe(
-          [SOME_EVENT, ANOTHER_EVENT],
-          [SOME_CONTEXT],
-          SOME_CHANNEL,
-        );
-      }).to.throw('No subscription found');
-      expect(
-        subscriptionManager.getChannelsSubscribedToEvent(
-          SOME_EVENT,
-          SOME_CONTEXT,
-        ),
-      ).to.deep.equal([SOME_CHANNEL]);
-    });
   });
 
   describe('cartesian product', () => {

src/bidiMapper/modules/session/SubscriptionManager.ts

@@ -244,56 +244,47 @@ export class SubscriptionManager {
       }),
     );
 
+    const isGlobalUnsubscribe = topLevelTraversables.size === 0;
     const newSubscriptions: Subscription[] = [];
-    let found = false;
+    const eventsMatched = new Set<ChromiumBidi.EventNames>();
+    const contextsMatched = new Set<BrowsingContext.BrowsingContext>();
     for (const subscription of this.#subscriptions) {
-      if (found || subscription.channel !== channel) {
+      if (subscription.channel !== channel) {
         newSubscriptions.push(subscription);
         continue;
       }
+      // Skip subscriptions when none of the event names match.
       if (intersection(subscription.eventNames, eventNames).size === 0) {
         newSubscriptions.push(subscription);
         continue;
       }
-      if (topLevelTraversables.size === 0) {
-        // If it is non-global subscription, keep it as is because we
-        // are only unsubscribing globally.
+      if (isGlobalUnsubscribe) {
+        // Skip non-global subscriptions.
         if (subscription.topLevelTraversableIds.size !== 0) {
           newSubscriptions.push(subscription);
           continue;
         }
         const subscriptionEventNames = new Set(subscription.eventNames);
         for (const eventName of eventNames) {
           if (subscriptionEventNames.has(eventName)) {
-            eventNames.delete(eventName);
+            eventsMatched.add(eventName);
             subscriptionEventNames.delete(eventName);
           }
         }
-        // If we consumed all events from the unsubscribe request, we
-        // can update the subscriptions.
-        if (eventNames.size === 0) {
-          found = true;
-        }
         // If some events remain in the subscription, we keep it.
         if (subscriptionEventNames.size !== 0) {
-          subscription.eventNames = subscriptionEventNames;
-          newSubscriptions.push(subscription);
+          newSubscriptions.push({
+            ...subscription,
+            eventNames: subscriptionEventNames,
+          });
         }
       } else {
         // Skip global subscriptions.
         if (subscription.topLevelTraversableIds.size === 0) {
           newSubscriptions.push(subscription);
           continue;
         }
-        // If event name sets are not exactly the same,
-        // it is not a match for unsubscribe.
-        if (
-          intersection(eventNames, subscription.eventNames).size !==
-          eventNames.size
-        ) {
-          newSubscriptions.push(subscription);
-          continue;
-        }
+
         // Splitting context subscriptions.
         const eventMap = new Map<
           ChromiumBidi.EventNames,
@@ -302,26 +293,22 @@ export class SubscriptionManager {
         for (const eventName of subscription.eventNames) {
           eventMap.set(eventName, new Set(subscription.topLevelTraversableIds));
         }
-        const toRemove = new Set<string>();
         for (const eventName of eventNames) {
           const eventContextSet = eventMap.get(eventName);
           if (!eventContextSet) {
             continue;
           }
           for (const toRemoveId of topLevelTraversables) {
             if (eventContextSet.has(toRemoveId)) {
-              toRemove.add(toRemoveId);
+              contextsMatched.add(toRemoveId);
+              eventsMatched.add(eventName);
               eventContextSet.delete(toRemoveId);
             }
           }
           if (eventContextSet.size === 0) {
             eventMap.delete(eventName);
           }
         }
-        // Consume traversables from the request.
-        for (const toRemoveId of toRemove) {
-          topLevelTraversables.delete(toRemoveId);
-        }
         for (const [eventName, remainingContextIds] of eventMap) {
           const partialSubscription: Subscription = {
             id: subscription.id,
@@ -331,15 +318,19 @@ export class SubscriptionManager {
           };
           newSubscriptions.push(partialSubscription);
         }
-        // If we consumed all traversables, we found a match.
-        if (topLevelTraversables.size === 0) {
-          found = true;
-        }
       }
     }
-    if (!found) {
+
+    // If some events did not match, it is an invalid request.
+    if (!equal(eventsMatched, eventNames)) {
+      throw new InvalidArgumentException('No subscription found');
+    }
+
+    // If some events did not match, it is an invalid request.
+    if (!isGlobalUnsubscribe && !equal(contextsMatched, topLevelTraversables)) {
       throw new InvalidArgumentException('No subscription found');
     }
+
     // Committing the new subscriptions.
     this.#subscriptions = newSubscriptions;
   }
@@ -377,3 +368,15 @@ export function difference<T>(setA: Set<T>, setB: Set<T>): Set<T> {
   }
   return result;
 }
+
+function equal<T>(setA: Set<T>, setB: Set<T>): boolean {
+  if (setA.size !== setB.size) {
+    return false;
+  }
+  for (const a of setA) {
+    if (!setB.has(a)) {
+      return false;
+    }
+  }
+  return true;
+}

tests/browsing_context/test_create.py

@@ -14,9 +14,9 @@
 # limitations under the License.
 import pytest
 from anys import ANY_STR
-from test_helpers import (ANY_TIMESTAMP, AnyExtending, execute_command,
-                          get_tree, goto_url, read_JSON_message,
-                          send_JSON_command, subscribe)
+from test_helpers import (ANY_TIMESTAMP, ANY_UUID, AnyExtending,
+                          execute_command, get_tree, goto_url,
+                          read_JSON_message, send_JSON_command, subscribe)
 
 
 @pytest.mark.asyncio
@@ -419,7 +419,7 @@ async def test_browsingContext_subscribe_to_contextCreated_emits_for_existing(
         'id': subscribe_command_id,
         'type': 'success',
         'result': {
-            'subscription': ANY_STR,
+            'subscription': ANY_UUID,
         }
     }
 

tests/log/test_log_entry_added.py

@@ -17,8 +17,8 @@
 
 import pytest
 from anys import ANY_STR
-from test_helpers import (ANY_TIMESTAMP, read_JSON_message, send_JSON_command,
-                          subscribe, wait_for_event)
+from test_helpers import (ANY_TIMESTAMP, ANY_UUID, read_JSON_message,
+                          send_JSON_command, subscribe, wait_for_event)
 
 
 @pytest.mark.asyncio
@@ -524,6 +524,6 @@ async def test_runtimeException_buffered(websocket, context_id):
         "type": "success",
         "id": subscribe_command_id,
         "result": {
-            "subscription": ANY_STR
+            "subscription": ANY_UUID
         }
     } == resp

tests/session/test_subscription.py

@@ -18,9 +18,9 @@
 
 import pytest
 from anys import ANY_DICT, ANY_STR, AnyWithEntries
-from test_helpers import (ANY_TIMESTAMP, AnyExtending, execute_command,
-                          get_next_command_id, read_JSON_message,
-                          send_JSON_command, subscribe)
+from test_helpers import (ANY_TIMESTAMP, ANY_UUID, AnyExtending,
+                          execute_command, get_next_command_id,
+                          read_JSON_message, send_JSON_command, subscribe)
 
 
 @pytest.mark.asyncio
@@ -50,7 +50,7 @@ async def test_subscribeWithoutContext_subscribesToEventsInAllContexts(
                 "events": ["browsingContext.load"]
             }
         })
-    assert result == {'subscription': ANY_STR}
+    assert result == {'subscription': ANY_UUID}
 
     # Navigate to some page.
     await send_JSON_command(
@@ -80,7 +80,7 @@ async def test_subscribeWithContext_subscribesToEventsInGivenContext(
                 "contexts": [context_id]
             }
         })
-    assert result == {"subscription": ANY_STR}
+    assert result == {"subscription": ANY_UUID}
 
     # Navigate to some page.
     await send_JSON_command(