Autotest fixes (#6907)

* Fix autotest settings validation bug when criterion is null

* Ensure autotested criteria do not have marks set if tests time out

Changelog.md

@@ -3,6 +3,9 @@
 ## [unreleased]
 - Fix import bug that prevents the peer review table from loading (#6908)
 
+- Fix autotest settings criterion JSON schema validation (#6907)
+- Ensure autotested criteria do not have marks set if tests time out (#6907)
+
 ## [v2.4.2]
 - Fix feedback file API not returning feedback file IDs correctly (#6875)
 - Allow inactive groups in the graders table to be toggled for display (#6778)

app/assets/javascripts/Components/autotest_manager.jsx

@@ -36,11 +36,6 @@ class AutotestManager extends React.Component {
                 category: {
                   "ui:title": I18n.t("automated_tests.category"),
                 },
-                extra_info: {
-                  criterion: {
-                    "ui:enumDisabled": [""],
-                  },
-                },
                 feedback_file_names: {
                   "ui:classNames": "feedback-file-names",
                   "ui:options": {orderable: false},

app/helpers/automated_tests_helper.rb

@@ -2,7 +2,8 @@
 
 module AutomatedTestsHelper
   def extra_test_group_schema(assignment)
-    criterion_names = assignment.ta_criteria.order(:name).pluck(:name)
+    criterion_names = [{ const: nil, title: I18n.t('not_applicable') }]
+    criterion_names += assignment.ta_criteria.order(:name).pluck(:name).map { |c| { const: c, title: c } }
     { type: :object,
       properties: {
         name: {
@@ -19,9 +20,10 @@ def extra_test_group_schema(assignment)
           title: I18n.t('automated_tests.display_output_title')
         },
         criterion: {
-          type: :string,
-          enum: criterion_names.presence || [''],
-          title: Criterion.model_name.human
+          type: %w[string null],
+          title: Criterion.model_name.human,
+          oneOf: criterion_names,
+          default: nil
         }
       },
       required: %w[display_output] }

app/models/submission.rb

@@ -120,20 +120,18 @@ def set_autotest_marks
     result.create_marks # creates marks for any new criteria that may have just been added
     result.marks.each do |mark|
       test_groups = mark.criterion.test_groups
-      if test_groups.empty? # there's at least one manually-assigned mark
+      test_group_results = test_run.test_group_results.where(test_group_id: test_groups.ids)
+      # don't update mark if there are no results, or if there was an error
+      if test_group_results.empty? || test_group_results.exists?(error_type: TestGroupResult::ERROR_TYPE.values)
         complete_marks = false
         next
       end
-      # don't update mark if there is an error
-      next if test_run.test_group_results
-                      .exists?(error_type: TestGroupResult::ERROR_TYPE.slice(:no_results, :test_error).values)
 
       all_marks_earned = 0.0
       all_marks_total = 0.0
-      test_groups.each do |test_group|
-        res = test_run.test_group_results.find_by(test_group: test_group)
-        all_marks_earned += res&.marks_earned || 0.0
-        all_marks_total += res&.marks_total || 0.0
+      test_group_results.each do |res|
+        all_marks_earned += res.marks_earned
+        all_marks_total += res.marks_total
       end
       if all_marks_earned == 0 || all_marks_total == 0
         final_mark = 0.0

spec/factories/test_group_results.rb

@@ -5,5 +5,6 @@
     marks_earned { 1 }
     marks_total { 1 }
     time { Faker::Number.number(digits: 4) }
+    error_type { nil }
   end
 end

spec/factories/test_groups.rb

@@ -3,6 +3,7 @@
     association :assignment
     sequence(:name) { |n| "Test Group #{n}" }
     sequence(:position) { |n| n }
+    criterion { nil }
 
     after :create do |test_group|
       test_group.update!(autotest_settings: { 'extra_info' => { 'test_group_id' => test_group.id } })

spec/models/submission_spec.rb

@@ -157,4 +157,42 @@
       end
     end
   end
+
+  describe '#set_autotest_marks' do
+    let(:submission) { create :submission }
+    let(:assignment) { submission.assignment }
+    let(:result) { submission.get_latest_result }
+    let!(:criterion) { create :flexible_criterion, assignment: assignment }
+    let!(:mark) { create :mark, criterion: criterion, result: result }
+    let(:test_group) { create :test_group, assignment: assignment, criterion: criterion }
+    let(:test_run) { create :test_run, grouping: submission.grouping, submission: submission }
+
+    context 'when a TestGroupResult succeeded with no error' do
+      let!(:test_group_result) do
+        create :test_group_result, test_group: test_group, test_run: test_run,
+                                   error_type: nil, marks_earned: 1, marks_total: 1
+      end
+
+      it 'updates the result mark' do
+        assignment.ta_criteria.reload
+        submission.set_autotest_marks
+        mark.reload
+        expect(mark.mark).to eq criterion.max_mark
+      end
+    end
+
+    context 'when a TestGroupResult timed out' do
+      let!(:test_group_result) do
+        create :test_group_result, test_group: test_group, test_run: test_run,
+                                   error_type: TestGroupResult::ERROR_TYPE[:timeout]
+      end
+
+      it 'does not update the result mark' do
+        assignment.ta_criteria.reload
+        submission.set_autotest_marks
+        mark.reload
+        expect(mark.mark).to be_nil
+      end
+    end
+  end
 end