Fixed docstrings, typos and added test cases (#1176)

* Fixed docstrings, typos and added test cases

* rollback printing out exception message in log line

Author: bayta-darell-1400

lib/engine_wrapper.py

@@ -39,6 +39,7 @@ def create_engine(engine_config: Configuration, game: model.Game | None = None)
     Use in a with-block to automatically close the engine when exiting the game.
 
     :param engine_config: The options for the engine.
+    :param game: The game to create the engine from.
     :return: An engine. Either UCI, XBoard, or Homemade.
     """
     cfg = engine_config.engine

lib/lichess_bot.py

@@ -136,6 +136,7 @@ def do_correspondence_ping(control_queue: CONTROL_QUEUE_TYPE, period: datetime.t
     """
     Tell the engine to check the correspondence games.
 
+    :param control_queue: Queue to put events in.
     :param period: How many seconds to wait before sending a correspondence ping.
     """
     while not stop.terminated:
@@ -167,7 +168,7 @@ def logging_configurer(level: int, filename: str | None, disable_auto_logs: bool
 
     :param level: The logging level. Either `logging.INFO` or `logging.DEBUG`.
     :param filename: The filename to write the logs to. If it is `None` then the logs aren't written to a file.
-    :param auto_log_filename: The filename for the automatic logger. If it is `None` then the logs aren't written to a file.
+    :param disable_auto_logs: Whether to disable automatic logging.
     """
     console_handler = RichHandler()
     console_formatter = logging.Formatter("%(message)s")
@@ -252,7 +253,7 @@ def start(li: lichess.Lichess, user_profile: UserProfileType, config: Configurat
     :param config: The config that the bot will use.
     :param logging_level: The logging level. Either `logging.INFO` or `logging.DEBUG`.
     :param log_filename: The filename to write the logs to. If it is `None` then the logs aren't written to a file.
-    :param auto_log_filename: The filename for the automatic logger. If it is `None` then the logs aren't written to a file.
+    :param disable_auto_logging: Whether to disable automatic logging.
     :param one_game: Whether the bot should play only one game. Only used in `test_bot/test_bot.py` to test lichess-bot.
     """
     logger.info(f"You're now connected to {config.url} and awaiting challenges.")
@@ -337,6 +338,7 @@ def lichess_bot_main(li: lichess.Lichess,
     :param control_queue: The queue containing all the events.
     :param correspondence_queue: The queue containing the correspondence games.
     :param logging_queue: The logging queue. Used by `logging_listener_proc`.
+    :param pgn_queue: The queue containing the PGN games.
     :param one_game: Whether the bot should play only one game. Only used in `test_bot/test_bot.py` to test lichess-bot.
     """
     max_games = config.challenge.concurrency
@@ -661,6 +663,7 @@ def play_game(li: lichess.Lichess,
     :param challenge_queue: The queue containing the challenges.
     :param correspondence_queue: The queue containing the correspondence games.
     :param logging_queue: The logging queue. Used by `logging_listener_proc`.
+    :param pgn_queue: The queue containing the PGN games.
     """
     thread_logging_configurer(logging_queue)
     logger = logging.getLogger(__name__)
@@ -766,7 +769,7 @@ def play_game(li: lichess.Lichess,
 
 
 def read_takeback_record(game: model.Game) -> int:
-    """Read the number of move takeback requests accepeted in a game."""
+    """Read the number of move takeback requests accepted in a game."""
     try:
         with open(takeback_record_file_name(game.id)) as takeback_file:
             return int(takeback_file.read())
@@ -775,7 +778,7 @@ def read_takeback_record(game: model.Game) -> int:
 
 
 def record_takeback(game: model.Game, accepted_count: int) -> None:
-    """Record the number of move takeback requests accepeted in a game."""
+    """Record the number of move takeback requests accepted in a game."""
     with open(takeback_record_file_name(game.id), "w") as takeback_file:
         takeback_file.write(str(accepted_count))
 

test_bot/test_config.py

@@ -0,0 +1,33 @@
+"""Test functions for config module."""
+import logging
+
+import pytest
+
+from lib import config
+
+
+def test_config_assert__false() -> None:
+    """Test that config_assert raises an exception with the provided error message."""
+    with pytest.raises(Exception, match="some error"):
+        config.config_assert(False, "some error")
+
+
+def test_config_assert__true() -> None:
+    """Test that config_assert does not raise when assertion is True."""
+    config.config_assert(True, "no error")
+
+
+def test_config_warn__true(caplog: pytest.LogCaptureFixture) -> None:
+    """Test that config_warn does not log a warning when assertion is True."""
+    with caplog.at_level(logging.WARNING):
+        config.config_warn(True, "this should not appear")
+        assert len(caplog.records) == 0  # No warning should be logged
+
+
+def test_config_warn__false(caplog: pytest.LogCaptureFixture) -> None:
+    """Test that config_warn logs a warning when assertion is False."""
+    with caplog.at_level(logging.WARNING):
+        config.config_warn(False, "test warning message")
+        assert "test warning message" in caplog.text
+        assert len(caplog.records) == 1
+        assert caplog.records[0].levelname == "WARNING"

test_bot/test_matchmaking.py

@@ -1,5 +1,8 @@
 """Test functions for matchmaking module."""
-from lib.matchmaking import game_category
+from unittest.mock import Mock
+from lib.matchmaking import game_category, Matchmaking
+from lib.config import Configuration
+from lib.lichess_types import UserProfileType
 
 
 def test_game_category_standard_bullet() -> None:
@@ -165,3 +168,59 @@ def test_game_category_realistic_scenarios() -> None:
 
     # 30+20 classical
     assert game_category("standard", 1800, 20, 0) == "classical"
+
+
+def test_get_random_config_value__returns_specific_value() -> None:
+    """Test that get_random_config_value returns the config value when it's not 'random'."""
+    # Create mock objects
+    mock_li = Mock()
+    mock_config = Configuration({
+        "challenge": {"variants": ["standard"]},
+        "matchmaking": {
+            "allow_matchmaking": False,
+            "block_list": [],
+            "online_block_list": [],
+            "challenge_timeout": 30
+        }
+    })
+    mock_user_profile: UserProfileType = {"username": "testbot", "perfs": {}}
+
+    # Create matchmaking instance
+    matchmaking = Matchmaking(mock_li, mock_config, mock_user_profile)
+
+    # Create config with a specific value
+    test_config = Configuration({"challenge_variant": "atomic"})
+
+    # Test that it returns the specific value, not a random choice
+    choices = ["standard", "chess960", "atomic", "horde"]
+    result = matchmaking.get_random_config_value(test_config, "challenge_variant", choices)
+
+    assert result == "atomic", f"Expected 'atomic' but got '{result}'"
+
+
+def test_get_random_config_value__returns_from_choices_when_random() -> None:
+    """Test that get_random_config_value returns a value from choices when config value is 'random'."""
+    # Create mock objects
+    mock_li = Mock()
+    mock_config = Configuration({
+        "challenge": {"variants": ["standard"]},
+        "matchmaking": {
+            "allow_matchmaking": False,
+            "block_list": [],
+            "online_block_list": [],
+            "challenge_timeout": 30
+        }
+    })
+    mock_user_profile: UserProfileType = {"username": "testbot", "perfs": {}}
+
+    # Create matchmaking instance
+    matchmaking = Matchmaking(mock_li, mock_config, mock_user_profile)
+
+    # Create config with "random" value
+    test_config = Configuration({"challenge_mode": "random"})
+
+    # Test that it returns one of the choices
+    choices = ["casual", "rated"]
+    result = matchmaking.get_random_config_value(test_config, "challenge_mode", choices)
+
+    assert result in choices, f"Expected result to be in {choices} but got '{result}'"

test_bot/test_model.py

@@ -80,6 +80,12 @@ def test_game() -> None:
     assert game_model.id == "zzzzzzzz"
     assert game_model.mode == "casual"
     assert game_model.is_white is False
+    assert game_model.my_color == "black"
+    assert game_model.url() == "https://lichess.org/zzzzzzzz/black"
+    assert game_model.short_url() == "https://lichess.org/zzzzzzzz"
+    assert game_model.pgn_event() == "Casual Bullet game"
+    assert game_model.time_control() == "90+1"
+    assert game_model.is_abortable() is True
 
 
 def test_player() -> None:
@@ -88,3 +94,17 @@ def test_player() -> None:
     player_model = model.Player(player)
     assert player_model.is_bot is True
     assert str(player_model) == "BOT b (3000)"
+
+
+def test_is_chess_960() -> None:
+    """Test the is_chess_960 function."""
+    items = [
+        {"fen": "rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR w KQkq - 0 1", "type": "standard", "is_960": False},
+        {"fen": "brnkrqnb/pppppppp/8/8/8/8/PPPPPPPP/BRNKRQNB w KQkq - 0 1", "type": "960", "is_960": True},  # pos1
+        {"fen": "nrbbnkqr/pppppppp/8/8/8/8/PPPPPPPP/NRBBNKQR w KQkq - 0 1", "type": "960", "is_960": True},  # pos2
+    ]
+    for item in items:
+        fen = str(item["fen"])
+        expected = item["is_960"]
+        result = model.is_chess_960(fen)
+        assert result == expected