From e29585b7085ab52e37489ca2ee2563224b846b96 Mon Sep 17 00:00:00 2001 From: Swetha Gopalakrishnan Date: Mon, 21 Sep 2026 15:12:55 +0000 Subject: [PATCH] chore(governance): remove generic tech-council from team_hierarchy and update tests --- org-tools/governance/README.md | 1 - .../governance/docs/validation_report.md | 4 +- org-tools/governance/rules/.github-rules.yml | 1 - .../governance/rules/conformance-rules.yml | 1 - org-tools/governance/rules/js-sdk-rules.yml | 1 - .../rules/meeting-minutes-rules.yml | 1 - .../governance/rules/python-sdk-rules.yml | 1 - org-tools/governance/rules/samples-rules.yml | 1 - org-tools/governance/rules/ucp-rules.yml | 1 - .../governance/rules/ucp-schema-rules.yml | 1 - .../tests/test_governance_config_parser.py | 128 ++++++++++--- org-tools/governance/tests/test_pr_models.py | 18 +- .../governance/tests/test_pr_validator.py | 168 +++++++++++++----- .../tests/test_validation_logger.py | 60 +++---- 14 files changed, 269 insertions(+), 118 deletions(-) diff --git a/org-tools/governance/README.md b/org-tools/governance/README.md index aae46f1..5812b22 100644 --- a/org-tools/governance/README.md +++ b/org-tools/governance/README.md @@ -47,7 +47,6 @@ Defines hierarchical roles with integer clearance tiers. Approvals from higher l team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/docs/validation_report.md b/org-tools/governance/docs/validation_report.md index 29cd76d..6da171e 100644 --- a/org-tools/governance/docs/validation_report.md +++ b/org-tools/governance/docs/validation_report.md @@ -112,10 +112,10 @@ For example, if the hierarchy is defined as: 1. `devops-maintainers` (Level 1) 2. `maintainers` (Level 2) -3. `tech-council` / Domain Tech Councils (`shopping-tech-council`, `food-tech-council`, `lodging-tech-council`, `payments-tech-council`) (Level 3) +3. Domain Tech Councils (`shopping-tech-council`, `food-tech-council`, `lodging-tech-council`, `payments-tech-council`) (Level 3) 4. `governance-council` (Level 4) -- A requirement for **`team 'maintainers' or higher in the UCP governance hierarchy`** (Level 2+) can be satisfied by approvals from members of `maintainers`, any Tech Council (`tech-council`, `shopping-tech-council`, `food-tech-council`, `lodging-tech-council`, `payments-tech-council`), or `governance-council`. +- A requirement for **`team 'maintainers' or higher in the UCP governance hierarchy`** (Level 2+) can be satisfied by approvals from members of `maintainers`, any Tech Council (`shopping-tech-council`, `food-tech-council`, `lodging-tech-council`, `payments-tech-council`), or `governance-council`. - A requirement for **`team 'shopping-tech-council' or higher in the UCP governance hierarchy`** (Level 3+) can be satisfied by approvals from members of any Tech Council or `governance-council`. - A requirement for an exact team (e.g., **`team 'devops-maintainers'`**) must be approved by an explicit member of that team, regardless of hierarchy. diff --git a/org-tools/governance/rules/.github-rules.yml b/org-tools/governance/rules/.github-rules.yml index 6cdffdc..18fbe10 100644 --- a/org-tools/governance/rules/.github-rules.yml +++ b/org-tools/governance/rules/.github-rules.yml @@ -51,7 +51,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/conformance-rules.yml b/org-tools/governance/rules/conformance-rules.yml index edec3a5..c50a56b 100644 --- a/org-tools/governance/rules/conformance-rules.yml +++ b/org-tools/governance/rules/conformance-rules.yml @@ -47,7 +47,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/js-sdk-rules.yml b/org-tools/governance/rules/js-sdk-rules.yml index a19f5e6..ee03f1e 100644 --- a/org-tools/governance/rules/js-sdk-rules.yml +++ b/org-tools/governance/rules/js-sdk-rules.yml @@ -54,7 +54,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/meeting-minutes-rules.yml b/org-tools/governance/rules/meeting-minutes-rules.yml index a323856..8c9c04b 100644 --- a/org-tools/governance/rules/meeting-minutes-rules.yml +++ b/org-tools/governance/rules/meeting-minutes-rules.yml @@ -91,7 +91,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/python-sdk-rules.yml b/org-tools/governance/rules/python-sdk-rules.yml index 4775185..1e4fa90 100644 --- a/org-tools/governance/rules/python-sdk-rules.yml +++ b/org-tools/governance/rules/python-sdk-rules.yml @@ -50,7 +50,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/samples-rules.yml b/org-tools/governance/rules/samples-rules.yml index 9caf99a..6d718e3 100644 --- a/org-tools/governance/rules/samples-rules.yml +++ b/org-tools/governance/rules/samples-rules.yml @@ -48,7 +48,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/ucp-rules.yml b/org-tools/governance/rules/ucp-rules.yml index 35db916..64acb3e 100644 --- a/org-tools/governance/rules/ucp-rules.yml +++ b/org-tools/governance/rules/ucp-rules.yml @@ -87,7 +87,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/rules/ucp-schema-rules.yml b/org-tools/governance/rules/ucp-schema-rules.yml index 5bfd65d..b3dc3c3 100644 --- a/org-tools/governance/rules/ucp-schema-rules.yml +++ b/org-tools/governance/rules/ucp-schema-rules.yml @@ -49,7 +49,6 @@ proxy_reviewers: team_hierarchy: devops-maintainers: 1 maintainers: 2 - tech-council: 3 shopping-tech-council: 3 food-tech-council: 3 lodging-tech-council: 3 diff --git a/org-tools/governance/tests/test_governance_config_parser.py b/org-tools/governance/tests/test_governance_config_parser.py index 341548b..fe92174 100644 --- a/org-tools/governance/tests/test_governance_config_parser.py +++ b/org-tools/governance/tests/test_governance_config_parser.py @@ -45,17 +45,35 @@ def test_parse_valid_dict(self): "team_hierarchy": { "devops": 1, "maintainers": 2, - "tech-council": 3, + "shopping-tech-council": 3, + "food-tech-council": 3, + "lodging-tech-council": 3, + "payments-tech-council": 3, }, "proxy_reviewers": ["proxy-user"], "fallback": {"requires": [{"min_team": "maintainers", "min_approvals": 1}]}, "rules": [ { - "name": "Core Rule", - "patterns": ["source/**/*.py"], - "excluded_patterns": ["source/special/**/*.py"], - "requires": [{"team": "tech-council", "min_approvals": 2}], - } + "name": "Shopping Tech Council Rule", + "patterns": ["tc/shopping/**"], + "excluded_patterns": ["tc/shopping/special/**/*.py"], + "requires": [{"team": "shopping-tech-council", "min_approvals": 1}], + }, + { + "name": "Food Tech Council Rule", + "patterns": ["tc/food/**"], + "requires": [{"team": "food-tech-council", "min_approvals": 1}], + }, + { + "name": "Lodging Tech Council Rule", + "patterns": ["tc/lodging/**"], + "requires": [{"team": "lodging-tech-council", "min_approvals": 1}], + }, + { + "name": "Payments Tech Council Rule", + "patterns": ["tc/payments/**"], + "requires": [{"team": "payments-tech-council", "min_approvals": 1}], + }, ], } @@ -68,7 +86,10 @@ def test_parse_valid_dict(self): { "devops": Team("devops", 1), "maintainers": Team("maintainers", 2), - "tech-council": Team("tech-council", 3), + "shopping-tech-council": Team("shopping-tech-council", 3), + "food-tech-council": Team("food-tech-council", 3), + "lodging-tech-council": Team("lodging-tech-council", 3), + "payments-tech-council": Team("payments-tech-council", 3), }, ) @@ -82,14 +103,47 @@ def test_parse_valid_dict(self): self.assertIsNone(config.fallback[0].team) # Assert Rules - self.assertEqual(len(config.rules), 1) - rule = config.rules[0] - self.assertEqual(rule.name, "Core Rule") - self.assertEqual(rule.patterns, ["source/**/*.py"]) - self.assertEqual(rule.excluded_patterns, ["source/special/**/*.py"]) - self.assertEqual(len(rule.requires_all), 1) - self.assertEqual(rule.requires_all[0].min_approvals, 2) - self.assertEqual(rule.requires_all[0].team, Team("tech-council", 3)) + self.assertEqual(len(config.rules), 4) + + rule_shopping = config.rules[0] + self.assertEqual(rule_shopping.name, "Shopping Tech Council Rule") + self.assertEqual(rule_shopping.patterns, ["tc/shopping/**"]) + self.assertEqual( + rule_shopping.excluded_patterns, ["tc/shopping/special/**/*.py"] + ) + self.assertEqual(len(rule_shopping.requires_all), 1) + self.assertEqual(rule_shopping.requires_all[0].min_approvals, 1) + self.assertEqual( + rule_shopping.requires_all[0].team, Team("shopping-tech-council", 3) + ) + + rule_food = config.rules[1] + self.assertEqual(rule_food.name, "Food Tech Council Rule") + self.assertEqual(rule_food.patterns, ["tc/food/**"]) + self.assertEqual(rule_food.excluded_patterns, []) + self.assertEqual(len(rule_food.requires_all), 1) + self.assertEqual(rule_food.requires_all[0].min_approvals, 1) + self.assertEqual(rule_food.requires_all[0].team, Team("food-tech-council", 3)) + + rule_lodging = config.rules[2] + self.assertEqual(rule_lodging.name, "Lodging Tech Council Rule") + self.assertEqual(rule_lodging.patterns, ["tc/lodging/**"]) + self.assertEqual(rule_lodging.excluded_patterns, []) + self.assertEqual(len(rule_lodging.requires_all), 1) + self.assertEqual(rule_lodging.requires_all[0].min_approvals, 1) + self.assertEqual( + rule_lodging.requires_all[0].team, Team("lodging-tech-council", 3) + ) + + rule_payments = config.rules[3] + self.assertEqual(rule_payments.name, "Payments Tech Council Rule") + self.assertEqual(rule_payments.patterns, ["tc/payments/**"]) + self.assertEqual(rule_payments.excluded_patterns, []) + self.assertEqual(len(rule_payments.requires_all), 1) + self.assertEqual(rule_payments.requires_all[0].min_approvals, 1) + self.assertEqual( + rule_payments.requires_all[0].team, Team("payments-tech-council", 3) + ) def test_parse_file_not_found(self): """Test parse_file raises FileNotFoundError for missing file.""" @@ -179,17 +233,35 @@ def test_parse_mixed_case_normalization(self): """Test that team names in hierarchy, rules, and fallback are normalized to lowercase.""" yaml_data = { "team_hierarchy": { - "Tech-Council": 3, + "Shopping-Tech-Council": 3, + "Food-Tech-Council": 3, + "Lodging-Tech-Council": 3, + "Payments-Tech-Council": 3, "MAINTAINERS": 2, }, "proxy_reviewers": ["proxy-user"], "fallback": {"requires": [{"min_team": "MAINTAINERS", "min_approvals": 1}]}, "rules": [ { - "name": "Core Rule", - "patterns": ["source/**/*.py"], - "requires": [{"team": "Tech-Council", "min_approvals": 2}], - } + "name": "Shopping Tech Council Rule", + "patterns": ["tc/shopping/**"], + "requires": [{"team": "Shopping-Tech-Council", "min_approvals": 1}], + }, + { + "name": "Food Tech Council Rule", + "patterns": ["tc/food/**"], + "requires": [{"team": "Food-Tech-Council", "min_approvals": 1}], + }, + { + "name": "Lodging Tech Council Rule", + "patterns": ["tc/lodging/**"], + "requires": [{"team": "Lodging-Tech-Council", "min_approvals": 1}], + }, + { + "name": "Payments Tech Council Rule", + "patterns": ["tc/payments/**"], + "requires": [{"team": "Payments-Tech-Council", "min_approvals": 1}], + }, ], } @@ -200,14 +272,26 @@ def test_parse_mixed_case_normalization(self): self.assertEqual( config.teams, { - "tech-council": Team("tech-council", 3), + "shopping-tech-council": Team("shopping-tech-council", 3), + "food-tech-council": Team("food-tech-council", 3), + "lodging-tech-council": Team("lodging-tech-council", 3), + "payments-tech-council": Team("payments-tech-council", 3), "maintainers": Team("maintainers", 2), }, ) # Verify fallback and rules resolved team references are lowercased self.assertEqual(config.fallback[0].min_team.name, "maintainers") - self.assertEqual(config.rules[0].requires_all[0].team.name, "tech-council") + self.assertEqual( + config.rules[0].requires_all[0].team.name, "shopping-tech-council" + ) + self.assertEqual(config.rules[1].requires_all[0].team.name, "food-tech-council") + self.assertEqual( + config.rules[2].requires_all[0].team.name, "lodging-tech-council" + ) + self.assertEqual( + config.rules[3].requires_all[0].team.name, "payments-tech-council" + ) class TestGovernanceConfigValidator(unittest.TestCase): diff --git a/org-tools/governance/tests/test_pr_models.py b/org-tools/governance/tests/test_pr_models.py index 7e54a2e..a5cf5db 100644 --- a/org-tools/governance/tests/test_pr_models.py +++ b/org-tools/governance/tests/test_pr_models.py @@ -53,7 +53,7 @@ def test_pull_request_lowercase_normalization(self): changed_files=["README.md"], reviews=[review_input], assigned_user_names=["Dave", "EVE"], - assigned_team_names=["Tech-Council", "ADMINS"], + assigned_team_names=["Shopping-Tech-Council", "ADMINS"], ) expected = PullRequest( @@ -63,7 +63,7 @@ def test_pull_request_lowercase_normalization(self): changed_files=["README.md"], reviews=[Review(user="bob", state=ReviewState.APPROVED)], assigned_user_names=["dave", "eve"], - assigned_team_names=["tech-council", "admins"], + assigned_team_names=["shopping-tech-council", "admins"], ) self.assertEqual(expected, pr) @@ -71,18 +71,18 @@ def test_team_memberships_lowercase_normalization(self): """Test that TeamMemberships keys and member sets are normalized to lowercase.""" memberships = TeamMemberships.create( members_by_team={ - "Tech-Council": {"Alice", "BOB"}, + "Shopping-Tech-Council": {"Alice", "BOB"}, "ADMINS": {"charlie", "Dave"}, }, teams={ - "tech-council": Team.create("tech-council", 3), + "shopping-tech-council": Team.create("shopping-tech-council", 3), "admins": Team.create("admins", 2), }, ) self.assertEqual( memberships.members_by_team, { - Team.create("tech-council", 3): {"alice", "bob"}, + Team.create("shopping-tech-council", 3): {"alice", "bob"}, Team.create("admins", 2): {"charlie", "dave"}, }, ) @@ -90,12 +90,12 @@ def test_team_memberships_lowercase_normalization(self): def test_user_create_factory(self): """Test that User.create resolves teams, level, and normalizes username.""" teams = { - "tech-council": Team.create("tech-council", 3), + "shopping-tech-council": Team.create("shopping-tech-council", 3), "admins": Team.create("admins", 2), } memberships = TeamMemberships.create( members_by_team={ - "tech-council": {"alice", "bob"}, + "shopping-tech-council": {"alice", "bob"}, "admins": {"bob", "charlie"}, }, teams=teams, @@ -104,13 +104,13 @@ def test_user_create_factory(self): # User in multiple teams, max level should be the highest (3) user_bob = User.create("BOB", memberships) self.assertEqual(user_bob.username, "bob") - self.assertEqual(user_bob.teams, {"tech-council", "admins"}) + self.assertEqual(user_bob.teams, {"shopping-tech-council", "admins"}) self.assertEqual(user_bob.level, 3) # User in one team user_alice = User.create("Alice", memberships) self.assertEqual(user_alice.username, "alice") - self.assertEqual(user_alice.teams, {"tech-council"}) + self.assertEqual(user_alice.teams, {"shopping-tech-council"}) self.assertEqual(user_alice.level, 3) # User in no teams diff --git a/org-tools/governance/tests/test_pr_validator.py b/org-tools/governance/tests/test_pr_validator.py index 5816f31..77e689e 100644 --- a/org-tools/governance/tests/test_pr_validator.py +++ b/org-tools/governance/tests/test_pr_validator.py @@ -69,7 +69,10 @@ def setUp(self): self.hierarchy = { "devops": Team(name="devops", level=1), "maintainers": Team(name="maintainers", level=2), - "tech-council": Team(name="tech-council", level=3), + "shopping-tech-council": Team(name="shopping-tech-council", level=3), + "lodging-tech-council": Team(name="lodging-tech-council", level=3), + "food-tech-council": Team(name="food-tech-council", level=3), + "payments-tech-council": Team(name="payments-tech-council", level=3), "governance-council": Team(name="governance-council", level=4), } @@ -89,7 +92,48 @@ def setUp(self): patterns=["source/**"], requires_all=[ RuleRequirement( - min_approvals=1, min_team=self.hierarchy["tech-council"] + min_approvals=1, + min_team=self.hierarchy["governance-council"], + ) + ], + ), + GovernanceRule( + name="Shopping Tech Council", + patterns=["tc/shopping/**"], + requires_all=[ + RuleRequirement( + min_approvals=1, + min_team=self.hierarchy["shopping-tech-council"], + ) + ], + ), + GovernanceRule( + name="Lodging Tech Council", + patterns=["tc/lodging/**"], + requires_all=[ + RuleRequirement( + min_approvals=1, + min_team=self.hierarchy["lodging-tech-council"], + ) + ], + ), + GovernanceRule( + name="Food Tech Council", + patterns=["tc/food/**"], + requires_all=[ + RuleRequirement( + min_approvals=1, + min_team=self.hierarchy["food-tech-council"], + ) + ], + ), + GovernanceRule( + name="Payments Tech Council", + patterns=["tc/payments/**"], + requires_all=[ + RuleRequirement( + min_approvals=1, + min_team=self.hierarchy["payments-tech-council"], ) ], ), @@ -116,7 +160,10 @@ def setUp(self): members_by_team={ "devops": {"dev1", "dev2"}, "maintainers": {"maint1", "maint2", "tc-member1"}, - "tech-council": {"tc-member1", "tc-member2"}, + "shopping-tech-council": {"tc-member1", "tc-member2"}, + "lodging-tech-council": {"lodging-tc-member1", "lodging-tc-member2"}, + "food-tech-council": {"food-tc-member1", "food-tc-member2"}, + "payments-tech-council": {"tc-member1", "payments-tc-member2"}, "governance-council": { "gov-member1", "gov-member2", @@ -159,7 +206,7 @@ def test_proxy_voter_override_passes(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[ Review(user="tc-member1", state=ReviewState.CHANGES_REQUESTED), Review(user="proxy1", state=ReviewState.APPROVED), @@ -175,7 +222,7 @@ def test_standard_rule_success(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[ Review(user="tc-member1", state=ReviewState.APPROVED), ], @@ -190,7 +237,7 @@ def test_insufficient_approvals_fails(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[ Review(user="dev1", state=ReviewState.APPROVED), ], @@ -399,10 +446,10 @@ def test_changes_requested_blocks(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/lodging/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), - Review(user="tc-member2", state=ReviewState.CHANGES_REQUESTED), + Review(user="lodging-tc-member1", state=ReviewState.APPROVED), + Review(user="lodging-tc-member2", state=ReviewState.CHANGES_REQUESTED), ], ) res = self.validator.validate(pr) @@ -417,9 +464,9 @@ def test_unauthorized_changes_requested_does_not_block(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/lodging/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), + Review(user="lodging-tc-member1", state=ReviewState.APPROVED), Review(user="dev1", state=ReviewState.CHANGES_REQUESTED), ], ) @@ -432,9 +479,9 @@ def test_proxy_changes_requested_blocks(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/food/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), + Review(user="food-tc-member1", state=ReviewState.APPROVED), Review(user="proxy2", state=ReviewState.CHANGES_REQUESTED), ], ) @@ -446,14 +493,14 @@ def test_proxy_changes_requested_blocks(self): def test_self_approval_restrictions(self): """Test restrictions on self-approval.""" - # Author tc-member1 cannot approve their own PR + # Author food-tc-member1 cannot approve their own PR pr = PullRequest( number=1, - author="tc-member1", + author="food-tc-member1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/food/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), + Review(user="food-tc-member1", state=ReviewState.APPROVED), ], ) res = self.validator.validate(pr) @@ -466,10 +513,10 @@ def test_dismissed_reviews(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/payments/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), - Review(user="tc-member1", state=ReviewState.DISMISSED), + Review(user="payments-tc-member2", state=ReviewState.APPROVED), + Review(user="payments-tc-member2", state=ReviewState.DISMISSED), ], ) res = self.validator.validate(pr) @@ -481,10 +528,10 @@ def test_commented_reviews_ignored(self): number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/payments/main.py"], reviews=[ - Review(user="tc-member1", state=ReviewState.APPROVED), - Review(user="tc-member1", state=ReviewState.COMMENTED), + Review(user="payments-tc-member2", state=ReviewState.APPROVED), + Review(user="payments-tc-member2", state=ReviewState.COMMENTED), ], ) res = self.validator.validate(pr) @@ -570,14 +617,14 @@ def test_multi_level_requirement(self): def test_requirement_status_reporting(self): """Test requirement status reporting in validation result.""" - # We check source/main.py (requires 1 from min_team tech-council) + # We check tc/shopping/main.py (requires 1 from min_team shopping-tech-council) # We request tc-member2 to review (1 eligible reviewer assigned) # but have 0 approvals. pr = PullRequest( number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[], assigned_user_names=["tc-member2"], assigned_team_names=[], @@ -588,7 +635,9 @@ def test_requirement_status_reporting(self): self.assertEqual(len(res.requirement_statuses), 1) status = res.requirement_statuses[0] - self.assertEqual(status.requirement.min_team, self.hierarchy["tech-council"]) + self.assertEqual( + status.requirement.min_team, self.hierarchy["shopping-tech-council"] + ) self.assertEqual(status.requirement.min_approvals, 1) self.assertEqual(status.approved_count, 0) self.assertEqual(status.assigned_count, 1) @@ -596,18 +645,18 @@ def test_requirement_status_reporting(self): def test_assigned_team_expansion_reporting(self): """Test that assigned teams are expanded to their eligible members in status reporting.""" - # We check source/main.py (requires 1 from min_team tech-council) - # We assign the tech-council team to review. - # Members of tech-council are tc-member1 and tc-member2. + # We check tc/shopping/main.py (requires 1 from min_team shopping-tech-council) + # We assign the shopping-tech-council team to review. + # Members of shopping-tech-council are tc-member1 and tc-member2. # Both are eligible reviewers. pr = PullRequest( number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[], assigned_user_names=[], - assigned_team_names=["tech-council"], + assigned_team_names=["shopping-tech-council"], ) res = self.validator.validate(pr) self.assertFalse(res.is_mergeable) @@ -615,10 +664,12 @@ def test_assigned_team_expansion_reporting(self): self.assertEqual(len(res.requirement_statuses), 1) status = res.requirement_statuses[0] - self.assertEqual(status.requirement.min_team, self.hierarchy["tech-council"]) + self.assertEqual( + status.requirement.min_team, self.hierarchy["shopping-tech-council"] + ) self.assertEqual(status.requirement.min_approvals, 1) self.assertEqual(status.approved_count, 0) - # assigned_count should be 2 because tech-council has 2 members (tc-member1, tc-member2) + # assigned_count should be 2 because shopping-tech-council has 2 members (tc-member1, tc-member2) # and both satisfy the min_team hierarchy requirement. self.assertEqual(status.assigned_count, 2) self.assertFalse(status.is_satisfied) @@ -693,16 +744,16 @@ def test_wildcard_matching(self): def test_rule_excludes(self): """Test rule pattern exclusion matching.""" - # Rule A matches source/** but excludes source/special/** + # Rule A matches tc/shopping/** but excludes tc/shopping/special/** rule = GovernanceRule( name="Source Rule", - patterns=["source/**"], + patterns=["tc/shopping/**"], requires_all=[ RuleRequirement( - min_approvals=1, min_team=self.hierarchy["tech-council"] + min_approvals=1, min_team=self.hierarchy["shopping-tech-council"] ) ], - excluded_patterns=["source/special/**"], + excluded_patterns=["tc/shopping/special/**"], ) config = GovernanceConfig( teams=self.hierarchy, @@ -712,18 +763,18 @@ def test_rule_excludes(self): ) validator = PullRequestValidator(config, self.memberships) - # Case A: Matches source/main.py (not excluded) + # Case A: Matches tc/shopping/main.py (not excluded) pr_ok = PullRequest( number=1, author="author1", is_draft=False, - changed_files=["source/main.py"], + changed_files=["tc/shopping/main.py"], reviews=[Review(user="tc-member1", state=ReviewState.APPROVED)], ) res_ok = validator.validate(pr_ok) self.assertTrue(res_ok.is_mergeable) - # Case B: Excluded source/special/helper.py (should use fallback rule) + # Case B: Excluded tc/shopping/special/helper.py (should use fallback rule) config_with_fallback = GovernanceConfig( teams=self.hierarchy, rules=[rule], @@ -740,7 +791,7 @@ def test_rule_excludes(self): number=1, author="author1", is_draft=False, - changed_files=["source/special/helper.py"], + changed_files=["tc/shopping/special/helper.py"], reviews=[ Review(user="dev1", state=ReviewState.APPROVED) ], # dev1 (L1) not enough for L2 fallback @@ -754,23 +805,48 @@ def test_rule_excludes(self): def test_merge_requirements_multiple_rules(self): """Test that requirements from multiple rules are merged by taking the maximum approvals needed.""" # Rule 1 (devops-rule): needs 2 devops approvals - # Rule 2 (central-rule): needs 1 devops approval and 1 tech-council approval - # Merged requirements should be: 2 devops approvals and 1 tech-council approval. + devops_rule = GovernanceRule( + name="devops-rule", + patterns=["scripts/**"], + requires_all=[ + RuleRequirement(min_approvals=2, min_team=self.hierarchy["devops"]) + ], + ) + # Rule 2 (central-rule): needs 1 devops approval and 1 shopping-tech-council approval + central_rule = GovernanceRule( + name="central-rule", + patterns=["tc/shopping/test.py"], + requires_all=[ + RuleRequirement(min_approvals=1, min_team=self.hierarchy["devops"]), + RuleRequirement( + min_approvals=1, min_team=self.hierarchy["shopping-tech-council"] + ), + ], + ) + config = GovernanceConfig( + teams=self.hierarchy, + rules=[devops_rule, central_rule], + fallback=self.fallback, + proxy_reviewers=self.proxy_reviewers, + ) + validator = PullRequestValidator(config, self.memberships) + + # Merged requirements should be: 2 devops approvals and 1 shopping-tech-council approval. pr = PullRequest( number=1, author="author1", is_draft=False, # Changed files matching both rules - changed_files=["source/ops/deploy.sh", "source/central/core.py"], + changed_files=["scripts/deploy.sh", "tc/shopping/test.py"], reviews=[ # 2 devops approvals Review(user="dev1", state=ReviewState.APPROVED), Review(user="dev2", state=ReviewState.APPROVED), - # 1 tech-council approval + # 1 shopping-tech-council approval Review(user="tc-member1", state=ReviewState.APPROVED), ], ) - res = self.validator.validate(pr) + res = validator.validate(pr) self.assertTrue(res.is_mergeable) self.assertEqual(res.mergeable_reason, MergeableReason.RULES_SATISFIED) diff --git a/org-tools/governance/tests/test_validation_logger.py b/org-tools/governance/tests/test_validation_logger.py index 88a5e1f..cee5bd7 100644 --- a/org-tools/governance/tests/test_validation_logger.py +++ b/org-tools/governance/tests/test_validation_logger.py @@ -157,7 +157,7 @@ def test_error_changes_requested(self, mock_open): @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_error_insufficient_approvals_summary(self, mock_open): """Test summary message when PR has insufficient approvals.""" - team = Team(name="tech-council", level=3) + team = Team(name="shopping-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status = RequirementStatus( requirement=req, @@ -187,17 +187,17 @@ def test_error_insufficient_approvals_summary(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'shopping-tech-council'\n" " * **Met:** 🔴 No (1/2 approved)\n" - " * **Pending:** Needs 1 approval from team 'tech-council' (0 eligible reviewers assigned). " - "Waiting for 1 more reviewer(s) to be assigned from team 'tech-council'.\n" + " * **Pending:** Needs 1 approval from team 'shopping-tech-council' (0 eligible reviewers assigned). " + "Waiting for 1 more reviewer(s) to be assigned from team 'shopping-tech-council'.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content) @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_waiting_for_approval(self, mock_open): """Test write_summary outputs 'Waiting for approval' when reviewers are assigned.""" - team = Team(name="tech-council", level=3) + team = Team(name="food-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status = RequirementStatus( requirement=req, @@ -227,17 +227,17 @@ def test_write_summary_waiting_for_approval(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'food-tech-council'\n" " * **Met:** 🔴 No (1/2 approved)\n" - " * **Pending:** Needs 1 approval from team 'tech-council' (1 eligible reviewer assigned). " - "Waiting for approval from team 'tech-council'.\n" + " * **Pending:** Needs 1 approval from team 'food-tech-council' (1 eligible reviewer assigned). " + "Waiting for approval from team 'food-tech-council'.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content) @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_assign_more_reviewers(self, mock_open): """Test write_summary outputs 'Assign X more reviewers' when not enough are assigned.""" - team = Team(name="tech-council", level=3) + team = Team(name="lodging-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status = RequirementStatus( requirement=req, @@ -267,17 +267,17 @@ def test_write_summary_assign_more_reviewers(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'lodging-tech-council'\n" " * **Met:** 🔴 No (0/2 approved)\n" - " * **Pending:** Needs 2 approvals from team 'tech-council' (1 eligible reviewer assigned). " - "Waiting for 1 more reviewer(s) to be assigned from team 'tech-council'.\n" + " * **Pending:** Needs 2 approvals from team 'lodging-tech-council' (1 eligible reviewer assigned). " + "Waiting for 1 more reviewer(s) to be assigned from team 'lodging-tech-council'.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content) @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_with_approvers(self, mock_open): """Test write_summary outputs approver details for partially filled slots.""" - team = Team(name="tech-council", level=3) + team = Team(name="payments-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status = RequirementStatus( requirement=req, @@ -308,17 +308,17 @@ def test_write_summary_with_approvers(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'payments-tech-council'\n" " * **Met:** 🔴 No (1/2 approved - approved by: alice)\n" - " * **Pending:** Needs 1 approval from team 'tech-council' (1 eligible reviewer assigned). " - "Waiting for approval from team 'tech-council'.\n" + " * **Pending:** Needs 1 approval from team 'payments-tech-council' (1 eligible reviewer assigned). " + "Waiting for approval from team 'payments-tech-council'.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content) @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_satisfied_requirement_omits_pending(self, mock_open): """Test write_summary omits the pending/unmet line entirely when requirement is satisfied.""" - team = Team(name="tech-council", level=3) + team = Team(name="shopping-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status = RequirementStatus( requirement=req, @@ -349,7 +349,7 @@ def test_write_summary_satisfied_requirement_omits_pending(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'shopping-tech-council'\n" " * **Met:** 🟢 Yes (2/2 approved - approved by: alice, bob)\n" ) self.assertEqual(expected, written_content) @@ -357,7 +357,7 @@ def test_write_summary_satisfied_requirement_omits_pending(self, mock_open): @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_with_file_statuses(self, mock_open): """Test write_summary fully formats the file-by-file breakdown section.""" - team = Team(name="tech-council", level=3) + team = Team(name="shopping-tech-council", level=3) req = RuleRequirement(min_approvals=2, team=team) status_satisfied = RequirementStatus( @@ -410,10 +410,10 @@ def test_write_summary_with_file_statuses(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **2 approvals** from team 'tech-council'\n" + "* **2 approvals** from team 'shopping-tech-council'\n" " * **Met:** 🔴 No (0/2 approved)\n" - " * **Pending:** Needs 2 approvals from team 'tech-council' (1 eligible reviewer assigned). " - "Waiting for 1 more reviewer(s) to be assigned from team 'tech-council'.\n" + " * **Pending:** Needs 2 approvals from team 'shopping-tech-council' (1 eligible reviewer assigned). " + "Waiting for 1 more reviewer(s) to be assigned from team 'shopping-tech-council'.\n" "\n" "\n" "---\n" @@ -423,23 +423,23 @@ def test_write_summary_with_file_statuses(self, mock_open): "* **File:** `src/main.py`\n" " * **Status:** 🟢 SATISFIED\n" " * **Requirements:**\n" - " * **2 approvals** from team 'tech-council'\n" + " * **2 approvals** from team 'shopping-tech-council'\n" " * **Met:** 🟢 Yes (2/2 approved - approved by: alice, bob)\n" "\n" "* **File:** `src/auth.py`\n" " * **Status:** 🔴 UNSATISFIED\n" " * **Requirements:**\n" - " * **2 approvals** from team 'tech-council'\n" + " * **2 approvals** from team 'shopping-tech-council'\n" " * **Met:** 🔴 No (0/2 approved)\n" - " * **Pending:** Needs 2 approvals from team 'tech-council' (1 eligible reviewer assigned). " - "Waiting for 1 more reviewer(s) to be assigned from team 'tech-council'.\n" + " * **Pending:** Needs 2 approvals from team 'shopping-tech-council' (1 eligible reviewer assigned). " + "Waiting for 1 more reviewer(s) to be assigned from team 'shopping-tech-council'.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content) @patch("builtins.open", new_callable=unittest.mock.mock_open) def test_write_summary_with_hierarchical_clearance(self, mock_open): """Test write_summary outputs correct terminology for hierarchical clearance rules.""" - min_team = Team(name="tech-council", level=3) + min_team = Team(name="shopping-tech-council", level=3) req = RuleRequirement(min_approvals=1, min_team=min_team) status = RequirementStatus( requirement=req, @@ -469,11 +469,11 @@ def test_write_summary_with_hierarchical_clearance(self, mock_open): "\n" "Merged requirements across all changed files:\n" "\n" - "* **1 approval** from team 'tech-council' or higher in the UCP governance hierarchy\n" + "* **1 approval** from team 'shopping-tech-council' or higher in the UCP governance hierarchy\n" " * **Met:** 🔴 No (0/1 approved)\n" - " * **Pending:** Needs 1 approval from team 'tech-council' or higher in the UCP governance hierarchy " + " * **Pending:** Needs 1 approval from team 'shopping-tech-council' or higher in the UCP governance hierarchy " "(0 eligible reviewers assigned). Waiting for 1 more reviewer(s) to be assigned from team " - "'tech-council' or higher in the UCP governance hierarchy.\n" + "'shopping-tech-council' or higher in the UCP governance hierarchy.\n" ) + HELP_SUFFIX self.assertEqual(expected, written_content)