Refactor comments for clarity and remove spec references in event ID normalization and holdout configuration tests

Author: FarhanAnjum-opti

optimizely/event/event_id_normalizer.py

@@ -22,21 +22,20 @@
     string** (numeric like ``"12345"`` or opaque like ``"default-12345"`` /
     ``"layer_abc"``). The fallback to ``experiment_id`` fires ONLY when the
     value is the empty string, ``None``, or missing. Non-string types are
-    out of scope for this normalization path (per spec assumptions; the
-    upstream datafile producer delivers string or null values).
+    out of scope for this normalization path (the upstream datafile
+    producer delivers string or null values).
   * ``variation_id`` retains the stricter contract: it MUST be a non-empty
     string of decimal digits ``0-9`` (leading zeros allowed). Empty,
     whitespace, non-string, and non-numeric inputs are normalized to
     ``None`` so the wire payload carries an explicit null.
   * ``entity_id`` on impression events shares the campaign_id normalization
     and is therefore byte-equivalent to the normalized campaign_id for the
-    same impression (FR-009).
+    same impression.
 
 The normalization path MUST NOT log, warn, or raise. It must never drop or
 defer event dispatch.
 """
 
-from __future__ import annotations
 
 from sys import version_info
 from typing import Any, Optional
@@ -49,41 +48,33 @@
 
 def is_non_empty_string(value: Any) -> TypeGuard[str]:
     """Return ``True`` if ``value`` is a non-empty :class:`str`.
-
-    Used for ``campaign_id`` and ``entity_id`` validation per the relaxed
-    FR-001 / FR-009 contract: any non-empty string is accepted regardless of
+    Any non-empty string is accepted regardless of
     character content (IDs may be opaque, e.g. ``"default-12345"``).
     """
     return isinstance(value, str) and value != ''
 
 
 def is_numeric_id_string(value: Any) -> TypeGuard[str]:
     """Return ``True`` if ``value`` is a non-empty decimal-digit string.
-
-    Used for ``variation_id`` validation per FR-003 (the only field that
-    retains the strict numeric-string contract). Whitespace, signs, decimal
-    points, exponents, and non-string types all return ``False``. Leading
+    Whitespace, signs, decimal points, exponents
+    and non-string types all return ``False``. Leading
     zeros are accepted.
     """
     if not isinstance(value, str):
         return False
     if value == '':
         return False
-    # ``str.isdigit`` rejects everything except [0-9] characters and the
-    # empty string. We've already excluded the empty case above. Note that
-    # ``isdigit`` also accepts some non-ASCII digit code points; ``isascii``
-    # combined with ``isdigit`` restricts us to plain decimal digits.
     return value.isascii() and value.isdigit()
 
 
 def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str:
-    """Normalize a decision-event ``campaign_id`` (FR-001/FR-002, FR-009).
+    """Normalize a decision-event ``campaign_id``.
 
     Returns ``campaign_id`` unchanged when it is a non-empty string (any
     character content — numeric like ``"12345"`` or opaque like
     ``"default-12345"``). Otherwise falls back to ``experiment_id`` (when it
     is itself a non-empty string). If neither is a non-empty string, returns
-    an empty string so the event still dispatches (FR-006).
+    an empty string so the event still dispatches.
     """
     if is_non_empty_string(campaign_id):
         return campaign_id
@@ -93,7 +84,7 @@ def normalize_campaign_id(campaign_id: Any, experiment_id: Any) -> str:
 
 
 def normalize_variation_id(variation_id: Any) -> Optional[str]:
-    """Normalize a decision-event ``variation_id`` (FR-003/FR-004).
+    """Normalize a decision-event ``variation_id``.
 
     Returns the original value if it is a valid numeric ID string. Otherwise
     returns ``None`` so the event payload carries an explicit null for the

optimizely/project_config.py

@@ -122,7 +122,7 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any):
             self.global_holdouts.append(holdout)
 
         # Process local holdouts: every entry must carry 'includedRules' (list of rule IDs).
-        # Entries without 'includedRules' are invalid per spec — log an error and exclude
+        # Entries without 'includedRules' are invalid — log an error and exclude
         # them from evaluation (do NOT fall back to global application).
         for holdout_data in local_holdouts_data:
             if 'includedRules' not in holdout_data or holdout_data.get('includedRules') is None:

tests/test_event_factory.py

@@ -1244,7 +1244,7 @@ class EventFactoryIdNormalizationIntegrationTest(base.BaseTest):
 
     These tests build real ``ImpressionEvent`` instances using crafted
     Experiment/Variation objects, then call ``EventFactory.create_log_event``
-    and inspect the dispatched payload. They exercise FR-001..FR-009.
+    and inspect the dispatched payload.
     """
 
     def setUp(self, *args, **kwargs):
@@ -1307,15 +1307,13 @@ def _dispatched_decision(self, impression_event):
         snapshot = log_event.params['visitors'][0]['snapshots'][0]
         return snapshot['decisions'][0], snapshot['events'][0]
 
-    # ------------------------------------------------------------------ FR-001
     def test_valid_campaign_id_is_passed_through(self):
         impression = self._build_impression('111127', '111182', '111129')
         decision, event = self._dispatched_decision(impression)
         self.assertEqual('111182', decision['campaign_id'])
-        # FR-009: entity_id mirrors campaign_id byte-for-byte.
+        # entity_id mirrors campaign_id byte-for-byte.
         self.assertEqual(decision['campaign_id'], event['entity_id'])
 
-    # ------------------------------------------------------------------ FR-002
     def test_empty_campaign_id_falls_back_to_experiment_id(self):
         impression = self._build_impression('111127', '', '111129')
         decision, event = self._dispatched_decision(impression)
@@ -1343,13 +1341,11 @@ def test_whitespace_campaign_id_passes_through(self):
         self.assertEqual(' ', decision['campaign_id'])
         self.assertEqual(' ', event['entity_id'])
 
-    # ------------------------------------------------------------------ FR-003
     def test_valid_variation_id_is_passed_through(self):
         impression = self._build_impression('111127', '111182', '111129')
         decision, _ = self._dispatched_decision(impression)
         self.assertEqual('111129', decision['variation_id'])
 
-    # ------------------------------------------------------------------ FR-004
     def test_empty_variation_id_becomes_none(self):
         impression = self._build_impression('111127', '111182', '')
         decision, _ = self._dispatched_decision(impression)
@@ -1365,7 +1361,6 @@ def test_whitespace_variation_id_becomes_none(self):
         decision, _ = self._dispatched_decision(impression)
         self.assertIsNone(decision['variation_id'])
 
-    # ------------------------------------------------------------------ FR-005
     def test_normalization_applies_to_rollout_decisions(self):
         impression = self._build_impression(
             '111127', '', 'bad_var', rule_type='rollout'
@@ -1403,9 +1398,8 @@ def test_holdout_with_opaque_layer_id_passes_through(self):
         self.assertEqual('default-12345', decision['campaign_id'])
         self.assertEqual('default-12345', event['entity_id'])
 
-    # ------------------------------------------------------------------ FR-006
     def test_event_still_dispatches_when_all_ids_invalid(self):
-        """FR-006: never drop / fail dispatch."""
+        """Event must still dispatch even when all IDs are invalid."""
         impression = self._build_impression('', '', '')
         log_event = EventFactory.create_log_event(impression, self.logger)
         self.assertIsNotNone(log_event)
@@ -1416,9 +1410,8 @@ def test_event_still_dispatches_when_all_ids_invalid(self):
         self.assertEqual('', event['entity_id'])
         self.assertIsNone(decision['variation_id'])
 
-    # ------------------------------------------------------------------ FR-009
     def test_entity_id_equals_campaign_id_byte_for_byte(self):
-        """FR-009: ``events[].entity_id`` must equal ``decisions[].campaign_id``."""
+        """``events[].entity_id`` must equal ``decisions[].campaign_id``."""
         for layer_id, exp_id, expected in [
             ('111182', '111127', '111182'),         # numeric campaign_id wins
             ('', '111127', '111127'),                # empty falls back to experiment_id
@@ -1433,11 +1426,8 @@ def test_entity_id_equals_campaign_id_byte_for_byte(self):
                 self.assertEqual(expected, decision['campaign_id'])
                 self.assertEqual(decision['campaign_id'], event['entity_id'])
 
-    # ----------------------------------------------------------------- FR-010
     def test_conversion_event_entity_id_unchanged(self):
-        """FR-010: conversion events derive entity_id from event.id, not the
-        normalizer.
-        """
+        """Conversion events derive entity_id from event.id, not the normalizer."""
         from optimizely.event.user_event_factory import UserEventFactory
 
         with mock.patch('time.time', return_value=42.123), mock.patch(

tests/test_event_id_normalizer.py

@@ -19,7 +19,7 @@
 
 
 class IsNonEmptyStringTest(unittest.TestCase):
-    """Cover :func:`event_id_normalizer.is_non_empty_string` (FR-001/FR-009).
+    """Cover :func:`event_id_normalizer.is_non_empty_string`.
 
     Any non-empty string is valid for ``campaign_id`` / ``entity_id`` — IDs
     may be numeric like ``"12345"`` or opaque like ``"default-12345"``.
@@ -35,8 +35,8 @@ def test_returns_true_for_opaque_string(self):
         self.assertTrue(event_id_normalizer.is_non_empty_string('abc'))
 
     def test_returns_true_for_whitespace_string(self):
-        # Whitespace is a non-empty string and so is accepted; the spec
-        # explicitly defers any character-content validation upstream.
+        # Whitespace is a non-empty string and so is accepted;
+        # character-content validation is deferred upstream.
         self.assertTrue(event_id_normalizer.is_non_empty_string(' '))
 
     def test_returns_false_for_empty_string(self):
@@ -46,8 +46,7 @@ def test_returns_false_for_none(self):
         self.assertFalse(event_id_normalizer.is_non_empty_string(None))
 
     def test_returns_false_for_non_string_types(self):
-        # Non-string types are out of scope per the spec assumptions; the
-        # predicate rejects them so the fallback path fires.
+        # Non-string types are rejected so the fallback path fires.
         self.assertFalse(event_id_normalizer.is_non_empty_string(12345))
         self.assertFalse(event_id_normalizer.is_non_empty_string(123.0))
         self.assertFalse(event_id_normalizer.is_non_empty_string(True))
@@ -58,7 +57,7 @@ def test_returns_false_for_non_string_types(self):
 class IsNumericIdStringTest(unittest.TestCase):
     """Cover :func:`event_id_normalizer.is_numeric_id_string` edge cases.
 
-    Used only for ``variation_id`` (FR-003), which retains the strict
+    Used only for ``variation_id``, which retains the strict
     decimal-digit contract.
     """
 
@@ -70,7 +69,7 @@ def test_returns_true_for_single_digit(self):
         self.assertTrue(event_id_normalizer.is_numeric_id_string('9'))
 
     def test_returns_true_for_leading_zeros(self):
-        # FR-003 explicitly allows leading zeros.
+        # Leading zeros are explicitly allowed.
         self.assertTrue(event_id_normalizer.is_numeric_id_string('007'))
         self.assertTrue(event_id_normalizer.is_numeric_id_string('00000'))
 
@@ -81,7 +80,7 @@ def test_returns_false_for_none(self):
         self.assertFalse(event_id_normalizer.is_numeric_id_string(None))
 
     def test_returns_false_for_int(self):
-        # FR-003 requires the value to be a string.
+        # The value must be a string.
         self.assertFalse(event_id_normalizer.is_numeric_id_string(12345))
         self.assertFalse(event_id_normalizer.is_numeric_id_string(0))
 
@@ -130,10 +129,10 @@ def test_returns_false_for_collections(self):
 
 
 class NormalizeCampaignIdTest(unittest.TestCase):
-    """Cover :func:`event_id_normalizer.normalize_campaign_id` per FR-001/002, FR-009.
+    """Cover :func:`event_id_normalizer.normalize_campaign_id`.
 
-    Per the relaxed spec, any non-empty string is valid for campaign_id —
-    fallback to ``experiment_id`` fires only on empty/None/missing.
+    Any non-empty string is valid for campaign_id — fallback to
+    ``experiment_id`` fires only on empty/None/missing.
     """
 
     def test_returns_campaign_id_when_numeric(self):
@@ -180,7 +179,7 @@ def test_falls_back_to_opaque_experiment_id(self):
         )
 
     def test_returns_empty_string_when_both_empty_or_none(self):
-        # Do not drop / fail dispatch (FR-006); return ''.
+        # Do not drop / fail dispatch; return ''.
         self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, None))
         self.assertEqual('', event_id_normalizer.normalize_campaign_id('', ''))
         self.assertEqual('', event_id_normalizer.normalize_campaign_id(None, ''))
@@ -193,7 +192,7 @@ def test_preserves_leading_zeros(self):
 
 
 class NormalizeVariationIdTest(unittest.TestCase):
-    """Cover :func:`event_id_normalizer.normalize_variation_id` per FR-003/004.
+    """Cover :func:`event_id_normalizer.normalize_variation_id`.
 
     ``variation_id`` retains the strict numeric-string contract.
     """

tests/test_holdout_config.py

@@ -471,7 +471,7 @@ def test_local_holdouts_section_entries_excluded_from_global_list(self):
         self.assertEqual(config.get_global_holdouts(), [])
 
     def test_local_holdouts_missing_included_rules_logged_and_excluded(self):
-        """Entries in 'localHoldouts' without 'includedRules' are invalid per spec.
+        """Entries in 'localHoldouts' without 'includedRules' are invalid.
 
         SDK must log an error and exclude the entry from evaluation. It must NOT
         fall back to global application (the partition between sections is hard).