From 73e737d75b4eeed45b94e892771ec02608f567da Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Thu, 28 May 2026 08:25:11 -0300 Subject: [PATCH 1/6] refactor: overhaul unit tests and achieve 100% coverage on core models and services --- tests/unit/models/test_criteria_tree.py | 102 ++++++++++++++++ .../test_pipeline_execution_accessors.py | 33 +++++- .../services/grader/test_submission_grader.py | 112 ++++++++++++++---- tests/unit/services/test_sandbox_service.py | 102 ++++++++++++++++ 4 files changed, 325 insertions(+), 24 deletions(-) create mode 100644 tests/unit/models/test_criteria_tree.py create mode 100644 tests/unit/services/test_sandbox_service.py diff --git a/tests/unit/models/test_criteria_tree.py b/tests/unit/models/test_criteria_tree.py new file mode 100644 index 00000000..90140c2f --- /dev/null +++ b/tests/unit/models/test_criteria_tree.py @@ -0,0 +1,102 @@ +import pytest +from unittest.mock import MagicMock +from autograder.models.criteria_tree import TestNode, SubjectNode, CategoryNode, CriteriaTree +from autograder.models.abstract.test_function import TestFunction + +def create_mock_test_function(): + tf = MagicMock(spec=TestFunction) + tf.name = "mock_test" + return tf + +def test_test_node_repr(): + tf = create_mock_test_function() + node = TestNode(name="my_test", test_function=tf) + assert repr(node) == "TestNode(my_test)" + + node_with_params = TestNode(name="my_test", test_function=tf, parameters={"p": 1}) + assert repr(node_with_params) == "TestNode(my_test, params={'p': 1})" + + node_with_file = TestNode(name="my_test", test_function=tf, file_target=["test.py"]) + assert repr(node_with_file) == "TestNode(my_test, file=['test.py'])" + + node_with_both = TestNode(name="my_test", test_function=tf, parameters={"p": 1}, file_target=["test.py"]) + assert repr(node_with_both) == "TestNode(my_test, params={'p': 1}, file=['test.py'])" + +def test_subject_node_repr(): + tf = create_mock_test_function() + test_node = TestNode(name="test", test_function=tf) + subject_with_tests = SubjectNode(name="subject1", weight=50.0, tests=[test_node]) + assert repr(subject_with_tests) == "SubjectNode(subject1, weight=50.0, tests=1)" + + subject_with_subjects = SubjectNode(name="subject2", weight=100.0, subjects=[subject_with_tests]) + assert repr(subject_with_subjects) == "SubjectNode(subject2, weight=100.0, subjects=1)" + +def test_category_node_repr(): + tf = create_mock_test_function() + test_node = TestNode(name="test", test_function=tf) + category_with_tests = CategoryNode(name="cat1", weight=100.0, tests=[test_node]) + assert repr(category_with_tests) == "CategoryNode(cat1, weight=100.0, tests=1)" + + subject_node = SubjectNode(name="subj1", weight=100.0, tests=[test_node]) + category_with_subjects = CategoryNode(name="cat2", weight=100.0, subjects=[subject_node]) + assert repr(category_with_subjects) == "CategoryNode(cat2, weight=100.0, subjects=1)" + +def test_criteria_tree_repr(): + base = CategoryNode(name="base", weight=100.0) + bonus = CategoryNode(name="bonus", weight=10.0) + penalty = CategoryNode(name="penalty", weight=10.0) + + tree_base_only = CriteriaTree(base=base) + assert repr(tree_base_only) == "CriteriaTree(categories=['base'])" + + tree_bonus = CriteriaTree(base=base, bonus=bonus) + assert "bonus" in repr(tree_bonus) + assert repr(tree_bonus) == "CriteriaTree(categories=['base', 'bonus'])" + + tree_full = CriteriaTree(base=base, bonus=bonus, penalty=penalty) + assert repr(tree_full) == "CriteriaTree(categories=['base', 'bonus', 'penalty'])" + +def test_subject_get_all_tests_recursive(): + tf = create_mock_test_function() + + t1 = TestNode(name="t1", test_function=tf) + t2 = TestNode(name="t2", test_function=tf) + t3 = TestNode(name="t3", test_function=tf) + + # Sub 2 has t3 + sub2 = SubjectNode(name="sub2", weight=50.0, tests=[t3]) + + # Sub 1 has t2 and contains sub2 + sub1 = SubjectNode(name="sub1", weight=100.0, tests=[t2], subjects=[sub2]) + + # Base subject has t1 and contains sub1 + base_sub = SubjectNode(name="base_sub", weight=100.0, tests=[t1], subjects=[sub1]) + + tests = base_sub.get_all_tests() + assert len(tests) == 3 + assert tests[0].name == "t1" + assert tests[1].name == "t2" + assert tests[2].name == "t3" + +def test_category_get_all_tests_recursive(): + tf = create_mock_test_function() + + t1 = TestNode(name="t1", test_function=tf) + t2 = TestNode(name="t2", test_function=tf) + t3 = TestNode(name="t3", test_function=tf) + t4 = TestNode(name="t4", test_function=tf) + + sub2 = SubjectNode(name="sub2", weight=50.0, tests=[t4]) + sub1 = SubjectNode(name="sub1", weight=100.0, tests=[t3], subjects=[sub2]) + + cat = CategoryNode(name="cat", weight=100.0, tests=[t1, t2]) + cat.add_subjects([sub1]) + + tests = cat.get_all_tests() + assert len(tests) == 4 + names = [t.name for t in tests] + assert names == ["t1", "t2", "t3", "t4"] + +def test_category_get_all_tests_no_tests(): + cat = CategoryNode(name="empty", weight=100.0) + assert cat.get_all_tests() == [] diff --git a/tests/unit/pipeline/test_pipeline_execution_accessors.py b/tests/unit/pipeline/test_pipeline_execution_accessors.py index 605eb263..ea437ef8 100644 --- a/tests/unit/pipeline/test_pipeline_execution_accessors.py +++ b/tests/unit/pipeline/test_pipeline_execution_accessors.py @@ -6,7 +6,7 @@ from autograder.models.pipeline_execution import PipelineExecution from autograder.models.result_tree import CategoryResultNode, ResultTree, RootResultNode from autograder.template_library.input_output import InputOutputTemplate - +import pytest def _build_pipeline_execution() -> PipelineExecution: submission = Submission( @@ -42,6 +42,12 @@ def test_typed_accessors_return_expected_artifacts(): assert pipeline_exec.get_focus() is focus assert pipeline_exec.get_feedback() == "ok" assert pipeline_exec.get_sandbox() is None + assert pipeline_exec.get_ai_batch_results() is None + +def test_ai_batch_results_accessor(): + pipeline_exec = _build_pipeline_execution() + pipeline_exec.add_step_result(StepResult(step=StepName.AI_BATCH, data={"some": "data"}, status=StepStatus.SUCCESS)) + assert pipeline_exec.get_ai_batch_results() == {"some": "data"} def test_typed_accessors_raise_on_missing_required_artifacts(): @@ -62,8 +68,33 @@ def test_typed_accessors_raise_on_missing_required_artifacts(): except ValueError as exc: assert "required" in str(exc).lower() + # Empty list of templates + pipeline_exec.step_results[-1].data = [] + with pytest.raises(ValueError, match="No templates loaded"): + pipeline_exec.get_loaded_template() + try: pipeline_exec.get_focus() assert False, "Expected ValueError for missing focus step" except ValueError as exc: assert "not executed" in str(exc).lower() + +def test_finish_execution_failed_status(): + pipeline_exec = _build_pipeline_execution() + pipeline_exec.set_failure() + pipeline_exec.finish_execution() + assert pipeline_exec.result is None + +def test_finish_execution_missing_feedback_content(): + pipeline_exec = _build_pipeline_execution() + + result_tree = ResultTree(root=RootResultNode(base=CategoryResultNode(name="base", weight=100))) + grade = GradeStepResult(final_score=100.0, result_tree=result_tree) + focus = Focus(base=[], penalty=[], bonus=[]) + + pipeline_exec.add_step_result(StepResult(step=StepName.GRADE, data=grade, status=StepStatus.SUCCESS)) + pipeline_exec.add_step_result(StepResult(step=StepName.FOCUS, data=focus, status=StepStatus.SUCCESS)) + pipeline_exec.add_step_result(StepResult(step=StepName.FEEDBACK, data=None, status=StepStatus.FAIL)) + + with pytest.raises(ValueError, match="Feedback step exists but produced no feedback content"): + pipeline_exec.finish_execution() diff --git a/tests/unit/services/grader/test_submission_grader.py b/tests/unit/services/grader/test_submission_grader.py index 020c182c..31812dc7 100644 --- a/tests/unit/services/grader/test_submission_grader.py +++ b/tests/unit/services/grader/test_submission_grader.py @@ -22,40 +22,38 @@ def execute(self, files=None, sandbox=None, **kwargs): @pytest.fixture def grader(): command_resolver = MagicMock() + # Mock command_resolver.resolve_command just in case tests have program_command + command_resolver.resolve_command.side_effect = lambda c, lang: c return SubmissionGrader( submission_files={}, command_resolver=command_resolver ) def test_balance_nodes_only_subjects(grader): - # If only subjects, they should sum to 100 - s1 = SubjectNode(name="S1", weight=30) - s2 = SubjectNode(name="S2", weight=30) - cat = CategoryNode(name="base", weight=100, subjects=[s1, s2]) - - # We need to mock process_subject because it calls process_category/process_subject recursively - # Actually SubmissionGrader.process_category calls __process_holder which calls process_subject + # Need tests inside subjects to have real scores + tf = MockTestFunction() + t1 = TestNode(name="T1", test_function=tf, weight=100) + t2 = TestNode(name="T2", test_function=tf, weight=100) - # Let's mock process_test and process_subject to avoid deep recursion for this test - grader.process_subject = MagicMock(side_effect=lambda s: MagicMock(weight=s.weight, score=100.0)) + s1 = SubjectNode(name="S1", weight=30, tests=[t1]) + s2 = SubjectNode(name="S2", weight=30, tests=[t2]) + cat = CategoryNode(name="base", weight=100, subjects=[s1, s2]) result = grader.process_category(cat) # Weights should be balanced to 50/50 because original sum was 60 assert result.subjects[0].weight == 50.0 assert result.subjects[1].weight == 50.0 + assert result.calculate_score() == 100.0 def test_balance_nodes_with_subjects_weight(grader): # Scenario: subjects_weight = 80 - # 1 subject, 1 test tf = MockTestFunction() - s1 = SubjectNode(name="S1", weight=100) + t_sub = TestNode(name="T_Sub", test_function=tf, weight=100) + s1 = SubjectNode(name="S1", weight=100, tests=[t_sub]) t1 = TestNode(name="T1", test_function=tf, weight=100) cat = CategoryNode(name="base", weight=100, subjects=[s1], tests=[t1], subjects_weight=80) - grader.process_subject = MagicMock(side_effect=lambda s: MagicMock(weight=s.weight, score=100.0)) - # process_test is not mocked, it will execute the test - result = grader.process_category(cat) # Subject should have weight 80, Test should have weight 20 @@ -69,14 +67,11 @@ def test_balance_nodes_with_subjects_weight_and_scores(grader): # Scenario: subjects_weight = 80 # Subject score = 100, Test score = 0 tf = MockTestFunction() - s1 = SubjectNode(name="S1", weight=100) + t_sub = TestNode(name="T_Sub", test_function=tf, weight=100, parameters={'score': 100.0}) + s1 = SubjectNode(name="S1", weight=100, tests=[t_sub]) t1 = TestNode(name="T1", test_function=tf, weight=100, parameters={'score': 0.0}) cat = CategoryNode(name="base", weight=100, subjects=[s1], tests=[t1], subjects_weight=80) - # Mock process_subject to return a result node with score 100 - from autograder.models.result_tree import SubjectResultNode - grader.process_subject = MagicMock(return_value=SubjectResultNode(name="S1", weight=100, subjects_weight=None, score=100.0)) - result = grader.process_category(cat) assert result.subjects[0].weight == pytest.approx(80.0) @@ -87,13 +82,84 @@ def test_balance_nodes_with_subjects_weight_and_scores(grader): def test_balance_nodes_zero_weights(grader): # If all weights are zero, they should be equal and sum to 100 * factor - s1 = SubjectNode(name="S1", weight=0) - s2 = SubjectNode(name="S2", weight=0) - cat = CategoryNode(name="base", weight=100, subjects=[s1, s2]) + tf = MockTestFunction() + t1 = TestNode(name="T1", test_function=tf, weight=100) + t2 = TestNode(name="T2", test_function=tf, weight=100) - grader.process_subject = MagicMock(side_effect=lambda s: MagicMock(weight=s.weight, score=100.0)) + s1 = SubjectNode(name="S1", weight=0, tests=[t1]) + s2 = SubjectNode(name="S2", weight=0, tests=[t2]) + cat = CategoryNode(name="base", weight=100, subjects=[s1, s2]) result = grader.process_category(cat) assert result.subjects[0].weight == 50.0 assert result.subjects[1].weight == 50.0 + assert result.calculate_score() == 100.0 + +from autograder.models.dataclass.submission import SubmissionFile + +def test_balance_nodes_subjects_and_tests_missing_subjects_weight(grader): + tf = MockTestFunction() + s1 = SubjectNode(name="S1", weight=100, tests=[TestNode(name="T1", test_function=tf)]) + t2 = TestNode(name="T2", test_function=tf) + + cat = CategoryNode(name="base", weight=100, subjects=[s1], tests=[t2], subjects_weight=None) + with pytest.raises(ValueError, match="missing 'subjects_weight' for base"): + grader.process_category(cat) + +def test_balance_nodes_empty(grader): + # Should not break if there are no subjects/tests + cat = CategoryNode(name="base", weight=100) + result = grader.process_category(cat) + assert result.score == 0.0 + +def test_process_test_program_command_resolution(): + command_resolver = MagicMock() + command_resolver.resolve_command.return_value = "python3 main.py" + + grader = SubmissionGrader( + submission_files={}, + command_resolver=command_resolver, + submission_language="python" + ) + + tf = MockTestFunction() + t1 = TestNode(name="T1", test_function=tf, weight=100, parameters={'program_command': 'python {main}'}) + + cat = CategoryNode(name="base", weight=100, tests=[t1]) + grader.process_category(cat) + + command_resolver.resolve_command.assert_called_once_with('python {main}', 'python') + +def test_get_file_target_all(): + file1 = SubmissionFile(filename="file1.py", content="") + file2 = SubmissionFile(filename="file2.py", content="") + grader = SubmissionGrader( + submission_files={"file1.py": file1, "file2.py": file2}, + command_resolver=MagicMock() + ) + + tf = MockTestFunction() + t1 = TestNode(name="T1", test_function=tf, weight=100, file_target=["all"]) + + cat = CategoryNode(name="base", weight=100, tests=[t1]) + grader.process_category(cat) + # The MockTestFunction would need to record what files it received to assert it, + # but process_test will just call it. Let's just assert get_file_target directly. + target_files = grader.get_file_target(t1) + assert len(target_files) == 2 + +def test_get_file_target_specific(): + file1 = SubmissionFile(filename="file1.py", content="") + file2 = SubmissionFile(filename="file2.py", content="") + grader = SubmissionGrader( + submission_files={"file1.py": file1, "file2.py": file2}, + command_resolver=MagicMock() + ) + + tf = MockTestFunction() + t1 = TestNode(name="T1", test_function=tf, weight=100, file_target=["file1.py"]) + + target_files = grader.get_file_target(t1) + assert len(target_files) == 1 + assert target_files[0] is file1 diff --git a/tests/unit/services/test_sandbox_service.py b/tests/unit/services/test_sandbox_service.py new file mode 100644 index 00000000..c19353db --- /dev/null +++ b/tests/unit/services/test_sandbox_service.py @@ -0,0 +1,102 @@ +import pytest +from unittest.mock import MagicMock +from autograder.services.sandbox_service import SandboxService +from autograder.models.dataclass.submission import Submission +from sandbox_manager.models.sandbox_models import Language, ResponseCategory + +@pytest.fixture +def sandbox_service(): + return SandboxService() + +@pytest.fixture +def mock_sandbox_manager(mocker): + manager_mock = MagicMock() + mocker.patch("sandbox_manager.manager.get_sandbox_manager", return_value=manager_mock) + return manager_mock + +def test_create_sandbox_no_language(sandbox_service): + submission = Submission(username="test", user_id="1", assignment_id="1", language=None, submission_files={"main.py": "..."}) + with pytest.raises(ValueError, match="Submission language is required"): + sandbox_service.create_sandbox(submission) + +def test_create_sandbox_manager_exception(sandbox_service, mock_sandbox_manager): + submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) + mock_sandbox_manager.get_sandbox.side_effect = Exception("Pool error") + + result = sandbox_service.create_sandbox(submission) + assert result is None + +def test_create_sandbox_workdir_failure_releases_container(sandbox_service, mock_sandbox_manager): + submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) + mock_sandbox = MagicMock() + mock_sandbox.prepare_workdir.side_effect = IOError("Disk full") + mock_sandbox_manager.get_sandbox.return_value = mock_sandbox + + result = sandbox_service.create_sandbox(submission) + + assert result is None + # CRITICAL: Verify the container was not leaked + mock_sandbox_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox) + +def test_create_sandbox_success(sandbox_service, mock_sandbox_manager): + submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) + mock_sandbox = MagicMock() + mock_sandbox_manager.get_sandbox.return_value = mock_sandbox + + result = sandbox_service.create_sandbox(submission) + assert result == mock_sandbox + mock_sandbox.prepare_workdir.assert_called_once_with(submission.submission_files) + +def test_create_sandbox_success_no_files(sandbox_service, mock_sandbox_manager): + submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files=None) + mock_sandbox = MagicMock() + mock_sandbox_manager.get_sandbox.return_value = mock_sandbox + + result = sandbox_service.create_sandbox(submission) + assert result == mock_sandbox + mock_sandbox.prepare_workdir.assert_not_called() + +def test_release_sandbox_success(sandbox_service, mock_sandbox_manager): + mock_sandbox = MagicMock() + sandbox_service.release_sandbox(Language.PYTHON, mock_sandbox) + mock_sandbox_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox) + +def test_release_sandbox_exception_handled(sandbox_service, mock_sandbox_manager): + mock_sandbox = MagicMock() + mock_sandbox_manager.release_sandbox.side_effect = Exception("Cannot release") + # Should not raise + sandbox_service.release_sandbox(Language.PYTHON, mock_sandbox) + +def test_run_setup_command_no_sandbox(sandbox_service): + response = sandbox_service.run_setup_command(None, "echo hi") + assert response.category == ResponseCategory.SYSTEM_ERROR + assert "Sandbox environment is required" in response.stderr + +def test_run_setup_command_dict_missing_command(sandbox_service): + mock_sandbox = MagicMock() + response = sandbox_service.run_setup_command(mock_sandbox, {"name": "Install deps"}) + assert response.category == ResponseCategory.SYSTEM_ERROR + # Expect error about missing field + assert response.exit_code == -1 + +def test_run_setup_command_dict_success(sandbox_service): + mock_sandbox = MagicMock() + expected_response = MagicMock(category=ResponseCategory.SUCCESS) + mock_sandbox.run_command.return_value = expected_response + + response = sandbox_service.run_setup_command(mock_sandbox, {"name": "Install deps", "command": "pip install -r req.txt"}) + assert response == expected_response + mock_sandbox.run_command.assert_called_once_with("pip install -r req.txt") + +def test_run_setup_command_invalid_format(sandbox_service): + mock_sandbox = MagicMock() + response = sandbox_service.run_setup_command(mock_sandbox, 12345) + assert response.category == ResponseCategory.SYSTEM_ERROR + +def test_run_setup_command_execution_exception(sandbox_service): + mock_sandbox = MagicMock() + mock_sandbox.run_command.side_effect = Exception("Command crashed") + + response = sandbox_service.run_setup_command(mock_sandbox, "echo hi") + assert response.category == ResponseCategory.SYSTEM_ERROR + assert "Command crashed" in response.stderr From f5342f8fd2abaf0e02da74063fd8f7584596856c Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Thu, 28 May 2026 08:34:57 -0300 Subject: [PATCH 2/6] fix: replace pytest-mock with standard unittest.mock.patch to fix CI failure --- tests/unit/services/test_sandbox_service.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/unit/services/test_sandbox_service.py b/tests/unit/services/test_sandbox_service.py index c19353db..fabae44d 100644 --- a/tests/unit/services/test_sandbox_service.py +++ b/tests/unit/services/test_sandbox_service.py @@ -1,5 +1,5 @@ import pytest -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from autograder.services.sandbox_service import SandboxService from autograder.models.dataclass.submission import Submission from sandbox_manager.models.sandbox_models import Language, ResponseCategory @@ -9,10 +9,11 @@ def sandbox_service(): return SandboxService() @pytest.fixture -def mock_sandbox_manager(mocker): - manager_mock = MagicMock() - mocker.patch("sandbox_manager.manager.get_sandbox_manager", return_value=manager_mock) - return manager_mock +def mock_sandbox_manager(): + with patch("sandbox_manager.manager.get_sandbox_manager") as mock_get: + manager_mock = MagicMock() + mock_get.return_value = manager_mock + yield manager_mock def test_create_sandbox_no_language(sandbox_service): submission = Submission(username="test", user_id="1", assignment_id="1", language=None, submission_files={"main.py": "..."}) From 3dc467c6f620defc1c13e4c33474bde0e979296d Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Thu, 28 May 2026 08:40:40 -0300 Subject: [PATCH 3/6] fix: address pylint failures in new test files --- tests/unit/models/test_criteria_tree.py | 11 +++++++++-- tests/unit/services/test_sandbox_service.py | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/unit/models/test_criteria_tree.py b/tests/unit/models/test_criteria_tree.py index 90140c2f..ccb5ce0a 100644 --- a/tests/unit/models/test_criteria_tree.py +++ b/tests/unit/models/test_criteria_tree.py @@ -1,14 +1,15 @@ -import pytest from unittest.mock import MagicMock from autograder.models.criteria_tree import TestNode, SubjectNode, CategoryNode, CriteriaTree from autograder.models.abstract.test_function import TestFunction def create_mock_test_function(): + """Helper to create a mock test function.""" tf = MagicMock(spec=TestFunction) tf.name = "mock_test" return tf def test_test_node_repr(): + """Test TestNode representation.""" tf = create_mock_test_function() node = TestNode(name="my_test", test_function=tf) assert repr(node) == "TestNode(my_test)" @@ -23,6 +24,7 @@ def test_test_node_repr(): assert repr(node_with_both) == "TestNode(my_test, params={'p': 1}, file=['test.py'])" def test_subject_node_repr(): + """Test SubjectNode representation.""" tf = create_mock_test_function() test_node = TestNode(name="test", test_function=tf) subject_with_tests = SubjectNode(name="subject1", weight=50.0, tests=[test_node]) @@ -32,6 +34,7 @@ def test_subject_node_repr(): assert repr(subject_with_subjects) == "SubjectNode(subject2, weight=100.0, subjects=1)" def test_category_node_repr(): + """Test CategoryNode representation.""" tf = create_mock_test_function() test_node = TestNode(name="test", test_function=tf) category_with_tests = CategoryNode(name="cat1", weight=100.0, tests=[test_node]) @@ -42,6 +45,7 @@ def test_category_node_repr(): assert repr(category_with_subjects) == "CategoryNode(cat2, weight=100.0, subjects=1)" def test_criteria_tree_repr(): + """Test CriteriaTree representation.""" base = CategoryNode(name="base", weight=100.0) bonus = CategoryNode(name="bonus", weight=10.0) penalty = CategoryNode(name="penalty", weight=10.0) @@ -57,6 +61,7 @@ def test_criteria_tree_repr(): assert repr(tree_full) == "CriteriaTree(categories=['base', 'bonus', 'penalty'])" def test_subject_get_all_tests_recursive(): + """Test SubjectNode.get_all_tests recursively.""" tf = create_mock_test_function() t1 = TestNode(name="t1", test_function=tf) @@ -79,6 +84,7 @@ def test_subject_get_all_tests_recursive(): assert tests[2].name == "t3" def test_category_get_all_tests_recursive(): + """Test CategoryNode.get_all_tests recursively.""" tf = create_mock_test_function() t1 = TestNode(name="t1", test_function=tf) @@ -98,5 +104,6 @@ def test_category_get_all_tests_recursive(): assert names == ["t1", "t2", "t3", "t4"] def test_category_get_all_tests_no_tests(): + """Test CategoryNode.get_all_tests with no tests.""" cat = CategoryNode(name="empty", weight=100.0) - assert cat.get_all_tests() == [] + assert not cat.get_all_tests() diff --git a/tests/unit/services/test_sandbox_service.py b/tests/unit/services/test_sandbox_service.py index fabae44d..30046213 100644 --- a/tests/unit/services/test_sandbox_service.py +++ b/tests/unit/services/test_sandbox_service.py @@ -6,21 +6,25 @@ @pytest.fixture def sandbox_service(): + """Fixture for SandboxService.""" return SandboxService() @pytest.fixture def mock_sandbox_manager(): + """Fixture for mocked sandbox manager.""" with patch("sandbox_manager.manager.get_sandbox_manager") as mock_get: manager_mock = MagicMock() mock_get.return_value = manager_mock yield manager_mock def test_create_sandbox_no_language(sandbox_service): + """Test creating sandbox without language.""" submission = Submission(username="test", user_id="1", assignment_id="1", language=None, submission_files={"main.py": "..."}) with pytest.raises(ValueError, match="Submission language is required"): sandbox_service.create_sandbox(submission) def test_create_sandbox_manager_exception(sandbox_service, mock_sandbox_manager): + """Test manager exception during sandbox creation.""" submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) mock_sandbox_manager.get_sandbox.side_effect = Exception("Pool error") @@ -28,6 +32,7 @@ def test_create_sandbox_manager_exception(sandbox_service, mock_sandbox_manager) assert result is None def test_create_sandbox_workdir_failure_releases_container(sandbox_service, mock_sandbox_manager): + """Test releasing container if workdir preparation fails.""" submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) mock_sandbox = MagicMock() mock_sandbox.prepare_workdir.side_effect = IOError("Disk full") @@ -40,6 +45,7 @@ def test_create_sandbox_workdir_failure_releases_container(sandbox_service, mock mock_sandbox_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox) def test_create_sandbox_success(sandbox_service, mock_sandbox_manager): + """Test successful sandbox creation.""" submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files={"main.py": "..."}) mock_sandbox = MagicMock() mock_sandbox_manager.get_sandbox.return_value = mock_sandbox @@ -49,6 +55,7 @@ def test_create_sandbox_success(sandbox_service, mock_sandbox_manager): mock_sandbox.prepare_workdir.assert_called_once_with(submission.submission_files) def test_create_sandbox_success_no_files(sandbox_service, mock_sandbox_manager): + """Test sandbox creation with no files.""" submission = Submission(username="test", user_id="1", assignment_id="1", language=Language.PYTHON, submission_files=None) mock_sandbox = MagicMock() mock_sandbox_manager.get_sandbox.return_value = mock_sandbox @@ -58,22 +65,26 @@ def test_create_sandbox_success_no_files(sandbox_service, mock_sandbox_manager): mock_sandbox.prepare_workdir.assert_not_called() def test_release_sandbox_success(sandbox_service, mock_sandbox_manager): + """Test successful sandbox release.""" mock_sandbox = MagicMock() sandbox_service.release_sandbox(Language.PYTHON, mock_sandbox) mock_sandbox_manager.release_sandbox.assert_called_once_with(Language.PYTHON, mock_sandbox) def test_release_sandbox_exception_handled(sandbox_service, mock_sandbox_manager): + """Test exception handling during sandbox release.""" mock_sandbox = MagicMock() mock_sandbox_manager.release_sandbox.side_effect = Exception("Cannot release") # Should not raise sandbox_service.release_sandbox(Language.PYTHON, mock_sandbox) def test_run_setup_command_no_sandbox(sandbox_service): + """Test running setup command without sandbox.""" response = sandbox_service.run_setup_command(None, "echo hi") assert response.category == ResponseCategory.SYSTEM_ERROR assert "Sandbox environment is required" in response.stderr def test_run_setup_command_dict_missing_command(sandbox_service): + """Test running setup command with missing command in dict.""" mock_sandbox = MagicMock() response = sandbox_service.run_setup_command(mock_sandbox, {"name": "Install deps"}) assert response.category == ResponseCategory.SYSTEM_ERROR @@ -81,6 +92,7 @@ def test_run_setup_command_dict_missing_command(sandbox_service): assert response.exit_code == -1 def test_run_setup_command_dict_success(sandbox_service): + """Test successful setup command execution from dict.""" mock_sandbox = MagicMock() expected_response = MagicMock(category=ResponseCategory.SUCCESS) mock_sandbox.run_command.return_value = expected_response @@ -90,11 +102,13 @@ def test_run_setup_command_dict_success(sandbox_service): mock_sandbox.run_command.assert_called_once_with("pip install -r req.txt") def test_run_setup_command_invalid_format(sandbox_service): + """Test setup command with invalid format.""" mock_sandbox = MagicMock() response = sandbox_service.run_setup_command(mock_sandbox, 12345) assert response.category == ResponseCategory.SYSTEM_ERROR def test_run_setup_command_execution_exception(sandbox_service): + """Test setup command execution exception.""" mock_sandbox = MagicMock() mock_sandbox.run_command.side_effect = Exception("Command crashed") From f2ce54ed8e0b31b0ff3769b9dcc48c0502b95bf9 Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Thu, 28 May 2026 08:45:42 -0300 Subject: [PATCH 4/6] fix: set PYTHONPATH in pylint workflow to resolve import errors --- .github/workflows/pylint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pylint.yml b/.github/workflows/pylint.yml index bdffaaef..d2d12aa1 100644 --- a/.github/workflows/pylint.yml +++ b/.github/workflows/pylint.yml @@ -32,4 +32,4 @@ jobs: - name: Analysing the code with pylint if: steps.changed-files.outputs.files != '' run: | - pylint ${{ steps.changed-files.outputs.files }} + PYTHONPATH=. pylint ${{ steps.changed-files.outputs.files }} From 89db551b9db2dc881618a3bc6bf99f1be3896318 Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Sat, 30 May 2026 09:56:32 -0300 Subject: [PATCH 5/6] chore: fix pylint errors and improve code quality - Add missing docstrings to core models and services. - Fix critical pylint errors (missing arguments, not-callable false positives). - Move imports to top level where possible. - Address broad exception catching with selective pylint-disable. - Fix signatures of abstract methods for consistency. - Refactor sandbox and pre-flight services for better adherence to standards. - Improve logging with lazy formatting. - Resolve various minor warnings (unused imports, reimports, etc.). --- autograder/autograder.py | 13 ++++++------- autograder/models/abstract/ai_test_function.py | 11 +++++++---- .../models/abstract/criteria_tree_processer.py | 9 ++++++--- autograder/models/abstract/step.py | 7 ++++--- autograder/models/config/category.py | 1 + autograder/models/config/criteria.py | 2 +- autograder/models/config/setup.py | 2 ++ autograder/models/config/subject.py | 1 + autograder/models/config/test.py | 2 +- autograder/models/criteria_tree.py | 2 ++ autograder/models/dataclass/focus.py | 3 ++- autograder/models/dataclass/grading_result.py | 1 + autograder/models/dataclass/submission.py | 4 +++- autograder/models/dataclass/test_result.py | 1 + autograder/models/pipeline_execution.py | 3 +-- autograder/services/assets/cache_manager.py | 7 +++++-- autograder/services/assets/provider.py | 4 +++- autograder/services/assets/resolver.py | 3 +++ autograder/services/assets/s3_provider.py | 5 ++++- autograder/services/focus_service.py | 8 +++++++- autograder/services/pre_flight_service.py | 8 +++++--- autograder/services/sandbox_service.py | 13 ++++++------- autograder/steps/load_template_step.py | 8 ++++++-- autograder/steps/pre_flight_step.py | 8 +++----- autograder/steps/structural_analysis_step.py | 6 +++--- github_action/github_action_service.py | 2 +- web/database/models/submission.py | 2 +- web/database/models/submission_result.py | 2 +- 28 files changed, 87 insertions(+), 51 deletions(-) diff --git a/autograder/autograder.py b/autograder/autograder.py index 83f8ba3e..5b01f8df 100644 --- a/autograder/autograder.py +++ b/autograder/autograder.py @@ -1,11 +1,13 @@ import logging +from sandbox_manager.manager import get_sandbox_manager from autograder.models.abstract.step import Step from autograder.models.dataclass.step_result import StepName from autograder.models.pipeline_execution import PipelineExecution, PipelineStatus from autograder.steps.step_registry import StepRegistry from autograder.models.dataclass.submission import Submission from autograder.services.template_library_service import TemplateLibraryService +from autograder.steps.load_template_step import TemplateLoaderStep logger = logging.getLogger(__name__) @@ -74,7 +76,7 @@ def run(self, submission: Submission): ) break logger.info("Step %s completed successfully (external_user_id=%s)", step_name, submission.user_id) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught pipeline_execution.status = PipelineStatus.INTERRUPTED logger.error( "Unhandled exception in step %s (external_user_id=%s): %s", @@ -84,7 +86,6 @@ def run(self, submission: Submission): exc_info=True, ) break - pipeline_execution.finish_execution() # Generates GradingResult object in pipeline execution # Cleanup: Destroy sandbox if it was created @@ -103,7 +104,6 @@ def _cleanup_sandbox(self, pipeline_execution: PipelineExecution) -> None: try: sandbox = pipeline_execution.sandbox if sandbox: - from sandbox_manager.manager import get_sandbox_manager manager = get_sandbox_manager() language = pipeline_execution.submission.language manager.destroy_sandbox(language, sandbox) @@ -113,7 +113,7 @@ def _cleanup_sandbox(self, pipeline_execution: PipelineExecution) -> None: pipeline_execution.submission.user_id, language.value if language else "none", ) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught # Log error but don't fail the pipeline logger.warning( "Failed to cleanup sandbox (external_user_id=%s): %s", @@ -122,7 +122,7 @@ def _cleanup_sandbox(self, pipeline_execution: PipelineExecution) -> None: ) -def build_pipeline( +def build_pipeline( # pylint: disable=too-many-arguments,too-many-locals template_name, include_feedback, grading_criteria, @@ -159,8 +159,7 @@ def build_pipeline( templates.append(template_service.load_custom_template(custom_template)) elif template_name: # Normalize template names (can be string, comma-separated string, or list) - from autograder.steps.load_template_step import TemplateLoaderStep - names = TemplateLoaderStep._normalize_template_names(template_name) + names = TemplateLoaderStep.normalize_template_names(template_name) for name in names: templates.append(template_service.load_builtin_template(name)) diff --git a/autograder/models/abstract/ai_test_function.py b/autograder/models/abstract/ai_test_function.py index 116b6bcc..6daea5a8 100644 --- a/autograder/models/abstract/ai_test_function.py +++ b/autograder/models/abstract/ai_test_function.py @@ -4,6 +4,7 @@ from autograder.models.abstract.test_function import TestFunction from autograder.models.dataclass.submission import SubmissionFile from autograder.models.dataclass.test_result import TestResult +from autograder.utils.executors.ai_executor import AiExecutor, TestInput from sandbox_manager.sandbox_container import SandboxContainer @@ -35,6 +36,7 @@ def execute( self, files: Optional[List[SubmissionFile]], sandbox: Optional[SandboxContainer], + *args, **kwargs, ) -> TestResult: """ @@ -45,15 +47,18 @@ def execute( test name in that dict and returns the result directly. If the dict is absent (standalone usage), it falls back to ``_run_single()``. """ + # args is not used in AI tests as parameters are in kwargs + _ = args pre_computed: Optional[Dict[str, TestResult]] = kwargs.get("pre_computed_results") if pre_computed is not None and self.name in pre_computed: return pre_computed[self.name] - return self._run_single(files, **kwargs) + return self._run_single(files, *args, **kwargs) def _run_single( self, files: Optional[List[SubmissionFile]], + *args, **kwargs, ) -> TestResult: """ @@ -62,9 +67,7 @@ def _run_single( Used when ``pre_computed_results`` is not available (e.g. unit tests, standalone runners, or pipelines that do not include ``AiBatchStep``). """ - # Local import to avoid circular dependencies. - from autograder.utils.executors.ai_executor import AiExecutor, TestInput - + _ = args locale: str = kwargs.get("locale", "en") prompt = self.build_prompt(files, **kwargs) submission_files = {f.filename: f.content for f in files} if files else {} diff --git a/autograder/models/abstract/criteria_tree_processer.py b/autograder/models/abstract/criteria_tree_processer.py index 8ee966f4..9224f78f 100644 --- a/autograder/models/abstract/criteria_tree_processer.py +++ b/autograder/models/abstract/criteria_tree_processer.py @@ -6,14 +6,17 @@ class CriteriaTreeProcesser(ABC): + """ + Abstract base class for processing the criteria tree structure. + """ @abstractmethod def process_subject(self, subject: "SubjectNode") -> Any: - pass + """Process a subject node in the criteria tree.""" @abstractmethod def process_test(self, test: "TestNode") -> Any: - pass + """Process a test node in the criteria tree.""" @abstractmethod def process_category(self, category: "CategoryNode") -> Any: - pass + """Process a category node in the criteria tree.""" diff --git a/autograder/models/abstract/step.py b/autograder/models/abstract/step.py index 3d0c00db..86deb0ee 100644 --- a/autograder/models/abstract/step.py +++ b/autograder/models/abstract/step.py @@ -9,11 +9,13 @@ class Step(ABC): + """ + Abstract base class for all pipeline steps. + """ @property @abstractmethod def step_name(self) -> StepName: """Return the name of the step (e.g. StepName.GRADE).""" - pass def execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: """ @@ -24,7 +26,7 @@ def execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: locale = pipeline_exec.locale try: return self._execute(pipeline_exec) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.exception("Step %s was interrupted: %s", self.step_name.value, str(e)) error_msg = t("system.error.step_interrupted", locale=locale, error=str(e)) @@ -42,5 +44,4 @@ def execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: @abstractmethod def _execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: """Internal execution logic to be implemented by subclasses.""" - pass diff --git a/autograder/models/config/category.py b/autograder/models/config/category.py index 824118fc..4f0be328 100644 --- a/autograder/models/config/category.py +++ b/autograder/models/config/category.py @@ -6,6 +6,7 @@ class CategoryConfig(BaseModel): + """Configuration for a grading category (base, bonus, or penalty).""" weight: float = Field( ..., ge=0, le=100, description="Weight of this category (0-100)" ) diff --git a/autograder/models/config/criteria.py b/autograder/models/config/criteria.py index 62a69e1c..58a82870 100644 --- a/autograder/models/config/criteria.py +++ b/autograder/models/config/criteria.py @@ -9,7 +9,7 @@ class CriteriaConfig(BaseModel): test_library: Optional[str] = Field( None, description="Name of the test library/template to use" - ) # TODO -> Remove this attribute (it already sits in grading config) + ) base: CategoryConfig = Field(..., description="Base grading criteria (required)") bonus: Optional[CategoryConfig] = Field(None, description="Bonus points criteria") penalty: Optional[CategoryConfig] = Field(None, description="Penalty criteria") diff --git a/autograder/models/config/setup.py b/autograder/models/config/setup.py index a6e02982..d57564b9 100644 --- a/autograder/models/config/setup.py +++ b/autograder/models/config/setup.py @@ -11,6 +11,7 @@ class AssetConfig(BaseModel): @field_validator('source') @classmethod def validate_source(cls, v: str) -> str: + """Validate the source path of the asset.""" if not v: raise ValueError("source must not be empty") if v.startswith('/'): @@ -22,6 +23,7 @@ def validate_source(cls, v: str) -> str: @field_validator('target') @classmethod def validate_target(cls, v: str) -> str: + """Validate the target path of the asset.""" if not v: raise ValueError("target must not be empty") if not v.startswith('/'): diff --git a/autograder/models/config/subject.py b/autograder/models/config/subject.py index eb7ccc5c..6b0a3da2 100644 --- a/autograder/models/config/subject.py +++ b/autograder/models/config/subject.py @@ -4,6 +4,7 @@ class SubjectConfig(BaseModel): + """Configuration for a subject within a grading category.""" subject_name: str = Field(..., description="Name of the subject") weight: float = Field( ..., ge=0, le=100, description="Weight of this subject (0-100)" diff --git a/autograder/models/config/test.py b/autograder/models/config/test.py index 779a6a26..b25e537f 100644 --- a/autograder/models/config/test.py +++ b/autograder/models/config/test.py @@ -39,7 +39,7 @@ def get_args_list(self) -> List[Any]: def get_kwargs_dict(self) -> Dict[str, Any]: """Convert named parameters to keyword arguments dictionary.""" kwargs = {} - if self.parameters: + if self.parameters is not None: kwargs.update({param.name: param.value for param in self.parameters}) if self.model_extra: diff --git a/autograder/models/criteria_tree.py b/autograder/models/criteria_tree.py index 96589290..e8a548b4 100644 --- a/autograder/models/criteria_tree.py +++ b/autograder/models/criteria_tree.py @@ -58,6 +58,7 @@ def __repr__(self): ) def get_all_tests(self) -> List[TestNode]: + """Recursively collect all test nodes under this subject.""" tests = [*self.tests] for subject in self.subjects: tests.extend(subject.get_all_tests()) @@ -88,6 +89,7 @@ def __repr__(self): ) def add_subjects(self, subjects: List[SubjectNode]) -> None: + """Add a list of subjects to this category.""" self.subjects.extend(subjects) def get_all_tests(self) -> List[TestNode]: diff --git a/autograder/models/dataclass/focus.py b/autograder/models/dataclass/focus.py index 4ea813be..5885f0a7 100644 --- a/autograder/models/dataclass/focus.py +++ b/autograder/models/dataclass/focus.py @@ -6,6 +6,7 @@ @dataclass class FocusedTest: + """Represents a test result that has been prioritized for feedback.""" test_result: TestResultNode diff_score: float @@ -19,6 +20,7 @@ def to_dict(self) -> Dict[str, Any]: @dataclass class Focus: + """Organizes prioritized tests by category (base, penalty, bonus).""" base: List[FocusedTest] penalty: Optional[List[FocusedTest]] bonus: Optional[List[FocusedTest]] @@ -30,4 +32,3 @@ def to_dict(self) -> Dict[str, Any]: "penalty": [test.to_dict() for test in self.penalty] if self.penalty else None, "bonus": [test.to_dict() for test in self.bonus] if self.bonus else None } - diff --git a/autograder/models/dataclass/grading_result.py b/autograder/models/dataclass/grading_result.py index 19d9bf97..00db2685 100644 --- a/autograder/models/dataclass/grading_result.py +++ b/autograder/models/dataclass/grading_result.py @@ -6,6 +6,7 @@ @dataclass class GradingResult: + """Holds the final results of a grading execution.""" final_score: float #status: str I'll evaluate if we keep this attribute or not feedback: Optional[str] = None diff --git a/autograder/models/dataclass/submission.py b/autograder/models/dataclass/submission.py index 15166a30..cc328dde 100644 --- a/autograder/models/dataclass/submission.py +++ b/autograder/models/dataclass/submission.py @@ -4,14 +4,16 @@ @dataclass class SubmissionFile: + """Represents a single file in a submission.""" filename: str content: str @dataclass class Submission: + """Represents a student's submission for an assignment.""" username: str user_id: int assignment_id: int submission_files: Dict[str,SubmissionFile] language: Optional[Language] = None - locale: str = "en" \ No newline at end of file + locale: str = "en" diff --git a/autograder/models/dataclass/test_result.py b/autograder/models/dataclass/test_result.py index e4fff000..f17615e3 100644 --- a/autograder/models/dataclass/test_result.py +++ b/autograder/models/dataclass/test_result.py @@ -17,6 +17,7 @@ def get_result(self): return [self] def to_dict(self) -> dict: + """Convert TestResult to dictionary for JSON serialization.""" return { "test_name": self.test_name, "score": self.score, diff --git a/autograder/models/pipeline_execution.py b/autograder/models/pipeline_execution.py index 3e82aeb3..35bb2eeb 100644 --- a/autograder/models/pipeline_execution.py +++ b/autograder/models/pipeline_execution.py @@ -6,13 +6,13 @@ from autograder.models.dataclass.grading_result import GradingResult from autograder.models.dataclass.step_result import StepResult, StepName, StepStatus from autograder.models.dataclass.submission import Submission +from autograder.models.dataclass.structural_analysis_result import StructuralAnalysisResult if TYPE_CHECKING: from autograder.models.abstract.template import Template from autograder.models.criteria_tree import CriteriaTree from autograder.models.dataclass.focus import Focus from autograder.models.dataclass.grade_step_result import GradeStepResult - from autograder.models.dataclass.structural_analysis_result import StructuralAnalysisResult from autograder.models.result_tree import ResultTree from sandbox_manager.sandbox_container import SandboxContainer @@ -133,7 +133,6 @@ def get_structural_analysis_result(self) -> Optional["StructuralAnalysisResult"] """ if not self.has_step_result(StepName.STRUCTURAL_ANALYSIS): return None - from autograder.models.dataclass.structural_analysis_result import StructuralAnalysisResult return cast( StructuralAnalysisResult, self._require_step_data(StepName.STRUCTURAL_ANALYSIS, "structural analysis result"), diff --git a/autograder/services/assets/cache_manager.py b/autograder/services/assets/cache_manager.py index 931bec63..56ad5946 100644 --- a/autograder/services/assets/cache_manager.py +++ b/autograder/services/assets/cache_manager.py @@ -7,6 +7,9 @@ logger = logging.getLogger("AssetCacheManager") class AssetCacheManager: + """ + Manages caching of assets in memory and on disk. + """ def __init__(self, in_memory_limit: int = 0, disk_cache_dir: str = "/tmp/autograder_assets_cache"): """ Initialize the asset cache manager. @@ -48,7 +51,7 @@ def get(self, asset_key: str) -> Optional[bytes]: self._add_to_memory_cache(asset_key, content) return content - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.error("Failed to read asset from disk cache: %s", str(e)) return None @@ -63,7 +66,7 @@ def put(self, asset_key: str, content: bytes) -> None: with open(disk_path, "wb") as f: f.write(content) logger.debug("Stored asset %s in disk cache", asset_key) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.error("Failed to write asset to disk cache: %s", str(e)) # Save to memory if enabled diff --git a/autograder/services/assets/provider.py b/autograder/services/assets/provider.py index ed6b054e..a99898f8 100644 --- a/autograder/services/assets/provider.py +++ b/autograder/services/assets/provider.py @@ -2,6 +2,9 @@ from typing import Optional class AssetProvider(ABC): + """ + Abstract base class for asset providers. + """ @abstractmethod def get_asset(self, source: str, target: str, read_only: bool = True) -> Optional[bytes]: """ @@ -15,4 +18,3 @@ def get_asset(self, source: str, target: str, read_only: bool = True) -> Optiona Returns: Raw content as bytes, or None if not found or on error. """ - ... diff --git a/autograder/services/assets/resolver.py b/autograder/services/assets/resolver.py index b09fb6ea..733ef805 100644 --- a/autograder/services/assets/resolver.py +++ b/autograder/services/assets/resolver.py @@ -9,6 +9,9 @@ logger = logging.getLogger("AssetSourceResolver") class AssetSourceResolver: + """ + Resolves asset configurations into actual asset content using providers and caches. + """ def __init__(self): in_memory_limit = int(os.getenv("EXTERNAL_ASSETS_IN_MEMORY_CACHE_LIMIT", "100")) self.cache_manager = AssetCacheManager(in_memory_limit=in_memory_limit) diff --git a/autograder/services/assets/s3_provider.py b/autograder/services/assets/s3_provider.py index 4f3b3a5c..9c26422c 100644 --- a/autograder/services/assets/s3_provider.py +++ b/autograder/services/assets/s3_provider.py @@ -9,6 +9,9 @@ logger = logging.getLogger("S3AssetProvider") class S3AssetProvider(AssetProvider): + """ + Asset provider that fetches files from Amazon S3. + """ def __init__(self, cache_manager: AssetCacheManager): self.cache_manager = cache_manager @@ -53,6 +56,6 @@ def get_asset(self, source: str, target: str, read_only: bool = True) -> Optiona self.cache_manager.put(cache_key, raw_content) return raw_content - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.error("Failed to fetch asset from S3 (%s): %s", source, str(e)) return None diff --git a/autograder/services/focus_service.py b/autograder/services/focus_service.py index 263edd3b..ea3b13bf 100644 --- a/autograder/services/focus_service.py +++ b/autograder/services/focus_service.py @@ -9,6 +9,9 @@ class FocusService: + """ + Service responsible for identifying the most impactful tests for student feedback. + """ def __calculate_impact( self, test: TestResultNode, cumulative_multiplier: float ) -> float: @@ -25,7 +28,7 @@ def __calculate_impact( def __process_subject( self, subject: SubjectResultNode, parent_multiplier: float ) -> List[FocusedTest]: - focused_tests = list() + focused_tests = [] # Determine the multiplier for children of this subject # If this subject has sub-subjects and tests, we might need to split the weight @@ -98,6 +101,9 @@ def __process_category(self, category: CategoryResultNode) -> List[FocusedTest]: return focused_tests def find(self, result_tree: ResultTree) -> Focus: + """ + Find and prioritize impactful tests from the result tree. + """ return Focus( base=self.__process_category(result_tree.root.base), penalty=self.__process_category(result_tree.root.penalty) diff --git a/autograder/services/pre_flight_service.py b/autograder/services/pre_flight_service.py index ea40f7bb..f5a03010 100644 --- a/autograder/services/pre_flight_service.py +++ b/autograder/services/pre_flight_service.py @@ -1,7 +1,6 @@ import logging from typing import List, Optional, Union from autograder.models.dataclass.preflight_error import PreflightError, PreflightCheckType -from autograder.models.dataclass.submission import Submission from autograder.translations import t from autograder.services.sandbox_service import SandboxService from sandbox_manager.sandbox_container import SandboxContainer @@ -10,7 +9,10 @@ from autograder.models.config.setup import SetupConfig, LanguageSetupConfig -class PreFlightService: +class PreFlightService: # pylint: disable=too-many-instance-attributes + """ + Service responsible for executing pre-flight checks (required files, setup commands). + """ def __init__(self, setup_config: Optional[Union[SetupConfig, dict]], submission_language: Language, locale: str = "en"): """ Initialize PreFlightService with language-specific setup configuration. @@ -59,7 +61,7 @@ def _resolve_setup_config(self, setup_config: SetupConfig, submission_language: raw_extra = setup_config.model_extra[lang_key] if isinstance(raw_extra, dict): return LanguageSetupConfig(**raw_extra) - elif isinstance(raw_extra, LanguageSetupConfig): + if isinstance(raw_extra, LanguageSetupConfig): return raw_extra self.logger.warning("No setup config found for language %s, using empty config", lang_key) diff --git a/autograder/services/sandbox_service.py b/autograder/services/sandbox_service.py index e7a47f49..1df60283 100644 --- a/autograder/services/sandbox_service.py +++ b/autograder/services/sandbox_service.py @@ -1,5 +1,6 @@ import logging -from typing import List, Optional, Any +from typing import Optional, Any +from sandbox_manager.manager import get_sandbox_manager from autograder.models.dataclass.submission import Submission from sandbox_manager.sandbox_container import SandboxContainer from sandbox_manager.models.sandbox_models import Language, ResponseCategory, CommandResponse @@ -27,7 +28,6 @@ def create_sandbox(self, submission: Submission) -> Optional[SandboxContainer]: raise ValueError("Submission language is required for sandbox creation") try: - from sandbox_manager.manager import get_sandbox_manager sandbox_manager = get_sandbox_manager() sandbox = sandbox_manager.get_sandbox(submission.language) self.logger.debug("Sandbox created for language %s", submission.language) @@ -37,14 +37,14 @@ def create_sandbox(self, submission: Submission) -> Optional[SandboxContainer]: try: sandbox.prepare_workdir(submission.submission_files) self.logger.debug("Workdir prepared with %s files", len(submission.submission_files)) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught self.logger.error("Failed to prepare workdir in sandbox: %s", str(e)) # Release the sandbox back to pool since it's unusable sandbox_manager.release_sandbox(submission.language, sandbox) return None return sandbox - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught self.logger.error("Failed to create sandbox for language %s: %s", submission.language, str(e)) return None @@ -53,11 +53,10 @@ def release_sandbox(self, language: Language, sandbox: SandboxContainer): Releases a sandbox back to the manager pool. """ try: - from sandbox_manager.manager import get_sandbox_manager sandbox_manager = get_sandbox_manager() sandbox_manager.release_sandbox(language, sandbox) self.logger.info("Sandbox released for language %s", language) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught self.logger.warning("Failed to release sandbox: %s", str(e)) def run_setup_command(self, sandbox: SandboxContainer, command_spec: Any, idx: int = 0, locale: Optional[str] = None) -> CommandResponse: @@ -118,7 +117,7 @@ def run_setup_command(self, sandbox: SandboxContainer, command_spec: Any, idx: i self.logger.debug("Executing setup command '%s': %s", command_name, command) try: return sandbox.run_command(command) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught error_msg = t("preflight.error.setup_command_failed_execution", locale=locale, command_name=command_name, error=str(e), command=command) self.logger.error(error_msg) return CommandResponse( diff --git a/autograder/steps/load_template_step.py b/autograder/steps/load_template_step.py index 4b60daf6..6662c386 100644 --- a/autograder/steps/load_template_step.py +++ b/autograder/steps/load_template_step.py @@ -18,7 +18,7 @@ def __init__(self, template_name: Union[str, List[str], None], custom_template=N """ Initialize the template loader step. """ - self._template_names = self._normalize_template_names(template_name) + self._template_names = self.normalize_template_names(template_name) self._custom_template = custom_template self._template_service = TemplateLibraryService.get_instance() self._templates = templates @@ -29,7 +29,11 @@ def __init__(self, template_name: Union[str, List[str], None], custom_template=N ) @staticmethod - def _normalize_template_names(template_name: Union[str, List[str], None]) -> List[str]: + def normalize_template_names(template_name: Union[str, List[str], None]) -> List[str]: + """ + Normalize template names into a list of strings. + Input can be a single string, a comma-separated string, or a list of strings. + """ if template_name is None: return [] diff --git a/autograder/steps/pre_flight_step.py b/autograder/steps/pre_flight_step.py index 94886fed..bd42cfc5 100644 --- a/autograder/steps/pre_flight_step.py +++ b/autograder/steps/pre_flight_step.py @@ -1,18 +1,16 @@ import logging -from typing import List from autograder.models.abstract.step import Step from autograder.models.pipeline_execution import PipelineExecution from autograder.models.dataclass.step_result import StepResult, StepName from autograder.services.pre_flight_service import PreFlightService from autograder.translations import t +from autograder.models.config.setup import SetupConfig +from autograder.services.assets.resolver import AssetSourceResolver logger = logging.getLogger(__name__) -from autograder.models.config.setup import SetupConfig -from autograder.services.assets.resolver import AssetSourceResolver - class PreFlightStep(Step): """ The Pre-flight step is responsible for: @@ -78,7 +76,7 @@ def _execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: try: resolved_assets = self._asset_resolver.resolve_assets(self._setup_config.assets) sandbox.inject_assets(resolved_assets) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught error_msg = f"Failed to inject assets: {str(e)}" logger.error("Asset injection failed (external_user_id=%s): %s", pipeline_exec.submission.user_id, error_msg) return pipeline_exec.add_step_result(StepResult.fail( diff --git a/autograder/steps/structural_analysis_step.py b/autograder/steps/structural_analysis_step.py index e530e71d..0d64413e 100644 --- a/autograder/steps/structural_analysis_step.py +++ b/autograder/steps/structural_analysis_step.py @@ -56,7 +56,7 @@ def _execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: ast_grep_lang = self._map_language(language) if not ast_grep_lang: - logger.warning(f"Language {language.value} is not supported by ast-grep; skipping.") + logger.warning("Language %s is not supported by ast-grep; skipping.", language.value) return pipeline_exec.add_step_result( StepResult.success( self.step_name, @@ -76,8 +76,8 @@ def _execute(self, pipeline_exec: PipelineExecution) -> PipelineExecution: try: roots[filename] = SgRoot(sub_file.content, ast_grep_lang) - except Exception as e: - logger.warning(f"Failed to parse {filename} with ast-grep: {e}") + except Exception as e: # pylint: disable=broad-exception-caught + logger.warning("Failed to parse %s with ast-grep: %s", filename, str(e)) roots[filename] = None result = StructuralAnalysisResult(roots=roots, available=True) diff --git a/github_action/github_action_service.py b/github_action/github_action_service.py index 77dee1f9..19e3c228 100644 --- a/github_action/github_action_service.py +++ b/github_action/github_action_service.py @@ -301,7 +301,7 @@ def autograder_pipeline_from_cloud( exporter = CloudExporter(client, config_db_id, language, student_name) self._cloud_exporter = exporter - self._submission_language = Language(language) + self._submission_language = Language(language) # pylint: disable=no-value-for-parameter self._locale = locale return build_pipeline( diff --git a/web/database/models/submission.py b/web/database/models/submission.py index 553ef4b1..746cfec0 100644 --- a/web/database/models/submission.py +++ b/web/database/models/submission.py @@ -39,7 +39,7 @@ class Submission(Base): nullable=False, index=True ) - submitted_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), nullable=False, index=True) + submitted_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), nullable=False, index=True) # pylint: disable=not-callable graded_at: Mapped[Optional[datetime]] = mapped_column(DateTime, nullable=True) submission_metadata: Mapped[Optional[dict]] = mapped_column(JSON, nullable=True) diff --git a/web/database/models/submission_result.py b/web/database/models/submission_result.py index 364f9684..ba7655cd 100644 --- a/web/database/models/submission_result.py +++ b/web/database/models/submission_result.py @@ -40,7 +40,7 @@ class SubmissionResult(Base): ) error_message: Mapped[Optional[str]] = mapped_column(Text, nullable=True) failed_at_step: Mapped[Optional[str]] = mapped_column(Text, nullable=True) - created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), nullable=False) + created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), nullable=False) # pylint: disable=not-callable # Relationships submission: Mapped["Submission"] = relationship("Submission", back_populates="result") From 305de7acf532ac8fd8ebe5adada76196c59be603 Mon Sep 17 00:00:00 2001 From: ArthurCRodrigues Date: Sat, 30 May 2026 10:08:55 -0300 Subject: [PATCH 6/6] fix: resolve test failures by reverting circular import changes and updating AI test mocks --- autograder/autograder.py | 4 ++-- autograder/services/sandbox_service.py | 3 ++- tests/unit/pipeline/test_ai_batch_step.py | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/autograder/autograder.py b/autograder/autograder.py index 5b01f8df..8ec924d0 100644 --- a/autograder/autograder.py +++ b/autograder/autograder.py @@ -1,13 +1,11 @@ import logging -from sandbox_manager.manager import get_sandbox_manager from autograder.models.abstract.step import Step from autograder.models.dataclass.step_result import StepName from autograder.models.pipeline_execution import PipelineExecution, PipelineStatus from autograder.steps.step_registry import StepRegistry from autograder.models.dataclass.submission import Submission from autograder.services.template_library_service import TemplateLibraryService -from autograder.steps.load_template_step import TemplateLoaderStep logger = logging.getLogger(__name__) @@ -104,6 +102,7 @@ def _cleanup_sandbox(self, pipeline_execution: PipelineExecution) -> None: try: sandbox = pipeline_execution.sandbox if sandbox: + from sandbox_manager.manager import get_sandbox_manager manager = get_sandbox_manager() language = pipeline_execution.submission.language manager.destroy_sandbox(language, sandbox) @@ -159,6 +158,7 @@ def build_pipeline( # pylint: disable=too-many-arguments,too-many-locals templates.append(template_service.load_custom_template(custom_template)) elif template_name: # Normalize template names (can be string, comma-separated string, or list) + from autograder.steps.load_template_step import TemplateLoaderStep names = TemplateLoaderStep.normalize_template_names(template_name) for name in names: templates.append(template_service.load_builtin_template(name)) diff --git a/autograder/services/sandbox_service.py b/autograder/services/sandbox_service.py index 1df60283..396b44f6 100644 --- a/autograder/services/sandbox_service.py +++ b/autograder/services/sandbox_service.py @@ -1,6 +1,5 @@ import logging from typing import Optional, Any -from sandbox_manager.manager import get_sandbox_manager from autograder.models.dataclass.submission import Submission from sandbox_manager.sandbox_container import SandboxContainer from sandbox_manager.models.sandbox_models import Language, ResponseCategory, CommandResponse @@ -28,6 +27,7 @@ def create_sandbox(self, submission: Submission) -> Optional[SandboxContainer]: raise ValueError("Submission language is required for sandbox creation") try: + from sandbox_manager.manager import get_sandbox_manager sandbox_manager = get_sandbox_manager() sandbox = sandbox_manager.get_sandbox(submission.language) self.logger.debug("Sandbox created for language %s", submission.language) @@ -53,6 +53,7 @@ def release_sandbox(self, language: Language, sandbox: SandboxContainer): Releases a sandbox back to the manager pool. """ try: + from sandbox_manager.manager import get_sandbox_manager sandbox_manager = get_sandbox_manager() sandbox_manager.release_sandbox(language, sandbox) self.logger.info("Sandbox released for language %s", language) diff --git a/tests/unit/pipeline/test_ai_batch_step.py b/tests/unit/pipeline/test_ai_batch_step.py index 57a5921d..b6c70717 100644 --- a/tests/unit/pipeline/test_ai_batch_step.py +++ b/tests/unit/pipeline/test_ai_batch_step.py @@ -127,7 +127,7 @@ def test_fallback_called_when_no_precomputed(self): ) with patch( - "autograder.utils.executors.ai_executor.AiExecutor" + "autograder.models.abstract.ai_test_function.AiExecutor" ) as mock_executor: mock_executor.return_value.run.return_value = {"ai_code_review": fallback_result} result = func.execute(files=[], sandbox=None) @@ -139,7 +139,7 @@ def test_fallback_returns_zero_result_on_empty_api_response(self): func = _ConcreteAiTest() with patch( - "autograder.utils.executors.ai_executor.AiExecutor" + "autograder.models.abstract.ai_test_function.AiExecutor" ) as mock_executor: mock_executor.return_value.run.return_value = {} result = func.execute(files=None, sandbox=None)