Skip to content

🧪 [Add test coverage for _save_best_model_to_disk] #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sheepdestroyer wants to merge 1 commit into
base: master
Choose a base branch
from
+41 −0
Open

🧪 [Add test coverage for _save_best_model_to_disk] #379

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions router/tests/test_save_best_model_to_disk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import pytest
from unittest.mock import patch, mock_open
from router import main

def test_save_best_model_to_disk_success():
best_model = {"model_name": "agent-gemma", "score": 100}

mock_file = mock_open()

with patch("builtins.open", mock_file), \
patch("json.dump") as mock_json_dump, \
patch("datetime.datetime") as mock_datetime:

# Setup mock datetime
mock_now = mock_datetime.now.return_value
mock_now.isoformat.return_value = "2023-10-25T12:00:00+00:00"

Comment on lines +8 to +17

@sourcery-ai sourcery-ai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Strengthen the exception-path test and consider additional error conditions.

The PermissionError test only asserts that open was called; pytest will fail on unexpected exceptions, but it would be clearer to assert the function’s return value (e.g., None) or that json.dump is not called. If _save_best_model_to_disk is meant to handle other filesystem errors (such as FileNotFoundError or OSError), consider parametrized tests for these cases to ensure all error paths are explicitly covered.

Suggested implementation:

@pytest.mark.parametrize("exception_type", [PermissionError, FileNotFoundError, OSError])
def test_save_best_model_to_disk_exception_handled(exception_type):
    best_model = {"model_name": "agent-gemma", "score": 100}

    mock_file = mock_open()

    with patch("builtins.open", mock_file), patch("json.dump") as mock_json_dump:
        # Simulate different filesystem-related exceptions when opening the file
        mock_file.side_effect = exception_type("unable to open file")

        result = main._save_best_model_to_disk(best_model)

        # Explicitly assert that the exception path returns a neutral value
        assert result is None

        # Ensure json.dump is never called on failure
        mock_json_dump.assert_not_called()

        # open should still be called with the expected path and mode
        mock_file.assert_called_once_with("/config/router_dir/best_free_model.json", "w")
  1. If _save_best_model_to_disk does not currently return None on error, update its implementation accordingly or adjust the assertion to match the intended return value.
  2. Ensure _save_best_model_to_disk is actually catching and handling PermissionError, FileNotFoundError, and OSError; if not, add the appropriate try/except blocks in router/main.py so these tests pass.

main._save_best_model_to_disk(best_model)
Comment on lines +10 to +18

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that the timestamp is generated in UTC.

The test verifies the Z transformation, but it would still pass if the implementation called datetime.now() without timezone.utc. Assert mock_datetime.now was called with datetime.timezone.utc to cover the upstream contract.

Suggested assertion 
+import datetime
+
...
         main._save_best_model_to_disk(best_model)
+        mock_datetime.now.assert_called_once_with(datetime.timezone.utc)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
with patch("builtins.open", mock_file), \
patch("json.dump") as mock_json_dump, \
patch("datetime.datetime") as mock_datetime:
# Setup mock datetime
mock_now = mock_datetime.now.return_value
mock_now.isoformat.return_value = "2023-10-25T12:00:00+00:00"
main._save_best_model_to_disk(best_model)
with patch("builtins.open", mock_file), \
patch("json.dump") as mock_json_dump, \
patch("datetime.datetime") as mock_datetime:
# Setup mock datetime
mock_now = mock_datetime.now.return_value
mock_now.isoformat.return_value = "2023-10-25T12:00:00+00:00"
main._save_best_model_to_disk(best_model)
mock_datetime.now.assert_called_once_with(datetime.timezone.utc)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@router/tests/test_save_best_model_to_disk.py` around lines 10 - 18, Update
the test around main._save_best_model_to_disk to assert that mock_datetime.now
is called with datetime.timezone.utc, while preserving the existing ISO
timestamp and Z-transformation assertions.


# Verify file opened correctly
mock_file.assert_called_once_with("/config/router_dir/best_free_model.json", "w")

# Verify json.dump called with correct payload
expected_payload = {
"model_name": "agent-gemma",
"score": 100,
"updated_at": "2023-10-25T12:00:00Z"
}
mock_json_dump.assert_called_once_with(expected_payload, mock_file(), indent=2)

def test_save_best_model_to_disk_exception_handled():
best_model = {"model_name": "agent-gemma"}

mock_file = mock_open()
mock_file.side_effect = PermissionError("Cannot write to disk")

# Should not raise exception
with patch("builtins.open", mock_file):
main._save_best_model_to_disk(best_model)

mock_file.assert_called_once_with("/config/router_dir/best_free_model.json", "w")