From cf58be97ee72abd4d2d85d90d856b0dfdced7f0f Mon Sep 17 00:00:00 2001 From: Felix Auringer <48409110+felixauringer@users.noreply.github.com> Date: Tue, 8 Jun 2021 12:36:49 +0200 Subject: [PATCH] Fix 17 previously failing specs --- .../concerns/submission_scoring.rb | 25 ++++--------- app/controllers/exercises_controller.rb | 3 +- .../remote_evaluation_controller.rb | 3 +- .../request_for_comments_controller.rb | 4 +-- app/controllers/submissions_controller.rb | 31 +++++++--------- spec/concerns/submission_scoring_spec.rb | 36 +++++++++---------- spec/controllers/exercises_controller_spec.rb | 28 ++++++++++++--- spec/features/editor_spec.rb | 7 ++-- 8 files changed, 66 insertions(+), 71 deletions(-) diff --git a/app/controllers/concerns/submission_scoring.rb b/app/controllers/concerns/submission_scoring.rb index 85da5106..ebcfb3d2 100644 --- a/app/controllers/concerns/submission_scoring.rb +++ b/app/controllers/concerns/submission_scoring.rb @@ -31,38 +31,27 @@ module SubmissionScoring LinterCheckRun.create_from(testrun, assessment) assessment = assessor.translate_linter(assessment, I18n.locale) - # replace file name with hint if linter is not used for grading. Refactor! - filename = t('exercises.implement.not_graded') if file.weight.zero? - end + # replace file name with hint if linter is not used for grading. Refactor! + filename = t('exercises.implement.not_graded', locale: :de) if file.weight.zero? + end output.merge!(assessment) output.merge!(filename: filename, message: feedback_message(file, output), weight: file.weight) end - private :collect_test_results - - def execute_test_file(file, submission) - # TODO: replace DockerClient here - DockerClient.new(execution_environment: file.context.execution_environment).execute_test_command(submission, - file.name_with_extension) - end - - private :execute_test_file - - def feedback_message(_file, output) - # TODO: why did we comment out set_locale and render_markdown? - set_locale + # TODO: make this a controller concern again to bring the locales nearer to the view + def feedback_message(file, output) if output[:score] == Assessor::MAXIMUM_SCORE && output[:file_role] == 'teacher_defined_test' I18n.t('exercises.implement.default_test_feedback') elsif output[:score] == Assessor::MAXIMUM_SCORE && output[:file_role] == 'teacher_defined_linter' I18n.t('exercises.implement.default_linter_feedback') else - render_markdown(file.feedback_message) + # render_markdown(file.feedback_message) + file.feedback_message end end def score_submission(outputs) - # outputs = collect_test_results(submission) submission = self score = 0.0 if outputs.present? diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index f3507103..af9b0086 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -4,7 +4,6 @@ class ExercisesController < ApplicationController include CommonBehavior include Lti include SubmissionParameters - include SubmissionScoring include TimeHelper before_action :handle_file_uploads, only: %i[create update] @@ -533,7 +532,7 @@ working_time_accumulated: working_time_accumulated}) def submit @submission = Submission.create(submission_params) - score_submission(@submission) + @submission.calculate_score if @submission.user.external_user? && lti_outcome_service?(@submission.exercise_id, @submission.user.id) transmit_lti_score else diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb index 2dd3bee7..96db6c8b 100644 --- a/app/controllers/remote_evaluation_controller.rb +++ b/app/controllers/remote_evaluation_controller.rb @@ -2,7 +2,6 @@ class RemoteEvaluationController < ApplicationController include RemoteEvaluationParameters - include SubmissionScoring include Lti skip_after_action :verify_authorized @@ -63,7 +62,7 @@ status: 202} validation_token = remote_evaluation_params[:validation_token] if (remote_evaluation_mapping = RemoteEvaluationMapping.find_by(validation_token: validation_token)) @submission = Submission.create(build_submission_params(cause, remote_evaluation_mapping)) - score_submission(@submission) + @submission.calculate_score else # TODO: better output # TODO: check token expired? diff --git a/app/controllers/request_for_comments_controller.rb b/app/controllers/request_for_comments_controller.rb index f3959af4..2fd4046e 100644 --- a/app/controllers/request_for_comments_controller.rb +++ b/app/controllers/request_for_comments_controller.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class RequestForCommentsController < ApplicationController - include SubmissionScoring - before_action :require_user! before_action :set_request_for_comment, only: %i[show mark_as_solved set_thank_you_note] before_action :set_study_group_grouping, @@ -121,7 +119,7 @@ class RequestForCommentsController < ApplicationController if @request_for_comment.save # create thread here and execute tests. A run is triggered from the frontend and does not need to be handled here. Thread.new do - score_submission(@request_for_comment.submission) + @request_for_comment.submission.calculate_score ensure ActiveRecord::Base.connection_pool.release_connection end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 93bdab1d..55972d54 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -5,7 +5,6 @@ class SubmissionsController < ApplicationController include CommonBehavior include Lti include SubmissionParameters - include SubmissionScoring include Tubesock::Hijack before_action :set_submission, @@ -237,25 +236,21 @@ class SubmissionsController < ApplicationController end def score - Thread.new do - hijack do |tubesock| - return kill_socket(tubesock) if @embed_options[:disable_run] + hijack do |tubesock| + return kill_socket(tubesock) if @embed_options[:disable_run] - tubesock.send_data(@submission.calculate_score) - # To enable hints when scoring a submission, uncomment the next line: - # send_hints(tubesock, StructuredError.where(submission: @submission)) - rescue Runner::Error::ExecutionTimeout => e - tubesock.send_data JSON.dump({cmd: :status, status: :timeout}) - Rails.logger.debug { "Running a submission failed: #{e.message}" } - rescue Runner::Error => e - tubesock.send_data JSON.dump({cmd: :status, status: :container_depleted}) - Rails.logger.debug { "Runner error while scoring a submission: #{e.message}" } - ensure - tubesock.send_data JSON.dump({cmd: :exit}) - tubesock.close - end + tubesock.send_data(@submission.calculate_score) + # To enable hints when scoring a submission, uncomment the next line: + # send_hints(tubesock, StructuredError.where(submission: @submission)) + rescue Runner::Error::ExecutionTimeout => e + tubesock.send_data JSON.dump({cmd: :status, status: :timeout}) + Rails.logger.debug { "Running a submission failed: #{e.message}" } + rescue Runner::Error => e + tubesock.send_data JSON.dump({cmd: :status, status: :container_depleted}) + Rails.logger.debug { "Runner error while scoring a submission: #{e.message}" } ensure - ActiveRecord::Base.connection_pool.release_connection + tubesock.send_data JSON.dump({cmd: :exit}) + tubesock.close end end diff --git a/spec/concerns/submission_scoring_spec.rb b/spec/concerns/submission_scoring_spec.rb index 086b2ae7..74d97ab8 100644 --- a/spec/concerns/submission_scoring_spec.rb +++ b/spec/concerns/submission_scoring_spec.rb @@ -2,36 +2,34 @@ require 'rails_helper' -class Controller < AnonymousController - include SubmissionScoring -end - -# This is broken since the Runner was added. -describe SubmissionScoring, skip: true do - let(:controller) { Controller.new } +describe SubmissionScoring do let(:submission) { FactoryBot.create(:submission, cause: 'submit') } - before do - controller.instance_variable_set(:@current_user, FactoryBot.create(:external_user)) - controller.instance_variable_set(:@_params, {}) - end - describe '#collect_test_results' do - after { controller.send(:collect_test_results, submission) } + let(:runner) { FactoryBot.create :runner } + + before do + allow(Runner).to receive(:for).and_return(runner) + allow(runner).to receive(:copy_files) + allow(runner).to receive(:execute_interactively).and_return(1.0) + end + + after { submission.calculate_score } it 'executes every teacher-defined test file' do + allow(submission).to receive(:score_submission) submission.collect_files.select(&:teacher_defined_assessment?).each do |file| - allow(controller).to receive(:execute_test_file).with(file, submission).and_return({}) + allow(submission).to receive(:test_result).with(any_args, file).and_return({}) end end + + it 'scores the submission' do + allow(submission).to receive(:score_submission).and_return([]) + end end describe '#score_submission', cleaning_strategy: :truncation do - after { controller.score_submission(submission) } - - it 'collects the test results' do - allow(controller).to receive(:collect_test_results).and_return([]) - end + after { submission.score_submission([]) } it 'assigns a score to the submissions' do expect(submission).to receive(:update).with(score: anything) diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index 6d75ef64..fa0c7628 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -236,17 +236,35 @@ describe ExercisesController do expect_template(:statistics) end - # This is broken since the Runner was added. - describe 'POST #submit', skip: true do + describe 'POST #submit' do let(:output) { {} } let(:perform_request) { post :submit, format: :json, params: {id: exercise.id, submission: {cause: 'submit', exercise_id: exercise.id}} } let(:user) { FactoryBot.create(:external_user) } + let(:scoring_response) do + [{ + status: 'ok', + stdout: '', + stderr: '', + waiting_for_container_time: 0, + container_execution_time: 0, + file_role: 'teacher_defined_test', + count: 1, + failed: 0, + error_messages: [], + passed: 1, + score: 1.0, + filename: 'index.html_spec.rb', + message: 'Well done.', + weight: 2.0, + }] + end before do FactoryBot.create(:lti_parameter, external_user: user, exercise: exercise) - allow_any_instance_of(Submission).to receive(:normalized_score).and_return(1) - allow(controller).to receive(:collect_test_results).and_return([{score: 1, weight: 1}]) - allow(controller).to receive(:score_submission).and_call_original + submission = FactoryBot.build(:submission, exercise: exercise, user: user) + allow(submission).to receive(:normalized_score).and_return(1) + allow(submission).to receive(:calculate_score).and_return(JSON.dump(scoring_response)) + allow(Submission).to receive(:create).and_return(submission) end context 'when LTI outcomes are supported' do diff --git a/spec/features/editor_spec.rb b/spec/features/editor_spec.rb index 1a8152ef..2f2b1126 100644 --- a/spec/features/editor_spec.rb +++ b/spec/features/editor_spec.rb @@ -94,10 +94,9 @@ describe 'Editor', js: true do end it 'contains a button for submitting the exercise' do - # This is broken since the Runner was added. - skip - - allow_any_instance_of(SubmissionsController).to receive(:score_submission).and_return(scoring_response) + submission = FactoryBot.build(:submission, user: user, exercise: exercise) + allow(submission).to receive(:calculate_score).and_return(JSON.dump(scoring_response)) + allow(Submission).to receive(:find).and_return(submission) click_button(I18n.t('exercises.editor.score')) expect(page).not_to have_css('#submit_outdated') expect(page).to have_css('#submit')