Waive time requirement for ranking custom games when recalling #1073
Conversation
WalkthroughPre-rate validity checks now accept per-team army outcomes; RECALL was added to outcome enums. CustomGame infers possible connection issues from team outcomes and only marks games TOO_SHORT when both short duration and quitting-like outcomes (including RECALL) are present. Tests and helpers updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Game as Game (server/games/game.py)
participant Custom as CustomGame (server/games/custom_game.py)
Game->>Game: build team_player_partial_outcomes
Game->>Custom: _run_pre_rate_validity_checks(team_player_partial_outcomes)
activate Custom
Custom->>Custom: duration = finished_at - launched_at
Note right of Custom `#F0F8FF`: looks_like_quitting = {DEFEAT, UNKNOWN, CONFLICTING, RECALL}
Custom->>Custom: analyze team_army_outcomes for quitting-like patterns
alt short duration AND potential connection issues
Custom->>Custom: mark_invalid(ValidityState.TOO_SHORT)
else
Custom->>Custom: proceed with normal validation
end
deactivate Custom
Custom-->>Game: return validity decision
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/games/custom_game.py(2 hunks)server/games/game.py(2 hunks)server/games/game_results.py(2 hunks)tests/unit_tests/test_custom_game.py(1 hunks)tests/unit_tests/test_game_resolution.py(1 hunks)tests/unit_tests/test_game_results.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/unit_tests/test_game_results.py (1)
server/games/game_results.py (3)
ArmyReportedOutcome(27-46)to_resolved(38-46)ArmyOutcome(15-24)
server/games/game.py (2)
server/games/custom_game.py (1)
_run_pre_rate_validity_checks(22-43)server/games/game_results.py (1)
ArmyOutcome(15-24)
tests/unit_tests/test_game_resolution.py (2)
server/games/game_results.py (3)
ArmyOutcome(15-24)GameResolutionError(237-238)resolve_game(241-305)server/db/typedefs.py (1)
GameOutcome(17-21)
tests/unit_tests/test_custom_game.py (5)
server/games/custom_game.py (1)
CustomGame(10-43)tests/conftest.py (1)
players(298-303)server/games/typedefs.py (2)
GameState(10-14)ValidityState(48-74)server/games/game.py (5)
players(212-223)set_player_option(616-621)launch(725-746)add_result(317-359)on_game_finish(467-488)tests/unit_tests/conftest.py (1)
add_connected_players(168-179)
server/games/custom_game.py (2)
server/games/game_results.py (2)
ArmyOutcome(15-24)outcome(105-117)server/games/game.py (2)
_run_pre_rate_validity_checks(490-491)players(212-223)
🪛 GitHub Actions: Lint
server/games/game_results.py
[error] 21-21: flake8: E261 at least two spaces before inline comment.
server/games/game.py
[warning] 519-519: flake8: W293 blank line contains whitespace.
server/games/custom_game.py
[error] 37-37: Need type annotation for "all_outcomes" (hint: "all_outcomes: dict[, ] = ...") [var-annotated]
[error] 39-39: Argument 1 to "ior" of "dict" has incompatible type "set[ArmyOutcome]"; expected "Iterable[tuple[Any, Any]]" [arg-type]
[error] 40-40: Unsupported left operand type for & ("dict[Any, Any]") [operator]
🪛 GitHub Check: flake8
server/games/game_results.py
[failure] 21-21:
at least two spaces before inline comment
server/games/game.py
[warning] 519-519:
blank line contains whitespace
tests/unit_tests/test_game_resolution.py
[failure] 49-49:
multiple spaces after ','
[failure] 49-49:
multiple spaces after ','
[failure] 48-48:
whitespace before ']'
[failure] 48-48:
multiple spaces after ','
[failure] 48-48:
multiple spaces after ','
[failure] 48-48:
multiple spaces after ','
[failure] 48-48:
multiple spaces after ','
[failure] 48-48:
multiple spaces after ','
tests/unit_tests/test_custom_game.py
[failure] 16-16:
too many blank lines (3)
🪛 GitHub Check: mypy
server/games/custom_game.py
[failure] 40-40:
Unsupported left operand type for & ("dict[Any, Any]") [operator]
[failure] 39-39:
Argument 1 to "ior" of "dict" has incompatible type "set[ArmyOutcome]"; expected "Iterable[tuple[Any, Any]]" [arg-type]
[failure] 37-37:
Need type annotation for "all_outcomes" (hint: "all_outcomes: dict[, ] = ...") [var-annotated]
🪛 Ruff (0.14.3)
tests/unit_tests/test_game_resolution.py
49-49: Indexed access to type list uses type tuple instead of an integer or slice
(RUF016)
⏰ 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 (8)
server/games/game_results.py (1)
33-33: LGTM!The RECALL variant is correctly added and will map to
ArmyOutcome.RECALLthrough theto_resolved()method.tests/unit_tests/test_game_results.py (1)
22-22: LGTM!The test correctly verifies that
ArmyReportedOutcome.RECALLresolves toArmyOutcome.RECALL.tests/unit_tests/test_custom_game.py (1)
16-36: LGTM!The test correctly validates that an early recall (within the grace period) results in a VALID game state, which aligns with the PR objective of waiving the time requirement for ranking when there is evidence of unanimous recall.
server/games/game.py (2)
490-491: LGTM!The signature update correctly accepts team army results for enhanced pre-rate validation checks.
515-520: LGTM!The logic correctly constructs
team_player_partial_outcomesand passes it to the validity checks, enabling outcome-based validation.server/games/custom_game.py (3)
22-23: LGTM!The signature correctly accepts
team_army_outcomesparameter for outcome-based validation.
25-36: Excellent design for detecting connection issues.The logic correctly identifies that only a unanimous recall indicates intentional forfeiture, while defeats could indicate connection problems. The grace period approach is sound.
42-43: LGTM!The conditional logic correctly applies the
TOO_SHORTvalidity state only when connection issues are suspected and the game is within the grace period.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/games/game.py(2 hunks)server/games/game_results.py(2 hunks)tests/unit_tests/test_custom_game.py(1 hunks)tests/unit_tests/test_game_resolution.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit_tests/test_custom_game.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit_tests/test_game_resolution.py (2)
server/db/typedefs.py (1)
GameOutcome(17-21)server/games/game_results.py (3)
ArmyOutcome(15-24)GameResolutionError(237-238)resolve_game(241-305)
server/games/game.py (2)
server/games/custom_game.py (1)
_run_pre_rate_validity_checks(22-43)server/games/game_results.py (1)
ArmyOutcome(15-24)
🪛 GitHub Actions: Lint
tests/unit_tests/test_game_resolution.py
[error] 10-10: flake8: E302 expected 2 blank lines, found 1.
🪛 GitHub Actions: Test
tests/unit_tests/test_game_resolution.py
[error] 10-11: NameError: name 'Optional' is not defined during test collection. Ensure 'Optional' is imported from typing (e.g., from typing import Optional).
🪛 GitHub Check: Codacy Static Code Analysis
tests/unit_tests/test_game_resolution.py
[warning] 11-11: tests/unit_tests/test_game_resolution.py#L11
undefined name 'Optional' (F821)
[warning] 13-13: tests/unit_tests/test_game_resolution.py#L13
undefined name 'Optional' (F821)
🪛 GitHub Check: flake8
tests/unit_tests/test_game_resolution.py
[failure] 23-23:
expected 2 blank lines after class or function definition, found 1
[failure] 10-10:
expected 2 blank lines, found 1
🪛 Ruff (0.14.3)
tests/unit_tests/test_game_resolution.py
11-11: Undefined name Optional
(F821)
13-13: Undefined name Optional
(F821)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
server/games/game_results.py (1)
21-21: LGTM! Clean enum extension for RECALL outcome.The RECALL additions to both enums integrate seamlessly with the existing resolution flow via
to_resolved().Also applies to: 33-33
server/games/game.py (1)
490-491: LGTM! Method signature correctly threaded to enable outcome-aware validity checks.The updated signature accepts per-team army outcomes, allowing subclasses (like
CustomGame) to implement recall-aware connection issue detection.Also applies to: 520-521
tests/unit_tests/test_game_resolution.py (1)
10-26: Clean refactor using test wrappers to centralize assertion logic.The
ResolutionTestwrapper class eliminates repetitive assertion patterns and improves test readability. The grid-based approach systematically covers all 36 outcome permutations.Also applies to: 38-71
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit_tests/test_game_resolution.py (2)
62-63: Clarify the column order comment.The two-line comment layout is confusing and doesn't clearly convey the column order. A single-line comment would be clearer.
Apply this diff:
- # Victory Recall Unknown - # Defeat Draw Conflicting + # Columns: Victory, Defeat, Recall, Draw, Unknown, Conflicting grid = [[ERR_, WIN_, WIN_, WIN_, WIN_, WIN_], # Victory
73-73: Fix variable name to follow PEP 8.Variable names should be lowercase.
Resolutionshould beresolution.Apply this diff:
- for outcome2, Resolution in enumerate(row): + for outcome2, resolution in enumerate(row): team_outcomes = [{outcome_list[outcome1]}, {outcome_list[outcome2]}] - Resolution(team_outcomes) + resolution(team_outcomes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_tests/test_game_resolution.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit_tests/test_game_resolution.py (2)
server/db/typedefs.py (1)
GameOutcome(17-21)server/games/game_results.py (3)
ArmyOutcome(15-24)GameResolutionError(237-238)resolve_game(241-305)
⏰ 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)
tests/unit_tests/test_game_resolution.py (3)
13-25: Well-designed test helper class.The
ResolutionTestclass provides a clean abstraction that centralizes resolution testing logic, making the test cases more readable and maintainable. The use of__call__allows instances to be invoked directly, which is elegant.
64-72: Comprehensive grid test correctly validates all outcome combinations.The grid accurately models the resolution logic for all 36 possible 1v1 outcome pairs. The values correctly treat RECALL the same as DEFEAT for resolution purposes, which aligns with the PR objectives. The use of
enumerateproperly accesses indices foroutcome_list.Also applies to: 74-75
42-43: Excellent refactoring of test assertions.The conversion from direct
pytest.raisescalls and assertions to theResolutionTestinstances makes the tests more concise and consistent while preserving their behavior. This improves maintainability.Also applies to: 49-49, 83-83, 91-91, 99-99, 107-107, 115-115, 123-123
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit_tests/test_game_resolution.py (1)
52-73: Consider improving comment formatting for clarity.The grid logic correctly tests all 36 outcome combinations, and the RECALL row properly matches the DEFEAT row as expected. However, the column header comment (lines 59-60) has confusing staggered formatting.
Consider applying this diff for clearer column alignment:
- err_, win_, draw, loss = (resolve_to_error, resolve_to_win, resolve_to_draw, resolve_to_loss) - # Victory Recall Unknown - # Defeat Draw Conflicting - grid = [[err_, win_, win_, win_, win_, win_], # Victory + err = resolve_to_error + win = resolve_to_win + draw = resolve_to_draw + loss = resolve_to_loss + # Columns: Victory, Defeat, Recall, Draw, Unknown, Conflicting + grid = [[err, win, win, win, win, win], # Victory - [loss, draw, draw, err_, err_, err_], # Defeat - [loss, draw, draw, err_, err_, err_], # Recall - [loss, err_, err_, draw, err_, err_], # Draw - [loss, err_, err_, err_, err_, err_], # Unknown - [loss, err_, err_, err_, err_, err_]] # Conflicting + [loss, draw, draw, err, err, err], # Defeat + [loss, draw, draw, err, err, err], # Recall + [loss, err, err, draw, err, err], # Draw + [loss, err, err, err, err, err], # Unknown + [loss, err, err, err, err, err]] # Conflicting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/games/custom_game.py(2 hunks)tests/unit_tests/test_game_resolution.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/games/custom_game.py (2)
server/games/game_results.py (2)
ArmyOutcome(15-24)outcome(105-117)server/games/game.py (2)
_run_pre_rate_validity_checks(490-491)players(212-223)
tests/unit_tests/test_game_resolution.py (2)
server/db/typedefs.py (1)
GameOutcome(17-21)server/games/game_results.py (3)
ArmyOutcome(15-24)GameResolutionError(237-238)resolve_game(241-305)
⏰ 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/games/custom_game.py (1)
6-6: LGTM! Import and signature align with recall feature.The import of
ArmyOutcomeand the updated method signature correctly support the new recall-aware validity checking logic.Also applies to: 22-22
tests/unit_tests/test_game_resolution.py (2)
13-30: LGTM! Helper class effectively centralizes test logic.The
ResolutionTestclass encapsulates resolution expectations and reduces duplication across tests. The snake_case instance names (resolve_to_error,resolve_to_win, etc.) clearly convey test intent.
33-121: LGTM! Tests correctly use the new helper instances.All test functions properly leverage the
ResolutionTestwrappers, maintaining existing behavior while improving readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/games/custom_game.py (2)
22-22: Parameter name inconsistency with base class.The base class in
game.pyusesteam_army_resultswhile this override usesteam_army_outcomes. Consider using consistent parameter naming across the inheritance hierarchy for clarity.Apply this diff for consistency:
- async def _run_pre_rate_validity_checks(self, team_army_outcomes: list[set[ArmyOutcome]]): + async def _run_pre_rate_validity_checks(self, team_army_results: list[set[ArmyOutcome]]): assert self.launched_at is not None # if there were connection issues, they are likely to show up early in # the match and manifest as players quitting out - in these cases, we # grant a grace period to custom games before we count them as ranked limit = len(self.players) * 60 duration = time.time() - self.launched_at # As we only get extremely limited data (the army results), our lens of # what can look like "quitting out" is rather large (i.e. the player is # "defeated"). # In other words, only if a team unanimously recalled would we know # there weren't any connection issues. looks_like_quitting = {ArmyOutcome.DEFEAT, ArmyOutcome.UNKNOWN, ArmyOutcome.CONFLICTING} - all_outcomes = {outcome for team in team_army_outcomes for outcome in team} + all_outcomes = {outcome for team in team_army_results for outcome in team} possible_conn_issues = len(all_outcomes & looks_like_quitting) > 0 if not self.enforce_rating and possible_conn_issues and duration < limit:
25-40: Logic correctly implements recall exemption from TOO_SHORT validation.The implementation properly achieves the PR objective by:
- Excluding
RECALLfromlooks_like_quitting, treating it as an intentional forfeit- Only inferring connection issues when outcomes include DEFEAT, UNKNOWN, or CONFLICTING
- Requiring all three conditions (non-enforced rating, possible connection issues, short duration) before marking TOO_SHORT
This means unanimous recalls bypass the time requirement as intended, while still catching likely disconnects.
Optional: Simplify boolean check on line 38.
- possible_conn_issues = len(all_outcomes & looks_like_quitting) > 0 + possible_conn_issues = bool(all_outcomes & looks_like_quitting)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/games/custom_game.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/games/custom_game.py (2)
server/games/game_results.py (2)
ArmyOutcome(15-24)outcome(105-117)server/games/game.py (2)
_run_pre_rate_validity_checks(490-491)players(212-223)
⏰ 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 (1)
server/games/custom_game.py (1)
6-6: LGTM! Import is correct and necessary.The
ArmyOutcomeimport is properly used in the method signature and logic to distinguish intentional recalls from potential connection issues.
|
Why all this hassle when Wouldn't it be way less work to just enforce rating on recall like it is done when ACU is killed? And it won't require any server changes at all |
|
Hmm, I didn't know the game code was in charge of that - I thought the server set that property. That might be easier. My question is then, from a design stand-point, should the game or the server ultimately be in charge of if a game is ranked? It seems like it fundamentally shouldn't be part of the game, but maybe the ease-of-use you're suggesting would make it preferable nonetheless. It should be noted that, at this point, if effort is the limiting factor, then all of the work to make this happen has already been done. |
|
I guess I just chose words poorly. By 'less work' I mean 'less complex' or 'less changes/diff', and that often does not necessarily mean that there were 'less work/effort' done. In contrast, it often means that more effort were put into research to understand the problem better. It seems that |
|
We'll go ahead with the game-centered approach using |
3 participants
The time requirement for ranking custom games is in place so that connection issues can be discovered early in the match and rehosted without penalty to rating. However, if a team unanimously agrees to recall during that time frame, it is safe to presume that it was not due to any connection issues and they actually do want to forfeit the match.
This PR introduces a check on the prevalidation step of ranking a game that skips the
TOO_SHORTpath if there wasn't anything that looks like it could have been a connection issue by introducing aRECALLarmy result.Note that the game itself will still need to be updated to report such a result; this is only the preparation step to allow the server to handle recalling.
Summary by CodeRabbit
New Features
Bug Fixes
Tests