Game Rating: Clean up some formatting and faulty logic that I missed …

…before (#519)

* Clean up some formatting and faulty logic that I missed before

* Simplify ended check

server/games/game.py

@@ -7,16 +7,15 @@
 from enum import Enum, unique
 from typing import Any, Dict, Optional, Tuple
 
-import trueskill
+from server.config import FFA_TEAM
+from server.games.game_rater import GameRater
 from server.games.game_results import GameOutcome, GameResult, GameResults
 from server.rating import RatingType
-from server.games.game_rater import GameRater
+from trueskill import Rating
 
 from ..abc.base_game import GameConnectionState, InitMode
 from ..players import Player, PlayerState
 
-from server.config import FFA_TEAM
-
 
 @unique
 class GameState(Enum):
@@ -373,9 +372,8 @@ def add_game_connection(self, game_connection):
                 f"Invalid GameConnectionState: {game_connection.state}"
             )
         if self.state != GameState.LOBBY and self.state != GameState.LIVE:
-            raise GameError(
-                "Invalid GameState: {state}".format(state=self.state)
-            )
+            raise GameError(f"Invalid GameState: {self.state}")
+
         self._logger.info("Added game connection %s", game_connection)
         self._connections[game_connection.player] = game_connection
 
@@ -398,7 +396,8 @@ async def remove_game_connection(self, game_connection):
         self._logger.info("Removed game connection %s", game_connection)
 
         def host_left_lobby() -> bool:
-            return game_connection.player == self.host and self.state != GameState.LIVE
+            return (game_connection.player == self.host and
+                    self.state != GameState.LIVE)
 
         if len(self._connections) == 0 or host_left_lobby():
             await self.on_game_end()
@@ -410,7 +409,7 @@ async def check_sim_end(self):
             return
         if self.state != GameState.LIVE:
             return
-        if len([ conn for conn in self._connections.values() if not conn.finished_sim ]) > 0:
+        if [conn for conn in self.connections if not conn.finished_sim]:
             return
         self.ended = True
         async with self._db.acquire() as conn:
@@ -757,8 +756,8 @@ async def update_game_player_stats(self):
 
             def is_observer() -> bool:
                 return (
-                    options.get('Team', -1) < 0
-                    or options.get('StartSpot', 0) < 0
+                    self._player_options[player.id].get("Team", -1) < 0
+                    or self._player_options[player.id].get("StartSpot", -1) < 0
                 )
 
             if is_observer():
@@ -817,7 +816,7 @@ def get_army_result(self, player):
 
         return self._results.outcome(army)
 
-    def compute_rating(self, rating=RatingType.GLOBAL):
+    def compute_rating(self, rating=RatingType.GLOBAL) -> Dict[Player, Rating]:
         """
         Compute new ratings
         :param rating: Rating type
@@ -830,8 +829,8 @@ def compute_rating(self, rating=RatingType.GLOBAL):
         if None in self.teams:
             raise GameError(
                 "Missing team for at least one player. (player, team): {}"
-                .format([(player, get_team(player))
-                         for player in self.players], )
+                .format([(player, self.get_player_option(player.id, 'Team'))
+                        for player in self.players])
             )
 
         outcome_by_player = {

server/games/game_rater.py

@@ -1,13 +1,13 @@
-import trueskill
-from trueskill import Rating
-from server.rating import RatingType
-from ..decorators import with_logger
+from typing import Dict, Iterable, List
 
-from server.games.game_results import GameOutcome
+import trueskill
 from server.config import FFA_TEAM
-
-from typing import List, Dict, Iterable
+from server.games.game_results import GameOutcome
 from server.players import Player
+from server.rating import RatingType
+from trueskill import Rating
+
+from ..decorators import with_logger
 
 
 class GameRatingError(Exception):
@@ -70,6 +70,9 @@ def _get_team_outcome(self, team: Iterable[Player]) -> GameOutcome:
         outcomes.discard(GameOutcome.UNKNOWN)
         if not outcomes:
             return GameOutcome.UNKNOWN
+        if GameOutcome.VICTORY in outcomes:
+            # One player surviving implies that the entire team won
+            return GameOutcome.VICTORY
         if len(outcomes) > 1:
             raise GameRatingError(
                 f"Attempted to rate game where one of the teams has inconsistent outcome. Teams: {self._players_by_team} Outcomes: {self._outcome_by_player}"

server/games/game_results.py

@@ -1,9 +1,10 @@
-from enum import Enum
 from collections import Counter
 from collections.abc import Mapping
-from server.decorators import with_logger
+from enum import Enum
 from typing import NamedTuple
 
+from server.decorators import with_logger
+
 
 class GameOutcome(Enum):
     VICTORY = 'victory'

tests/unit_tests/test_game.py

@@ -4,6 +4,7 @@
 from unittest import mock
 
 import pytest
+from asynctest import CoroutineMock
 from server.gameconnection import GameConnection, GameConnectionState
 from server.games import CoopGame, CustomGame
 from server.games.game import (
@@ -12,7 +13,6 @@
 from server.games.game_rater import GameRatingError
 from server.games.game_results import GameOutcome
 from server.rating import RatingType
-from asynctest import CoroutineMock
 from tests.unit_tests.conftest import (
     add_connected_player, add_connected_players, make_mock_game_connection
 )
@@ -389,6 +389,18 @@ async def test_game_is_invalid_due_to_desyncs(game: Game, players):
     assert game.validity is ValidityState.TOO_MANY_DESYNCS
 
 
+async def test_compute_rating_raises_game_error(game: Game, players):
+    game.state = GameState.LOBBY
+    add_connected_players(game, [players.hosting, players.joining])
+    # add_connected_players sets this, so we need to unset it again
+    del game._player_options[players.hosting.id]["Team"]
+    game.set_player_option(players.joining.id, "Team", 1)
+    await game.launch()
+
+    with pytest.raises(GameError):
+        game.compute_rating(rating=RatingType.LADDER_1V1)
+
+
 async def test_compute_rating_computes_global_ratings(game: Game, players):
     game.state = GameState.LOBBY
     players.hosting.ratings[RatingType.GLOBAL] = Rating(1500, 250)
@@ -436,7 +448,7 @@ async def test_compute_rating_balanced_teamgame(game: Game, player_factory):
     await game.launch()
     for player, result, team in players:
         await game.add_result(
-            player, player.id - 1, 'victory' if team is 2 else 'defeat', result
+            player, player.id - 1, 'victory' if team == 2 else 'defeat', result
         )
     result = game.compute_rating()
     for team in result:
@@ -486,6 +498,51 @@ async def test_compute_rating_sum_of_scores_edge_case(
             else:
                 assert new_rating < old_rating
 
+
+async def test_compute_rating_only_one_surviver(game: Game, player_factory):
+    """
+    When a player dies their score is reported as "defeat", but this does not
+    necessarily mean they lost the game, if their team mates went on and later
+    reported a "victory".
+    """
+    game.state = GameState.LOBBY
+    win_team = 2
+    lose_team = 3
+    players = [
+            (
+                player_factory(login=f"{i}", player_id=i, global_rating=Rating(1500, 200)),
+                outcome, result, team
+            )
+            for i, (outcome, result, team) in enumerate([
+                ("defeat", -10, lose_team),
+                ("defeat", -10, lose_team),
+                ("defeat", -10, lose_team),
+                ("defeat", -10, lose_team),
+                ("defeat", -10, win_team),
+                ("defeat", -10, win_team),
+                ("defeat", -10, win_team),
+                ("victory", 10, win_team),
+            ], 1)]
+    add_connected_players(game, [player for player, _, _, _ in players])
+    for player, _, _, team in players:
+        game.set_player_option(player.id, 'Team', team)
+        game.set_player_option(player.id, 'Army', player.id - 1)
+    await game.launch()
+
+    for player, outcome, result, team in players:
+        await game.add_result(player, player.id - 1, outcome, result)
+
+    result = game.compute_rating()
+    for team in result:
+        for player, new_rating in team.items():
+            old_rating = Rating(*player.ratings[RatingType.GLOBAL])
+            # `team` index in result might not coincide with `team` index in players
+            if player.id > 4:
+                assert new_rating > old_rating
+            else:
+                assert new_rating < old_rating
+
+
 async def test_compute_rating_two_player_FFA(game: Game, player_factory):
     game.state = GameState.LOBBY
     players = [
@@ -501,13 +558,13 @@ async def test_compute_rating_two_player_FFA(game: Game, player_factory):
     await game.launch()
 
     for player, result, _ in players:
-        outcome = 'victory' if player.id is 1 else 'defeat'
+        outcome = 'victory' if player.id == 1 else 'defeat'
         await game.add_result(player, player.id - 1, outcome, result)
     result = game.compute_rating()
     for team in result:
         for player, new_rating in team.items():
             old_rating = Rating(*player.ratings[RatingType.GLOBAL])
-            assert (new_rating > old_rating) is (player.id is 1)
+            assert (new_rating > old_rating) is (player.id == 1)
 
 
 async def test_compute_rating_does_not_rate_multi_team(
@@ -528,7 +585,7 @@ async def test_compute_rating_does_not_rate_multi_team(
     await game.launch()
 
     for player, result, _ in players:
-        outcome = 'victory' if result is 10 else 'defeat'
+        outcome = 'victory' if result == 10 else 'defeat'
         await game.add_result(player, player.id - 1, outcome, result)
     with pytest.raises(GameRatingError):
         game.compute_rating()
@@ -552,7 +609,7 @@ async def test_compute_rating_does_not_rate_multi_FFA(
     await game.launch()
 
     for player, result, _ in players:
-        outcome = 'victory' if result is 10 else 'defeat'
+        outcome = 'victory' if result == 10 else 'defeat'
         await game.add_result(player, player.id - 1, outcome, result)
     with pytest.raises(GameRatingError):
         game.compute_rating()
@@ -625,13 +682,14 @@ async def test_compute_rating_works_with_partially_unknown_results(
     await game.launch()
 
     for player, result, _ in players:
-        outcome = 'victory' if result is 10 else 'unknown'
+        outcome = 'victory' if result == 10 else 'unknown'
         await game.add_result(player, player.id - 1, outcome, result)
     result = game.compute_rating()
     for team in result:
         for player, new_rating in team.items():
             assert new_rating != Rating(*player.ratings[RatingType.GLOBAL])
 
+
 async def test_game_get_army_result_ignores_unknown_results(game,
                                                             game_add_players):
     game.state = GameState.LOBBY
@@ -956,7 +1014,6 @@ async def test_game_outcomes_no_results(game: Game, database, players):
     assert await game_player_scores(database, game) == expected_scores
 
 
-
 async def test_game_outcomes_conflicting(game: Game, database, players):
     game.state = GameState.LOBBY
     players.hosting.ratings[RatingType.LADDER_1V1] = Rating(1500, 250)

tests/unit_tests/test_game_rater.py

@@ -1,14 +1,14 @@
 import pytest
-
 from server.games.game_rater import GameRater, GameRatingError
 from server.games.game_results import GameOutcome
-
 from server.rating import RatingType
 from trueskill import Rating
 
+
 class MockPlayer:
     ratings = {RatingType.GLOBAL: (1500, 500), RatingType.LADDER_1V1: (1500, 500)}
 
+
 def test_get_rating_groups():
     p1, p2 = MockPlayer(), MockPlayer()
     players_by_team = {2: [p1], 3: [p2]}
@@ -50,7 +50,7 @@ def test_team_outcome_throws_if_inconsistent():
     p1, p2, p3, p4 = MockPlayer(), MockPlayer(), MockPlayer(), MockPlayer()
     players_by_team = {2: [p1, p2], 3: [p3, p4]}
     outcome_py_player = {
-        p1: GameOutcome.VICTORY,
+        p1: GameOutcome.MUTUAL_DRAW,
         p2: GameOutcome.DEFEAT,
         p3: GameOutcome.DEFEAT,
         p4: GameOutcome.DRAW
@@ -63,11 +63,27 @@ def test_team_outcome_throws_if_inconsistent():
         rater._get_team_outcome(players_by_team[3])
 
 
+def test_team_outcome_victory_has_priority():
+    p1, p2, p3, p4 = MockPlayer(), MockPlayer(), MockPlayer(), MockPlayer()
+    players_by_team = {2: [p1, p2], 3: [p3, p4]}
+    outcome_py_player = {
+        p1: GameOutcome.VICTORY,
+        p2: GameOutcome.DEFEAT,
+        p3: GameOutcome.DEFEAT,
+        p4: GameOutcome.DEFEAT
+    }
+
+    rater = GameRater(players_by_team, outcome_py_player)
+    assert rater._get_team_outcome(players_by_team[2]) is GameOutcome.VICTORY
+    assert rater._get_team_outcome(players_by_team[3]) is GameOutcome.DEFEAT
+
+
 def test_ranks():
     rater = GameRater({}, {})
     assert rater._ranks_from_team_outcomes([GameOutcome.VICTORY, GameOutcome.DEFEAT]) == [0,1]
     assert rater._ranks_from_team_outcomes([GameOutcome.DEFEAT, GameOutcome.VICTORY]) == [1,0]
 
+
 def test_ranks_with_unknown():
     rater = GameRater({}, {})
     assert rater._ranks_from_team_outcomes([GameOutcome.UNKNOWN, GameOutcome.DEFEAT]) == [0,1]
@@ -77,15 +93,18 @@ def test_ranks_with_unknown():
     with pytest.raises(GameRatingError):
         rater._ranks_from_team_outcomes([GameOutcome.UNKNOWN, GameOutcome.UNKNOWN])
 
+
 def test_ranks_with_double_victory_is_inconsistent():
     rater = GameRater({}, {})
     with pytest.raises(GameRatingError):
         rater._ranks_from_team_outcomes([GameOutcome.VICTORY, GameOutcome.VICTORY])
 
+
 def test_ranks_with_double_defeat_treated_as_draw():
     rater = GameRater({}, {})
     assert rater._ranks_from_team_outcomes([GameOutcome.DEFEAT, GameOutcome.DEFEAT]) == [0,0]
 
+
 def test_ranks_with_draw():
     rater = GameRater({}, {})
 
@@ -99,6 +118,7 @@ def test_ranks_with_draw():
     with pytest.raises(GameRatingError):
         rater._ranks_from_team_outcomes([GameOutcome.UNKNOWN, GameOutcome.DRAW])
 
+
 def test_compute_rating():
     p1, p2 = MockPlayer(), MockPlayer()
     players_by_team = {2: [p1], 3: [p2]}