From 5d1d0d000c083a8476e29ad2eefdf66dffd2060e Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 25 Feb 2026 12:08:48 +0100 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20#67=20review=20?= =?UTF-8?q?=E2=80=94=20error=20propagation,=20test=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SessionManager.new_game() now returns "" on dir creation failure instead of proceeding with a broken game-id. Main menu guards against empty return. Test suite tracks and cleans up created save directories in after_test(). Co-Authored-By: Claude Opus 4.6 --- client/scripts/autoloads/session_manager.gd | 2 +- client/tests/test_session_manager_sprint19.gd | 35 ++++++++++++++----- client/ui/main_menu.gd | 5 ++- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/client/scripts/autoloads/session_manager.gd b/client/scripts/autoloads/session_manager.gd index e15148dcb..a41c4b554 100644 --- a/client/scripts/autoloads/session_manager.gd +++ b/client/scripts/autoloads/session_manager.gd @@ -30,6 +30,7 @@ func new_game() -> String: if err != OK: push_error("SessionManager: failed to create save dir %s: %s" % [ save_path, error_string(err)]) + return "" GameState.current_game_id = game_id return game_id @@ -74,7 +75,6 @@ func quit_to_menu(node: Node) -> void: if _quit_dialog != null and is_instance_valid(_quit_dialog): return # Dialog already open _quit_dialog = ConfirmationDialog.new() - _quit_dialog.title = "" _quit_dialog.dialog_text = UIStrings.get_text("menu.confirm_quit") _quit_dialog.ok_button_text = UIStrings.get_text("menu.confirm_yes") _quit_dialog.cancel_button_text = UIStrings.get_text("menu.confirm_no") diff --git a/client/tests/test_session_manager_sprint19.gd b/client/tests/test_session_manager_sprint19.gd index 44bc41d81..ef6f68fd3 100644 --- a/client/tests/test_session_manager_sprint19.gd +++ b/client/tests/test_session_manager_sprint19.gd @@ -4,6 +4,9 @@ class_name TestSessionManagerSprint19 extends GdUnitTestSuite +# Game IDs created during the current test — deleted in after_test(). +var _created_ids: Array = [] + # --------------------------------------------------------------------------- # Lifecycle @@ -11,12 +14,28 @@ extends GdUnitTestSuite func before_test() -> void: GameState.current_game_id = "" + _created_ids = [] func after_test() -> void: + for game_id in _created_ids: + var path := "user://saves/" + game_id + DirAccess.remove_absolute(ProjectSettings.globalize_path(path)) + _created_ids.clear() GameState.current_game_id = "" +# --------------------------------------------------------------------------- +# Helper: call new_game() and track the created directory for cleanup. +# --------------------------------------------------------------------------- + +func _new_game() -> String: + var game_id := SessionManager.new_game() + if not game_id.is_empty(): + _created_ids.append(game_id) + return game_id + + # --------------------------------------------------------------------------- # GameState.current_game_id field # --------------------------------------------------------------------------- @@ -53,14 +72,14 @@ func test_session_manager_autoload_exists() -> void: # --------------------------------------------------------------------------- func test_new_game_returns_non_empty_string() -> void: - var game_id := SessionManager.new_game() + var game_id := _new_game() assert_str(game_id).override_failure_message( "SessionManager.new_game() must return a non-empty game-id string" ).is_not_empty() func test_new_game_sets_current_game_id_on_gamestate() -> void: - var game_id := SessionManager.new_game() + var game_id := _new_game() assert_str(GameState.current_game_id).override_failure_message( "new_game() must set GameState.current_game_id" ).is_equal(game_id) @@ -68,7 +87,7 @@ func test_new_game_sets_current_game_id_on_gamestate() -> void: func test_new_game_id_format_has_two_dashes() -> void: ## Format: -- — two separator dashes. - var game_id := SessionManager.new_game() + var game_id := _new_game() var parts := game_id.split("-") assert_int(parts.size()).override_failure_message( "game-id must have format -- (3 parts separated by '-')" @@ -77,7 +96,7 @@ func test_new_game_id_format_has_two_dashes() -> void: func test_new_game_id_first_part_is_8_digits() -> void: ## First part is YYYYMMDD — 8 decimal digits. - var game_id := SessionManager.new_game() + var game_id := _new_game() var parts := game_id.split("-") assert_int(parts[0].length()).override_failure_message( "game-id first part (date) must be 8 characters (YYYYMMDD)" @@ -86,7 +105,7 @@ func test_new_game_id_first_part_is_8_digits() -> void: func test_new_game_id_second_part_is_6_digits() -> void: ## Second part is HHMMSS — 6 decimal digits. - var game_id := SessionManager.new_game() + var game_id := _new_game() var parts := game_id.split("-") assert_int(parts[1].length()).override_failure_message( "game-id second part (time) must be 6 characters (HHMMSS)" @@ -95,7 +114,7 @@ func test_new_game_id_second_part_is_6_digits() -> void: func test_new_game_id_third_part_is_6_hex_chars() -> void: ## Third part is 6 hex characters (RNG seed). - var game_id := SessionManager.new_game() + var game_id := _new_game() var parts := game_id.split("-") assert_int(parts[2].length()).override_failure_message( "game-id third part (hex seed) must be 6 characters" @@ -105,8 +124,8 @@ func test_new_game_id_third_part_is_6_hex_chars() -> void: func test_new_game_ids_are_unique() -> void: ## Two rapid new_game() calls should produce different IDs ## (different RNG seeds; same-second timestamps are valid but seeds differ). - var id1 := SessionManager.new_game() - var id2 := SessionManager.new_game() + var id1 := _new_game() + var id2 := _new_game() # Check that hex seeds differ (they almost certainly will) var seed1 := id1.split("-")[2] var seed2 := id2.split("-")[2] diff --git a/client/ui/main_menu.gd b/client/ui/main_menu.gd index 0c6e4a22b..705abc86e 100644 --- a/client/ui/main_menu.gd +++ b/client/ui/main_menu.gd @@ -32,7 +32,10 @@ func _refresh_continue_state() -> void: func _on_new_game() -> void: - SessionManager.new_game() + var game_id := SessionManager.new_game() + if game_id.is_empty(): + push_error("MainMenu: new_game() failed to create save directory — cannot start") + return get_tree().change_scene_to_file(GAME_SCENE)