Feature/#1032 working veto system#1033
Conversation
| if (tokensTotalPerPlayer[player.id] > veto_tokens_per_player): | ||
| raise RuntimeError(f"Player {player.id} has too many vetoes!") |
There was a problem hiding this comment.
We should not do validation of the number of tokens per player when starting the game. This validation and error raising should happen when the server receives the set_vetoes message. This is because as it is now a player could send the server more than the max amount of vetoes and then cause a game to never be able to launch.
| def calculate_dynamic_tokens_per_map(self, M: float, tokens: list[int]) -> float: | ||
| sorted_tokens = sorted(tokens) | ||
| if (sorted_tokens.count(0) >= M): | ||
| return 1 | ||
|
|
||
| result = 1; last = 0; index = 0 | ||
| while (index < len(sorted_tokens)): | ||
| (index, last) = next(((i, el) for i, el in enumerate(sorted_tokens) if el > last), (len(sorted_tokens) - 1, sorted_tokens[-1])) | ||
| index += 1 | ||
| divider = index - M | ||
| if (divider <= 0): | ||
| continue | ||
|
|
||
| result = sum(sorted_tokens[:index]) / divider | ||
| upperLimit = sorted_tokens[index] if index < len(sorted_tokens) else float('inf') | ||
| if (result <= upperLimit): | ||
| return result | ||
|
|
||
| raise Exception("Failed to calculate dynamic tokens per map: impossible vetoes setup") |
There was a problem hiding this comment.
Similarly here we are throwing an exception based on user input so a user could craft a veto selection that could cause the majority of games to fail to start. It would be better to have some default value rather than throwing an exception.
|
also idk how least_common should work |
Askaholic
force-pushed
the
develop
branch
3 times, most recently
from
75a18d1 to
0a667d2
Compare
There was a problem hiding this comment.
I only wanted to ask what calculate_dynamic_tokens_per_map function is doing, because i don't fully comprehend it from the code, but also left some other comments/suggestions.
I am not a maintainer of this repo, therefore feel free to ignore them or use them at your own risk
| if player not in connected_players | ||
| ]) | ||
|
|
||
| def calculate_dynamic_tokens_per_map(self, M: float, tokens: list[int]) -> float: |
There was a problem hiding this comment.
are you trying to achieve sum(weights) = M?
There was a problem hiding this comment.
calculate_dynamic_tokens_per_map finds the minimal possible max_tokens_per_map while still respecting M
There was a problem hiding this comment.
but when do you decide that max_tokens_per_map can't be reduced anymore?
There was a problem hiding this comment.
Its just some math
it solves series of equations, if vetoes set correctly, one of them is guaranteed to be correct answer
if vetoes set incorrectly, it will return 0
There was a problem hiding this comment.
i'll prepare some explanation how math works
| map_pool: MapPool, | ||
| min_rating: Optional[int], | ||
| max_rating: Optional[int] | ||
| max_rating: Optional[int], |
There was a problem hiding this comment.
why are all of those arguments not inside MapPool 🤔 ?
There was a problem hiding this comment.
because its not in the mapPool in the database, but in matchmaker_queue_map_pool (queue bracket), so should be consistent between repos
and yea its that way in db due to some reasons which were discussed long ago (f.e. u can use same pool for multiple brackets)
There was a problem hiding this comment.
server/matchmaker/map_pool.py
Outdated
| map_id = random.choices(least_common, weights=weights, k=1)[0][0] | ||
| return self.maps[map_id].get_map() | ||
| # Multiply weight by 2 if map is least common and not vetoed by anyone | ||
| mapList = list((map.map_pool_map_version_id, map, 2 if (map.id in least_common_ids) and (vetoesMap.get(map.map_pool_map_version_id, 0) == 0) else 1) for id, map in self.maps.items()) |
There was a problem hiding this comment.
don't you feel like this line is a bit long?)
There was a problem hiding this comment.
long lines for long PP
|
i'll add some improvements soon |
|
ok not soon but tomorrow ;) |
| 4) otherwise | ||
| solving equation for current group | ||
| and checking the result vs upper border | ||
| and upper border is equal to the amount of tokens applied to the map next to last map in our group, or infinity if there is no such one |
There was a problem hiding this comment.
Maybe the explanation could be shorter
Lets denote max_tokens_per_map = len(tokens) =
If some
Given sorted array, excluding the last element
This excluding won't help if
Lets denote
But
This means, that we can stop trying to decrease
And you can describte the algorithm as easy as:
- Find
$d = \frac{\sum_{i=1}^{n}a_{i}}{n - M}$ , if$d > \max(tokens)$ then$d$ is found. otherwise exclude all$a_{i} \geq d$ . Repeat while$n > M$
(And of course handle special case when
| solving equation for current group | ||
| and checking the result vs upper border | ||
| and upper border is equal to the amount of tokens applied to the map next to last map in our group, or infinity if there is no such one | ||
| """ |
There was a problem hiding this comment.
maybe put docstring inside the function definition?
| minimum_maps_after_veto = 1 | ||
| self._logger.error(f"Wrong vetoes setup detected for pool {pool.id} in queue {queue.id}") | ||
| result.append( | ||
| MatchmakerQueueMapPoolVetoData( |
There was a problem hiding this comment.
I still don't really understand why you need MatchmakerQueueMapPoolVetoData. Why don't you just initialize MatchmakerQueueMapPool with correct vetoes setup and use it?
There was a problem hiding this comment.
atleast to avoid having any possible issues with queue.map_pools.clear() in update_data
theoretically user can change vetoes mid-queues-update, then server will delete all user's vetoes and he will need to apply all of them again which is very bad
There was a problem hiding this comment.
Askaholic said "Technically we do run the matchmaking algorithm in a separate thread because it is the one piece of the code that has the potential to block the rest of the server if there are a lot of players in queue."
So i prefer to have robust solution here
There was a problem hiding this comment.
I don't think that statement is relevant to vetos because the 'chose_map' stage happens after the matchmaking algorithm has finished. But you should probably set it up in such a way that vetos are locked in the moment a match is found like we do with factions in the PlayerParty.on_matched function. I think keeping the behavior for choosing vetos and factions consistent makes the most sense.
There was a problem hiding this comment.
I don't think that statement is relevant to vetos because the 'chose_map' stage happens after the matchmaking algorithm has finished. But you should probably set it up in such a way that vetos are locked in the moment a match is found like we do with factions in the
PlayerParty.on_matchedfunction. I think keeping the behavior for choosing vetos and factions consistent makes the most sense.
its not about chose_map, it about command_set_player_vetoes in lobbyconection.py
it uses matchmaker map pool data and this is async function, and i am not 100% sure how this works here
also whats the point of locking vetoes when match found? what can possibly go wrong if we dont do that?
There was a problem hiding this comment.
also whats the point of locking vetoes when match found?
As I said:
keeping the behavior for choosing vetos and factions consistent makes the most sense.
This will be less confusing to players.
server/matchmaker/map_pool.py
Outdated
| vetoes_map = {} | ||
|
|
||
| # Filter and count played map IDs | ||
| repetition_counts = Counter(id_ for id_ in played_map_ids if id_ in self.maps) |
There was a problem hiding this comment.
From what I can tell, the number of repetitions is now completely ignored? That means if we're in an 8 player game, and 7 players just played map A while 1 player just played map B, both are treated equally as 'repeated maps' whereas previously map A would have been guaranteed to not be picked due to the higher play count.
There was a problem hiding this comment.
Yes, number of repetitions is completely ignored in the code
OK, i know how it can be not-ignored, will modify it
There was a problem hiding this comment.
From what I can tell, the number of repetitions is now completely ignored? That means if we're in an 8 player game, and 7 players just played map A while 1 player just played map B, both are treated equally as 'repeated maps' whereas previously map A would have been guaranteed to not be picked due to the higher play count.
So, i just finished to modyfying it, and now remember why i didnt do it in a first place:
Current idea is that if some map is banned a lot more than the repeated map (weight is less than repeated one multiplied by LADDER_ANTI_REPETITION_WEIGHT_THRESHOLD), then the weight transfer doesnt happen
However, if we chain transfer from more-repeated-ones to less-repeated ones, it causes an issue: now bad map can affect good map weight, if there is some a little-banned map in between of them, and repeated one
[note: "weight" later is veto-system-weight, not map inner weight]
For example, the theshold is 0.75, so map with two repeats and weight 0.6 should not be abile to transfer weights to map with weight 1, because 1 * 0.75 = 0.75 > 0.6. But if there is middle map with repeat 1 and 0.8 weight, then weight is transferred to 0.8 weight map first, and then to map with weight 1
But if to think about it, this makes sense, kinda: the more repeats the map have, the stronger we want it to tranfer weight
so i adjusted the formula: now we multiply not by threshold, but my Math.pow(theshold, repetition_diff )
So now its not a bug, but a feature, the problem is solved
I made a lot of tests also to help to understand, what-the-hell-is-going-on here
UPD: modified the function again to resolve all issues and make it more flexible
Askaholic
left a comment
Askaholic
left a comment
There was a problem hiding this comment.
I'm having a very hard time reviewing this PR because it seems that at just about every step of the way there is some complicated function with 4 or 5 levels of indentation that does some sort of magic for an operation that I wouldn't have expected to be so complicated.
I think it would help a lot to have some unit tests, as tests are a great way of providing 'black box' examples of expected input/output. That might help me understand what is even going on and then make it easier to work backwards to figure out how the code is arriving at those values.
Another thing I'm always a fan of is making use of vertical whitespace to logically group operations together. At the moment there are a lot of if statements, for loops, etc. all crammed up against each other which is also contributing to the difficulty of reading the code.
If I was able to wrap my head around what was actually going on better I'd also be able to suggest more concrete ways of refactoring the code to make it easier to read. I believe there is a lot of 'extract to function' refactoring that could happen to clean things up and make it easier to see what the overarching algorithms are doing, but without a clear picture of the code I can't suggest specific refactors to do.
So yea, I think the most important next step is to start writing some unit tests as I think this will make it easier to review.
server/players.py
Outdated
| BracketID = int | ||
| MapPoolMapVersionId = int | ||
| VetoTokensApplied = int | ||
| PlayerVetoes = dict[BracketID, dict[MapPoolMapVersionId, VetoTokensApplied]] |
There was a problem hiding this comment.
It might help with readability if you were to make PlayerVetos a real class so that you can add helper functions to it.
For instance a get_weight function that does this: max(0, 1 - vetoes_map.get(id_, 0) / max_tokens_per_map).
Or also the update_vetos function currently living on the player object.
There was a problem hiding this comment.
I think PlayerRatings would be a good example to look at.
Askaholic
force-pushed
the
feature/#1032-working-veto-system
branch
from
f940af8 to
2b91d6b
Compare
| "vetoes": [ | ||
| { | ||
| "map_pool_map_version_id": map_id, | ||
| "veto_tokens_applied": tokens, | ||
| "matchmaker_queue_map_pool_id": bracket_id | ||
| } | ||
| for bracket_id, vetoes in self._vetoes.items() | ||
| for map_id, tokens in vetoes.items() | ||
| ] | ||
| } |
There was a problem hiding this comment.
Here's an idea. Instead of using the surrogate keys for map_pool_map_version_id and matchmaker_queue_map_pool_id which really only exist for database optimization, how about we use composite keys instead. It makes no difference to us on the python side and I think it would simplify things quite a bit, since you wouldn't have to keep track of what the difference between all these different ids is.
So instead of map_pool_map_version_id we would just use the map_pool_id and the map_version_id together.
And instead of matchmaker_queue_map_pool_id we would use queue_name (the technical name would be used in the protocol messages, but it doesn't matter if we use queue_id or queue_name internally) and map_pool_id.
Immediately I notice that map_pool_id is shared between both of those keys which is a nice win. The message would look something like this:
{
"vetoes": [
{
"queue_name": "ladder1v1",
"map_pool_id": 123,
"map_version_id": 123456,
"tokens": 4,
},
...
]
}But some of the redundancy could be reduced more if we use a nested dict rather than a list:
{
"vetoes": {
"ladder1v1": {
"123": {
"123456": 4,
...
}
}
},
}Of course that loses the expressiveness, so maybe some balance between the two:
{
"vetoes": {
"ladder1v1": [
{
"map_pool_id": 123,
"map_version_id": 123456,
"tokens": 4,
},
...
]
},
}There was a problem hiding this comment.
Do as you see, my lord, i will adjust the client / commons / etc to whatever server will be in the end
I also noticed all this boilerplate with multiple ids, was thinking like making mapPoolMap actual class instead of just a protocol, so mapPoolMap has id (which currently is map_pool_map_version_id), and map inside of it has own id, so we dont have two ids in the same object
On top (1) its dynamic tokens per map case. Dynamic means it can shrink any positive amount of tokens to any positive value > 0. On the bottom (2) we are checking how many maps can be banned (by all players in total) vs how many maps max can be banned (as mm team prefers). It they are equal, that means players ban maximum amount of maps, but this is still OK |
|
@ Gatsik since askaholic left, may you review this PR? (note: the client for the vetos still required to be polished, so approving is ok, merging is not, too soon) |
WalkthroughA veto system for ladder matchmaking was added: new VetoService and PlayerVetoes, DB/schema additions for queue map pools, veto-aware initial map-weighting and anti-repetition adjustments, LobbyConnection and Player API extensions, types expanded for map-version tracking, and tests updated to exercise veto flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LobbyConn as LobbyConnection
participant VetoSvc as VetoService
participant PlayerSvc as PlayerService
Client->>LobbyConn: set_player_vetoes(msg)
LobbyConn->>VetoSvc: set_player_vetoes(player, vetoes)
VetoSvc->>VetoSvc: validate & adjust vetoes
alt valid
VetoSvc->>PlayerSvc: persist vetoes
VetoSvc-->>Client: vetoes_info (updated, forced? / silent?)
else invalid
VetoSvc-->>Client: error (ClientError)
end
sequenceDiagram
participant Ladder as LadderService
participant Queue as MatchmakerQueue
participant Veto as VetoService
participant Pool as MapPool
Ladder->>Queue: prepare match / select queue_map_pool
Queue->>Veto: generate_initial_weights_for_match(players, queue_map_pool)
Veto-->>Queue: initial_weights (per-map)
Queue->>Pool: choose_map(played_ids, initial_weights)
Pool->>Pool: apply_antirepetition_adjustment(initial_weights, played_ids,...)
Pool-->>Queue: chosen map (with map_pool_map_version_id)
Queue-->>Ladder: match ready / launch options include map_pool_map_version_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/types.py (1)
129-149: map_pool_map_version_id not propagated in NeroxisGeneratedMap.get_map().The
get_map()method creates aMapinstance but doesn't passself.map_pool_map_version_idto it, breaking the linkage chain. This will cause veto tracking to fail for generated maps.Apply this diff:
return Map( id=self.id, folder_name=folder_name, ranked=True, weight=self.weight, + map_pool_map_version_id=self.map_pool_map_version_id, )
♻️ Duplicate comments (1)
server/matchmaker/map_pool.py (1)
108-108: Silence Ruff S311 explicitly (non-crypto RNG is intentional here)Bandit is already silenced; Ruff still flags S311. Add noqa.
Covered in the previous diff by adding “# noqa: S311”.
🧹 Nitpick comments (17)
server/db/models.py (1)
298-306: Consider documenting the Float type choice forminimum_maps_after_veto.The
minimum_maps_after_vetocolumn usesFloatrather thanInteger, which is unusual for counting maps. This appears intentional based on usage in the veto system (dynamic token calculations can result in fractional thresholds), but the rationale isn't immediately clear from the schema alone.Consider adding a comment in the schema or documentation explaining why Float is used here, or confirm whether Integer would be more appropriate if the fractional values aren't actually needed.
server/ladder_service/veto_system.py (1)
170-209: Consider extracting veto weight calculation logic.The
generate_initial_weights_for_matchmethod mixes multiple concerns: unpacking the matchmaker config tuple, aggregating vetoes from players, handling dynamic token calculation, error logging, and computing final weights. This makes the function harder to test and understand.Consider extracting helper methods:
def _aggregate_player_vetoes(self, players, pool_id, maps) -> dict[int, int]: """Aggregate veto tokens applied to each map by all players.""" vetoes_map = defaultdict(int) for m in maps: for player in players: bracket_vetoes = player.vetoes.get_vetoes_for_bracket(pool_id) vetoes_map[m.map_pool_map_version_id] += bracket_vetoes.get( m.map_pool_map_version_id, 0 ) return vetoes_map def _compute_map_weights(self, maps, vetoes_map, max_tokens_per_map) -> dict[int, float]: """Convert veto counts to map selection weights.""" return { m.map_pool_map_version_id: max( 0, 1 - vetoes_map.get(m.map_pool_map_version_id, 0) / max_tokens_per_map, ) for m in maps }Then simplify the main method to orchestrate these steps.
server/config.py (1)
140-142: Anti‑repetition knobs: confirm intended semantics and ordering.
- repeat_factor=0.8 contradicts doc that “bigger → stronger”; verify intended <1 behavior or adjust doc.
- Threshold order matters; ensure they’re sorted at use site to avoid surprises.
If you want deterministic behavior, normalize at use:
- adjusted_weights = self.apply_antirepetition_adjustment( - initial_weights, played_map_pool_map_version_ids, config.LADDER_ANTI_REPETITION_WEIGHT_BASE_THRESHOLDS, config.LADDER_ANTI_REPETITION_REPEAT_COUNTS_FACTOR - ) + base = sorted(config.LADDER_ANTI_REPETITION_WEIGHT_BASE_THRESHOLDS, reverse=True) + adjusted_weights = self.apply_antirepetition_adjustment( + initial_weights, played_map_pool_map_version_ids, base, config.LADDER_ANTI_REPETITION_REPEAT_COUNTS_FACTOR + )server/__init__.py (1)
129-130: Ensure service registration; minor all nit.
- Verify create_services registers a "veto_service" key; otherwise connection_factory will KeyError at runtime.
- Deduplicate "RatingService" in all.
Minimal fix for the nit:
__all__ = ( @@ - "RatingService", - "RatingService", + "RatingService", "ServerInstance", "VetoService",Also applies to: 161-162, 200-211
tests/unit_tests/test_veto_system.py (2)
12-122: Clarify spec for “minimum_maps_after_veto == pool size” (TODO).Current test asserts invalid when equal and max_tokens_per_map==0; confirm that’s intended (especially when veto_tokens_per_player==0). If equality should be allowed, flip expectation and adjust _is_valid_veto_config_for_queue accordingly.
162-229: Great coverage; add monotonicity properties.Consider property tests:
- If M increases, returned T should be non‑decreasing.
- Permuting tokens doesn’t change T (already checked once).
- Scaling all tokens by k>1 should not decrease T.
I can add Hypothesis tests if useful.
tests/integration_tests/test_game.py (1)
171-199: Draw helper OK; confirm army indexing assumption.Assumes armies are 1..len(protos). If that ever differs, derive armies from game_launch payloads.
tests/unit_tests/test_map_pool.py (2)
104-116: Avoid None keys in weight maps by setting map_pool_map_version_id for generated mapapply_antirepetition_adjustment uses map_pool_map_version_id as weight keys. Leaving it as None is tolerated but weakens typing and can mask issues. Set it explicitly for consistency.
Apply:
- generated_map = NeroxisGeneratedMap.of({ + generated_map = NeroxisGeneratedMap.of({ "version": "0.0.0", "spawns": 2, "size": 512, "type": "neroxis" - }) + }, map_pool_map_version_id=4)
142-164: Test ties to config values; consider decoupling for stabilityThis test depends on current LADDER_ANTI_REPETITION_* config to deterministically favor map 1. A config tweak can break it. Patch config within the test or pass explicit initial_weights into choose_map to keep assertions stable.
Would you like a quick patch that monkeypatches config thresholds/factor inside this test?
tests/unit_tests/conftest.py (1)
55-59: Shutdown order: prefer strict reverse-init sequenceMinor: shut services down in exact reverse of initialization to avoid future coupling surprises (e.g., tasks referencing still-running services).
Suggested order: ladder_service → veto_service → violation_service → game_service → player_service.
tests/unit_tests/test_ladder_service.py (2)
873-887: Optional: include map_pool_map_version_id in Map constructors for clarityEven with choose_map mocked, passing map_pool_map_version_id improves consistency with the new data model and avoids future surprises if mocks are removed.
Would you like me to update these constructors in-place?
Also applies to: 889-904, 929-943, 945-959, 989-1003, 1005-1019
71-75: Use NamedTuple attribute access instead of tuple indexingIndexing into
MatchmakerQueueMapPoolwith[1]is brittle. The second field ismap_pool, so prefer.map_poolfor clarity and future field-order safety.Apply across all instances:
- assert list(queue.map_pools[1][1].maps.values()) == [ + assert list(queue.map_pools[1].map_pool.maps.values()) == [ - assert list(queue.map_pools[2][1].maps.values()) == [ + assert list(queue.map_pools[2].map_pool.maps.values()) == [ - assert list(queue.map_pools[3][1].maps.values()) == [ + assert list(queue.map_pools[3].map_pool.maps.values()) == [ - assert list(queue.map_pools[4][1].maps.values()) == [ + assert list(queue.map_pools[4].map_pool.maps.values()) == [Also applies to: lines 76-82, 83-87, 92-105
tests/integration_tests/test_matchmaker_vetoes.py (1)
41-58: Optional: reduce iteration counts to speed up CIRunning 20 rounds per scenario can be slow/flaky under load. Consider lowering the count and tightening assertions (or seed the RNG) to keep tests fast and stable.
Also applies to: 65-81, 110-137
server/matchmaker/map_pool.py (2)
29-39: Type hints should accept Optional[int] keysinitial_weights/played_map_ids can include None (e.g., generator maps). Widen annotations to reflect reality and silence type-checkers.
Apply:
-from typing import ClassVar, Iterable, NamedTuple, Optional +from typing import ClassVar, Iterable, Mapping, NamedTuple, Optional- def apply_antirepetition_adjustment(self, initial_weights: dict[int, float], played_map_ids: Iterable[int], base_thresholds: list[float], repeat_factor: float) -> dict[int, float]: + def apply_antirepetition_adjustment( + self, + initial_weights: Mapping[Optional[int], float], + played_map_ids: Iterable[Optional[int]], + base_thresholds: list[float], + repeat_factor: float, + ) -> dict[Optional[int], float]:- def choose_map(self, played_map_ids: Iterable[int] = (), initial_weights: Optional[dict[int, float]] = None) -> Map: + def choose_map(self, played_map_ids: Iterable[int] = (), initial_weights: Optional[Mapping[Optional[int], float]] = None) -> Map:
40-55: Normalize thresholds to avoid order/duplication sensitivityEnsure thresholds are sorted and deduplicated after extension; this stabilizes selection behavior.
Apply:
def get_notrepeated_weight_transfer_targets(current_weight, rep_count): thresholds = list(base_thresholds) factor = repeat_factor ** (rep_count - 1) if factor < 1: - thresholds.extend(t * factor for t in base_thresholds if t * factor < base_thresholds[-1]) + thresholds.extend(t * factor for t in base_thresholds) + thresholds = sorted(set(thresholds))server/matchmaker/matchmaker_queue.py (1)
90-90: Consider using*_for cleaner unpacking.The unpacking pattern works but can be simplified per the Gatsik suggestion in past comments.
As per coding guidelines.
Apply this diff:
- for _, map_pool, min_rating, max_rating, *_, in self.map_pools.values(): + for _, map_pool, min_rating, max_rating, *_ in self.map_pools.values():Note: The trailing comma after
*_is redundant.server/ladder_service/ladder_service.py (1)
574-574: Optional: Consider refactoring exception message per static analysis.Ruff suggests abstracting the raise statement and avoiding long messages in the exception call. This is a minor style improvement.
Apply this diff if desired:
+ def _no_map_pool_error(): + return RuntimeError(f"No map pool available for rating {rating}!") + map_pool = queue.get_map_pool_for_rating(rating) if not map_pool: - raise RuntimeError(f"No map pool available for rating {rating}!") + raise _no_map_pool_error()Alternatively, define a custom exception class if this error is expected to be caught and handled specifically elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
server/__init__.py(3 hunks)server/config.py(1 hunks)server/db/models.py(1 hunks)server/ladder_service/ladder_service.py(11 hunks)server/ladder_service/veto_system.py(1 hunks)server/lobbyconnection.py(4 hunks)server/matchmaker/__init__.py(1 hunks)server/matchmaker/map_pool.py(2 hunks)server/matchmaker/matchmaker_queue.py(4 hunks)server/players.py(3 hunks)server/types.py(5 hunks)tests/data/test-data.sql(3 hunks)tests/integration_tests/conftest.py(5 hunks)tests/integration_tests/test_game.py(1 hunks)tests/integration_tests/test_matchmaker.py(2 hunks)tests/integration_tests/test_matchmaker_vetoes.py(1 hunks)tests/integration_tests/test_server_instance.py(2 hunks)tests/integration_tests/test_servercontext.py(1 hunks)tests/unit_tests/conftest.py(4 hunks)tests/unit_tests/test_ladder_service.py(9 hunks)tests/unit_tests/test_lobbyconnection.py(2 hunks)tests/unit_tests/test_map_pool.py(7 hunks)tests/unit_tests/test_matchmaker_queue.py(6 hunks)tests/unit_tests/test_veto_system.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
tests/integration_tests/test_server_instance.py (3)
tests/integration_tests/conftest.py (2)
veto_service(79-83)oauth_service(117-127)tests/unit_tests/conftest.py (1)
veto_service(93-97)tests/conftest.py (1)
oauth_service(422-423)
tests/integration_tests/test_servercontext.py (2)
tests/integration_tests/conftest.py (1)
veto_service(79-83)tests/unit_tests/conftest.py (1)
veto_service(93-97)
tests/unit_tests/test_veto_system.py (4)
tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (3)
_cap_tokens(323-335)_is_valid_veto_config_for_queue(257-280)calculate_dynamic_tokens_per_map(211-254)server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)tests/conftest.py (1)
queue_factory(373-395)
tests/integration_tests/test_game.py (3)
tests/integration_tests/conftest.py (4)
send_message(630-631)lobby_server(310-311)connect_and_sign_in(559-569)read_until_command(532-545)e2e_tests/websocket_protocol.py (1)
send_message(41-48)e2e_tests/fafclient.py (2)
send_message(44-49)read_until_command(61-67)
tests/unit_tests/test_map_pool.py (2)
server/types.py (6)
Map(55-73)map_pool_map_version_id(47-47)weight(50-50)NeroxisGeneratedMap(79-149)of(93-117)id(45-45)server/matchmaker/map_pool.py (2)
choose_map(80-108)apply_antirepetition_adjustment(29-78)
tests/integration_tests/conftest.py (4)
server/ladder_service/veto_system.py (1)
VetoService(41-254)tests/unit_tests/conftest.py (3)
ladder_service(65-81)violation_service(85-89)veto_service(93-97)tests/conftest.py (3)
database(198-200)game_service(346-361)player_service(307-310)server/ladder_service/ladder_service.py (2)
LadderService(71-795)initialize(96-98)
tests/unit_tests/test_matchmaker_queue.py (2)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (1)
add_map_pool(83-87)
tests/integration_tests/test_matchmaker_vetoes.py (7)
server/players.py (1)
PlayerState(24-31)tests/utils/event_loop.py (1)
fast_forward(52-69)tests/integration_tests/conftest.py (3)
connect_and_sign_in(559-569)lobby_server(310-311)get(410-414)tests/integration_tests/test_game.py (4)
client_response(124-127)end_game_as_draw(171-198)gen_vetoes(201-209)queue_player_for_matchmaking(212-226)tests/conftest.py (2)
player_service(307-310)database(198-200)server/player_service.py (1)
get_player(189-190)server/ladder_service/ladder_service.py (1)
update_data(100-154)
tests/unit_tests/conftest.py (5)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)violation_service(71-75)veto_service(79-83)server/ladder_service/veto_system.py (1)
VetoService(41-254)tests/conftest.py (6)
player_service(307-310)database(198-200)game_service(346-361)game_stats_service(427-428)rating_service(326-332)message_queue_service(336-342)server/player_service.py (2)
PlayerService(41-252)initialize(52-56)server/ladder_service/ladder_service.py (2)
LadderService(71-795)initialize(96-98)
server/matchmaker/map_pool.py (1)
server/types.py (7)
weight(50-50)Map(55-73)id(45-45)map_pool_map_version_id(47-47)get_map(52-52)get_map(72-73)get_map(129-149)
server/ladder_service/veto_system.py (6)
server/decorators.py (1)
with_logger(15-29)server/exceptions.py (1)
ClientError(10-21)server/matchmaker/map_pool.py (1)
MatchmakerQueueMapPool(114-121)server/player_service.py (2)
PlayerService(41-252)all_players(72-73)server/players.py (4)
Player(35-185)to_dict(145-175)write_message(133-143)send_message(121-131)server/types.py (3)
MatchmakerQueueMapPoolVetoData(35-40)id(45-45)map_pool_map_version_id(47-47)
server/matchmaker/matchmaker_queue.py (1)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)
server/lobbyconnection.py (2)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)veto_service(79-83)get(410-414)server/ladder_service/veto_system.py (2)
VetoService(41-254)set_player_vetoes(128-146)
tests/unit_tests/test_ladder_service.py (6)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (2)
MatchmakerQueue(46-304)add_map_pool(83-87)tests/conftest.py (2)
database(198-200)game_service(346-361)tests/integration_tests/conftest.py (3)
violation_service(71-75)veto_service(79-83)ladder_service(51-67)server/ladder_service/ladder_service.py (1)
LadderService(71-795)server/types.py (5)
Map(55-73)map_pool_map_version_id(47-47)NeroxisGeneratedMap(79-149)of(93-117)id(45-45)
server/players.py (3)
server/decorators.py (1)
with_logger(15-29)server/ladder_service/veto_system.py (1)
PlayerVetoes(19-37)server/factions.py (1)
Faction(10-30)
server/ladder_service/ladder_service.py (7)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)violation_service(71-75)veto_service(79-83)tests/unit_tests/conftest.py (3)
ladder_service(65-81)violation_service(85-89)veto_service(93-97)server/ladder_service/veto_system.py (3)
VetoService(41-254)update_pools_veto_config(49-90)generate_initial_weights_for_match(170-209)server/matchmaker/map_pool.py (3)
MapPool(13-111)MatchmakerQueueMapPool(114-121)choose_map(80-108)server/matchmaker/matchmaker_queue.py (2)
MatchmakerQueue(46-304)get_map_pool_for_rating(89-97)tests/conftest.py (4)
players(298-303)database(198-200)game_service(346-361)game(209-210)server/types.py (8)
GameLaunchOptions(23-32)Map(55-73)MapPoolMap(43-52)NeroxisGeneratedMap(79-149)id(45-45)map_pool_map_version_id(47-47)of(93-117)weight(50-50)
server/__init__.py (3)
tests/integration_tests/conftest.py (2)
ladder_service(51-67)veto_service(79-83)tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (1)
VetoService(41-254)
tests/unit_tests/test_lobbyconnection.py (3)
tests/integration_tests/conftest.py (2)
ladder_service(51-67)veto_service(79-83)tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (1)
VetoService(41-254)
server/matchmaker/__init__.py (2)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (1)
MatchmakerQueue(46-304)
🪛 Ruff (0.14.1)
tests/integration_tests/test_game.py
173-173: Avoid specifying long messages outside the exception class
(TRY003)
server/matchmaker/map_pool.py
86-86: Avoid specifying long messages outside the exception class
(TRY003)
108-108: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
server/ladder_service/veto_system.py
135-135: Avoid specifying long messages outside the exception class
(TRY003)
server/ladder_service/ladder_service.py
574-574: Abstract raise to an inner function
(TRY301)
574-574: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (27)
tests/data/test-data.sql (1)
317-324: LGTM! Test data covers multiple veto configuration scenarios.The test data includes varied veto configurations:
- Different token counts per player (0, 1, 2)
- Different max_tokens_per_map (0, 1, 2)
- Consistent minimum_maps_after_veto (1.0)
This provides good coverage for testing the veto system with different constraints.
server/ladder_service/veto_system.py (2)
128-147: Address TODOs before merging or document the decision.The TODO comments at lines 69 and 152-156 indicate uncertainty about the design of force-adjusting player vetoes. From the PR discussion, there's already feedback suggesting this approach should be reconsidered in favor of validation-only with client-side error handling.
These design decisions should be resolved before merging:
- Should the server auto-adjust invalid vetoes, or return validation errors?
- If auto-adjustment is kept, what's the migration path to validation-only?
Based on the PR comments and past review feedback, the preference seems to be moving toward validation-only. Consider filing a follow-up issue to track this technical debt if you plan to merge with auto-adjustment as an interim solution.
211-254: Algorithm correctness verified through comprehensive test coverage.The codebase already contains thorough unit tests for
calculate_dynamic_tokens_per_mapintests/unit_tests/test_veto_system.py. All requested edge cases are covered:
- All maps with zero tokens (should return 1): Test case
(2.0, [0, 0, 0], 1.0)at line 164- Impossible configurations (should return 0): Test case
(1, [1], 0)at line 168- Fractional M values: Multiple cases including
(1.5, ...),(2.5, ...),(1.9, ...), and(4.5, ...)- Large token disparities between maps: Cases like
(2.5, [0, 0, 2], 4.0)and(1.5, [0, 2], 4.0)The test suite includes post-calculation verification that confirms the result produces a sum ≥ M with floating-point tolerance (1e-9 relative precision), and order-independence is verified with duplicate test cases using different input orderings.
tests/integration_tests/test_matchmaker.py (1)
48-64: LGTM! Test correctly validatesmap_pool_map_version_idpropagation.The test now captures the runtime
map_pool_map_version_idvalue from the game_launch message, validates it's in the expected range, and includes it in the assertion. This properly tests the new field without hardcoding assumptions about which specific map will be selected.server/matchmaker/__init__.py (1)
7-20: LGTM! Public API correctly exports new type.The addition of
MatchmakerQueueMapPoolto the package exports is appropriate and follows the existing pattern. This makes the type available for external use without requiring knowledge of the internal module structure.server/players.py (1)
59-61: Lazy import avoids circular dependency.The import of
PlayerVetoesinside__init__prevents circular dependency issues between server/players.py and server/ladder_service/veto_system.py. This is a reasonable pattern for this scenario.Note: Per past review comments, there was discussion about whether veto management should live on the Player object or in a separate service (similar to PartyService). The current implementation keeps it simple by adding a
vetoesattribute, which is acceptable for now, but consider the service-based approach if veto management becomes more complex.tests/integration_tests/test_server_instance.py (1)
24-56: LGTM! Test properly wires veto_service into server instance.The veto_service is correctly added as a fixture parameter and passed through to the ServerInstance's _override_services. This follows the existing pattern for other services like oauth_service.
server/lobbyconnection.py (1)
1392-1406: Command handler correctly processes veto updates.The
command_set_player_vetoeshandler appropriately:
- Validates player is authenticated (assert)
- Transforms flat message list into nested dict structure
- Delegates validation and storage to VetoService
The nested dict construction is straightforward and handles the bracket → map → tokens hierarchy correctly.
Minor note: The
assert self.player is not Nonewill be automatically handled by the authentication check inensure_authenticated, so this is more of a type-checking hint than a runtime check.tests/integration_tests/test_servercontext.py (1)
52-63: LobbyConnection wiring LGTM.New veto_service param is correctly injected for tests.
tests/unit_tests/test_lobbyconnection.py (1)
18-19: Ctor update LGTM.VetoService added as a dependency and autospecced correctly.
Also applies to: 98-102
tests/unit_tests/test_veto_system.py (1)
125-160: _cap_tokens tests LGTM.Edge cases (zero remaining, zero max_per_map, over‑requests) are covered.
tests/unit_tests/test_matchmaker_queue.py (1)
294-301: Wrapper migration LGTM.Switch to MatchmakerQueueMapPool matches updated API; bounds behavior preserved.
Also applies to: 310-317, 329-336, 349-355, 377-391
tests/integration_tests/test_game.py (2)
201-209: Veto payload helper LGTM.Structure matches set_player_vetoes input.
212-226: Wait for server ack before starting search to prevent race condition.After sending
set_player_vetoes, add a wait forvetoes_infoto ensure the server has processed and normalized the vetoes before initiating matchmaking.async def queue_player_for_matchmaking(user, lobby_server, queue_name="ladder1v1", vetoes=None): if vetoes is None: vetoes = [] player_id, _, proto = await connect_and_sign_in(user, lobby_server) await read_until_command(proto, "game_info") if vetoes: await proto.send_message({ "command": "set_player_vetoes", "vetoes": vetoes }) + await read_until_command(proto, "vetoes_info", timeout=10) await start_search(proto, queue_name)tests/unit_tests/conftest.py (1)
92-98: LGTM: VetoService fixture lifecycleInitialization and teardown look correct and consistent with LadderService usage.
server/types.py (2)
32-32: LGTM: map_pool_map_version_id added to GameLaunchOptions.This field enables the client to receive map pool version context for each game launch.
35-40: LGTM: MatchmakerQueueMapPoolVetoData structure.The NamedTuple cleanly encapsulates veto configuration data for a queue's map pool bracket.
tests/integration_tests/conftest.py (3)
78-84: LGTM: veto_service fixture properly configured.The fixture follows the established pattern for service lifecycle management with proper initialization and shutdown.
51-64: LGTM: ladder_service integration with veto_service.The fixture correctly wires the veto_service dependency into LadderService construction.
163-185: LGTM: veto_service integrated into lobby server factory.The service is properly added to both the factory signature and the _override_services dictionary.
server/ladder_service/ladder_service.py (7)
84-91: LGTM: VetoService integrated into LadderService.The veto_service parameter and storage follow the established service dependency pattern.
126-145: LGTM: Veto configuration loaded and propagated to map pools.The database unpacking and MatchmakerQueueMapPool construction correctly include all veto-related fields.
152-154: LGTM: Affected players' searches canceled on veto config update.This ensures players whose vetoes were adjusted due to configuration changes are removed from the queue, preventing stale veto state from affecting matchmaking.
161-202: LGTM: map_pool_map_version_id propagated through data loading.The field is correctly aliased in the query (line 161) and passed to both Map (line 190) and NeroxisGeneratedMap.of (line 202), enabling veto tracking per map pool version.
229-270: LGTM: Veto fields retrieved from database.The query correctly aliases matchmaker_queue_map_pool_id and selects veto_tokens_per_player, max_tokens_per_map, and minimum_maps_after_veto fields, which are then unpacked into the map_pools list.
642-643: LGTM: map_pool_map_version_id included in game launch options.This ensures clients receive the map pool version context for each game, enabling proper veto tracking on the client side.
572-590: No changes required; dictionary key access is correct.The code correctly uses
map_pool.idto access the dictionary. Themap_poolsdictionary is keyed bymatchmaker_queue_map_pool.map_pool.id(line 87 of matchmaker_queue.py), andget_map_pool_for_rating()returns themap_poolfield from MatchmakerQueueMapPool (aMapPoolobject). Accessing the dictionary with this returnedmap_pool.idis consistent with the storage key, so there is no key inconsistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/integration_tests/test_matchmaker_vetoes.py (2)
38-40: DRY the pop-timer patches via a fixtureSame patches repeated in multiple tests. Extract into a module-scoped fixture to reduce duplication and keep timings consistent.
+# conftest.py +import pytest + +@pytest.fixture(autouse=True) +def fast_pop_timer(mocker): + mocker.patch("server.matchmaker.pop_timer.config.QUEUE_POP_TIME_MAX", 0.02) + mocker.patch("server.matchmaker.pop_timer.config.QUEUE_POP_TIME_MIN", 0.01)Then drop the per-test patches.
Also applies to: 62-64, 85-87, 112-114
102-107: Avoid brittle hard-coded map_version IDs in assertionsAs DB fixtures evolve, these constants may drift. Derive expected IDs from the pool or assert relational properties (e.g., “not vetoed by either team”) instead.
Also applies to: 132-136, 178-181
server/ladder_service/veto_system.py (5)
57-61: Variable naming nit: pools_vetodata → pools_veto_data (consistency)Minor readability polish; reduces cognitive load.
- pools_vetodata = self.extract_pools_veto_config(queues) + new_pools_veto_data = self.extract_pools_veto_config(queues) - if self.pools_veto_data != pools_vetodata: - self.pools_veto_data = pools_vetodata + if self.pools_veto_data != new_pools_veto_data: + self.pools_veto_data = new_pools_veto_data
148-169: Encapsulate PlayerVetoes mutationAvoid touching player.vetoes._vetoes directly; add a setter/update method to PlayerVetoes to keep invariants localized.
class PlayerVetoes: def __init__(self): self._vetoes: dict[BracketID, VetosMap] = {} + def set_all(self, new_vetoes: dict[BracketID, VetosMap]) -> None: + self._vetoes = new_vetoes # usage - player.vetoes._vetoes = adjusted_vetoes + player.vetoes.set_all(adjusted_vetoes)Also applies to: 298-321
170-182: Prefer named fields over positional unpackingUsing tuple positions from MatchmakerQueueMapPool harms readability. Access by name.
- ( - pool_id, - pool, - *_, - max_tokens_per_map, - minimum_maps_after_veto - ) = matchmaker_queue_map_pool + pool_id = matchmaker_queue_map_pool.id + pool = matchmaker_queue_map_pool.map_pool + max_tokens_per_map = matchmaker_queue_map_pool.max_tokens_per_map + minimum_maps_after_veto = matchmaker_queue_map_pool.minimum_maps_after_veto
257-280: Defensive guard for denominator and constraintsUse > 0 (not != 0) and assert non-negativity to make intent explicit. Based on learnings.
- if queue_config.max_tokens_per_map != 0: + # max_tokens_per_map is guaranteed non-negative; treat only positive as enabling static cap + if queue_config.max_tokens_per_map > 0: total_players = queue.team_size * 2 vetoable_maps_per_player = queue_config.veto_tokens_per_player / queue_config.max_tokens_per_map vetoable_maps = total_players * vetoable_maps_per_player if vetoable_maps > num_maps - queue_config.minimum_maps_after_veto: return FalseOptionally, enforce at entry:
+ assert queue_config.veto_tokens_per_player >= 0 + assert queue_config.max_tokens_per_map >= 0 + assert queue_config.minimum_maps_after_veto >= 0
323-335: Clarify truncation semantics when capping per-map tokensCasting float to int floors silently. If this is intentional, make it explicit with math.floor and a brief comment.
- if max_per_map > 0: - applied = int(min(applied, max_per_map)) + if max_per_map > 0: + # floor to ensure we never exceed the static cap + from math import floor + applied = min(applied, floor(max_per_map))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/ladder_service/veto_system.py(1 hunks)server/types.py(5 hunks)tests/integration_tests/test_matchmaker_vetoes.py(1 hunks)tests/unit_tests/test_veto_system.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/types.py
- tests/unit_tests/test_veto_system.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.707Z
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.
📚 Learning: 2025-10-26T15:11:39.707Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.707Z
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:
server/ladder_service/veto_system.py
📚 Learning: 2025-10-26T14:43:05.074Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:257-280
Timestamp: 2025-10-26T14:43:05.074Z
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:
server/ladder_service/veto_system.py
🧬 Code graph analysis (2)
tests/integration_tests/test_matchmaker_vetoes.py (8)
server/players.py (1)
PlayerState(24-31)tests/utils/event_loop.py (1)
fast_forward(52-69)tests/integration_tests/conftest.py (3)
connect_and_sign_in(559-569)lobby_server(310-311)get(410-414)tests/integration_tests/test_game.py (4)
client_response(124-127)end_game_as_draw(171-198)gen_vetoes(201-209)queue_player_for_matchmaking(212-226)tests/conftest.py (2)
player_service(307-310)database(198-200)server/player_service.py (1)
get_player(189-190)server/ladder_service/veto_system.py (1)
to_dict(26-37)server/ladder_service/ladder_service.py (1)
update_data(100-154)
server/ladder_service/veto_system.py (7)
server/core/service.py (1)
Service(13-54)server/decorators.py (1)
with_logger(15-29)server/exceptions.py (1)
ClientError(10-21)server/matchmaker/map_pool.py (1)
MatchmakerQueueMapPool(114-121)server/player_service.py (1)
all_players(72-73)server/players.py (3)
Player(35-185)write_message(133-143)send_message(121-131)server/types.py (3)
MatchmakerQueueMapPoolVetoData(35-40)id(45-45)map_pool_map_version_id(47-47)
🪛 Ruff (0.14.1)
server/ladder_service/veto_system.py
135-135: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (3)
server/ladder_service/veto_system.py (2)
80-87: Background vs. request-context messagingupdate_pools_veto_config correctly uses write_message. set_player_vetoes awaits send_message; ensure it’s only called from the player’s connection handler. If not, switch to write_message.
Also applies to: 141-147
68-88: ****The review comment incorrectly identifies a bug.
all_playersis a@propertydecorator atserver/player_service.py:72, not a method. Properties are accessed as attributes without parentheses. The code inveto_system.py:68is correct:for player in self.player_service.all_players:Applying the suggested fix would introduce a bug by attempting to call a property. All usages across the codebase correctly omit parentheses.
Likely an incorrect or invalid review comment.
tests/integration_tests/test_matchmaker_vetoes.py (1)
41-48: Review comment is reasonable but imprecise in recommendationsThe code review correctly identifies that all four flagged tests contain
for _ in range(20):loops with I/O operations (player queueing, match waits up to 10-30s). However, the suggestion requires clarification:
- test_partial_vetoes explicitly validates that both maps 10 and 11 are used via
assert chosen_maps == {10, 11}, requiring sufficient iterations for statistical validity. Reducing iterations could cause false test failures.- The other tests similarly repeat match scenarios to verify veto logic across multiple attempts, not arbitrary repetition.
- The suggestion to "seed match selection for determinism" is vague and would require changes to matchmaker mocking strategy, not just loop counts.
To make this actionable, the review should specify: (1) target iteration count with justification, (2) concrete approach to achieve determinism (e.g., mock the matchmaker's map selection), or (3) measured runtime data showing the loops cause CI bottlenecks.
K-ETFreeman
changed the base branch from
develop
to
feature/#1032-working-veto-system
K-ETFreeman
merged commit 3795aff
into
FAForever:feature/#1032-working-veto-system
4 participants
Working on #1032
I need some feedback, what is right, what is wrong, what needs to be changed, etc
Summary by CodeRabbit
New Features
Improvements
Tests