Refactor/veto stuff #1075
Refactor/veto stuff #1075
Conversation
WalkthroughRemoves Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration_tests/test_matchmaker_vetoes.py (1)
13-21: Good guard test for map pool coverage and name uniquenessThis test nicely encodes the assumptions the veto tests rely on: that pools 1–4 are non‑empty and that each pool’s maps have unique
folder_names, makingmapnamea reliable identity for veto behavior. If more pools become relevant to veto tests later, consider extending the[1, 2, 3, 4]list or deriving it from the fixtures, but the current explicit list is fine for the existing setup. Based on learnings, this aligns with howfetch_map_poolspopulatesMap/NeroxisGeneratedMapinstances.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/ladder_service/ladder_service.py(1 hunks)server/types.py(0 hunks)tests/integration_tests/test_matchmaker.py(1 hunks)tests/integration_tests/test_matchmaker_vetoes.py(5 hunks)
💤 Files with no reviewable changes (1)
- server/types.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/types.py:46-47
Timestamp: 2025-10-26T14:51:28.358Z
Learning: In server/types.py, the MapPoolMap Protocol intentionally requires map_pool_map_version_id to be non-Optional (int) because maps in a map pool context must have this ID. The concrete implementations (Map and NeroxisGeneratedMap) use Optional[int] to support usage outside of map pools, but when used as MapPoolMap they must be instantiated with a non-None value.
📚 Learning: 2025-10-26T15:11:39.923Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.pytests/integration_tests/test_matchmaker.pyserver/ladder_service/ladder_service.py
📚 Learning: 2025-10-26T14:43:30.226Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/matchmaker/map_pool.py:80-109
Timestamp: 2025-10-26T14:43:30.226Z
Learning: In server/matchmaker/map_pool.py, the choose_map method does not need a guard against all-zero final_weights because the veto system validation logic (including minimum_maps_after_veto constraints) prevents pools from being configured where all maps can be fully banned, ensuring at least some maps always have non-zero weights.
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.py
📚 Learning: 2025-10-26T14:43:05.147Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:257-280
Timestamp: 2025-10-26T14:43:05.147Z
Learning: In server/ladder_service/veto_system.py, the field max_tokens_per_map is guaranteed to be non-negative through external constraints (not enforced at the database schema level in the visible code).
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.py
🧬 Code graph analysis (2)
tests/integration_tests/test_matchmaker_vetoes.py (4)
tests/conftest.py (1)
database(198-200)tests/integration_tests/conftest.py (1)
ladder_service(51-67)server/ladder_service/ladder_service.py (1)
fetch_map_pools(156-220)tests/integration_tests/test_game.py (1)
end_game_as_draw(171-198)
server/ladder_service/ladder_service.py (1)
server/games/game.py (1)
get_player_option(623-627)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: unit-test
🔇 Additional comments (5)
tests/integration_tests/test_matchmaker.py (1)
46-61: Validating launch payload without map_pool_map_version_id looks correctUsing the dict equality (after deleting
mapname) to assert only the expected fields, includingmap_position, is a clean way to ensuremap_pool_map_version_idis gone from thegame_launchmessage while preserving the previous behavior checks.tests/integration_tests/test_matchmaker_vetoes.py (3)
64-64: Switch from version IDs to explicitmapnamechecks is consistentAsserting
msg1["mapname"] == "scmp_015.v0003"in both veto‑related tests keeps the previous intent (verifying the exact chosen map) while no longer depending onmap_pool_map_version_id. This matches howmapnameis derived fromfolder_nameinfetch_map_poolsand exposed viaGameLaunchOptions.Also applies to: 87-87
110-113: Partial veto tests usingmapnameset semantics are soundRecording
chosen_mapnameinto a set and asserting:
- each pick is in
["scmp_002", "scmp_003"], and- the final
chosen_mapsset equals{"scmp_002", "scmp_003"}
preserves the original coverage: both allowed maps must actually be selected over multiple runs, and no unexpected maps slip through. The earlier uniqueness test on pool map names makes thismapname‑level check equivalent to the prior ID‑based assertions.Also applies to: 115-115
140-142: TMM veto behavior check viamapnameis appropriateChecking
msg1["mapname"] == "scmp_002"for the TMM veto scenario keeps the test tightly coupled to the configured pool while avoiding direct use ofmap_pool_map_version_id. This is consistent with the rest of the refactor and the wayGameLaunchOptions.mapnameis populated.server/ladder_service/ladder_service.py (1)
635-643: GameLaunchOptions construction correctly dropsmap_pool_map_version_idBuilding
GameLaunchOptionswithmapname=game_map.folder_nameandmap_positionfrom"StartSpot"while omittingmap_pool_map_version_idmatches the updated public type and the way tests now validategame_launchpayloads. The internalmap_pool_map_version_idonMap/NeroxisGeneratedMapremains available for veto and pool logic, but is no longer leaked to clients, which is consistent with the design intent. Based on learnings, this keeps the MapPoolMap invariants intact while simplifying the launch options.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration_tests/test_matchmaker_vetoes.py (3)
13-27: New uniqueness test clearly encodes the assumptions the veto tests rely onThe checks on pools 1–3 and
folder_nameuniqueness nicely codify the fixture assumptions your other veto tests need. Consider tightening the filter to only include truthy folder names (e.g.,getattr(m, "folder_name", None)andif folder_name) so that a future empty string/Nonedoesn’t count as a “regular” map.
71-71: Mapname-based assertion aligns with removal ofmap_pool_map_version_idSwitching this assertion to
msg1["mapname"] == "scmp_015.v0003"keeps the intent of the test (vetoes forcing a specific map) while matching the updated launch options payload. The only trade-off is continued coupling to the exact test DB map version, which seems acceptable here.If you ever want to reduce brittleness, you could centralize these expected map names into constants tied to the integration test fixture data.
117-119: Partial veto test correctly tracks chosen map names instead of IDsRenaming to
chosen_mapname, checking membership in["scmp_002", "scmp_003"], and asserting the finalchosen_mapsset equals{"scmp_002", "scmp_003"}preserves the original coverage while decoupling from map_pool_map_version_id. This still validates that both remaining maps are reachable under veto constraints.If you want slightly clearer intent, you could inline the expected set as a constant (e.g.,
EXPECTED_MAPS = {"scmp_002", "scmp_003"}) and reuse it in both the membership assertion and the final equality check.Also applies to: 122-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration_tests/test_matchmaker_vetoes.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/types.py:46-47
Timestamp: 2025-10-26T14:51:28.358Z
Learning: In server/types.py, the MapPoolMap Protocol intentionally requires map_pool_map_version_id to be non-Optional (int) because maps in a map pool context must have this ID. The concrete implementations (Map and NeroxisGeneratedMap) use Optional[int] to support usage outside of map pools, but when used as MapPoolMap they must be instantiated with a non-None value.
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/matchmaker/map_pool.py:80-109
Timestamp: 2025-10-26T14:43:30.226Z
Learning: In server/matchmaker/map_pool.py, the choose_map method does not need a guard against all-zero final_weights because the veto system validation logic (including minimum_maps_after_veto constraints) prevents pools from being configured where all maps can be fully banned, ensuring at least some maps always have non-zero weights.
📚 Learning: 2025-10-26T15:11:39.923Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.py
📚 Learning: 2025-10-26T14:43:30.226Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/matchmaker/map_pool.py:80-109
Timestamp: 2025-10-26T14:43:30.226Z
Learning: In server/matchmaker/map_pool.py, the choose_map method does not need a guard against all-zero final_weights because the veto system validation logic (including minimum_maps_after_veto constraints) prevents pools from being configured where all maps can be fully banned, ensuring at least some maps always have non-zero weights.
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.py
📚 Learning: 2025-10-26T14:43:05.147Z
Learnt from: K-ETFreeman
Repo: FAForever/server PR: 1033
File: server/ladder_service/veto_system.py:257-280
Timestamp: 2025-10-26T14:43:05.147Z
Learning: In server/ladder_service/veto_system.py, the field max_tokens_per_map is guaranteed to be non-negative through external constraints (not enforced at the database schema level in the visible code).
Applied to files:
tests/integration_tests/test_matchmaker_vetoes.py
🧬 Code graph analysis (1)
tests/integration_tests/test_matchmaker_vetoes.py (2)
tests/integration_tests/conftest.py (1)
ladder_service(51-67)tests/integration_tests/test_game.py (1)
end_game_as_draw(171-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: unit-test
🔇 Additional comments (2)
tests/integration_tests/test_matchmaker_vetoes.py (2)
94-94: Consistent use ofmapnamein dynamic max-tokens testThis assertion mirrors the previous test’s expectation and correctly uses
mapnameinstead of an internal ID, keeping the dynamic max-tokens scenario behaviorally identical after the refactor.
148-148: TMM veto scenario now asserts on mapname, matching new launch options contractThe change to assert
msg1["mapname"] == "scmp_002"keeps the TMM veto behavior check intact and consistent with the removal ofmap_pool_map_version_idfromGameLaunchOptions.
| for queue in ladder_service.queues.values(): | ||
| for mq_map_pool in queue.map_pools.values(): | ||
| pool_id = mq_map_pool.map_pool.id | ||
| if pool_id in [1, 2, 3]: |
There was a problem hiding this comment.
on your first attempt there were 4 pool ids, and tests failed. so you reduced their amount to 3. why? wasn't the failure legitimate?
There was a problem hiding this comment.
error was not due to that
pool 4 just is not used in any veto tests so it doesnt care about its content
Removed map_pool_map_version_id from GameLaunchOptions and fixed the tests so they use map name instead
added additional test which checks if map names are unique since its not guaranteed but required for veto system tests
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.