Merge pull request #760 from openHPI/create_linter_checks

Store Linter Run Assessment

app/controllers/concerns/submission_scoring.rb

@@ -12,14 +12,14 @@ module SubmissionScoring
         output = execute_test_file(file, submission)
         assessment = assessor.assess(output)
         passed = ((assessment[:passed] == assessment[:count]) and (assessment[:score]).positive?)
-        testrun_output = passed ? nil : 'message: ' + output[:message].to_s + "\n stdout: " + output[:stdout].to_s + "\n stderr: " + output[:stderr].to_s
+        testrun_output = passed ? nil : 'status: ' + output[:status].to_s + "\n stdout: " + output[:stdout].to_s + "\n stderr: " + output[:stderr].to_s
         unless testrun_output.blank?
           submission.exercise.execution_environment.error_templates.each do |template|
             pattern = Regexp.new(template.signature).freeze
             StructuredError.create_from_template(template, testrun_output, submission) if pattern.match(testrun_output)
           end
         end
-        Testrun.new(
+        testrun = Testrun.create(
           submission: submission,
           cause: 'assess', # Required to differ run and assess for RfC show
           file: file, # Test file that was executed
@@ -27,7 +27,10 @@ module SubmissionScoring
           output: testrun_output,
           container_execution_time: output[:container_execution_time],
           waiting_for_container_time: output[:waiting_for_container_time]
-        ).save
+        )
+
+        LinterCheckRun.create_from(testrun, assessment) if file.teacher_defined_linter?
+
         output.merge!(assessment)
         output.merge!(filename: file.name_with_extension, message: feedback_message(file, output), weight: file.weight)
         # end

app/models/linter_check.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class LinterCheck < ApplicationRecord
+  has_many :linter_check_runs
+end

app/models/linter_check_run.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class LinterCheckRun < ApplicationRecord
+  belongs_to :linter_check
+  belongs_to :testrun
+  belongs_to :file, class_name: 'CodeOcean::File'
+
+  def self.create_from(testrun, assessment)
+    assessment[:detailed_linter_results].each do |linter_result|
+      check = LinterCheck.find_or_create_by!(code: linter_result[:code]) do |new_check|
+        new_check.name = linter_result[:name]
+        new_check.severity = linter_result[:severity]
+      end
+
+      file = testrun.submission.file_by_name(linter_result[:file_name])
+
+      LinterCheckRun.create!(
+        linter_check: check,
+        result: linter_result[:result],
+        line: linter_result[:line],
+        scope: linter_result[:scope],
+        testrun: testrun,
+        file: file
+      )
+    end
+  rescue ActiveRecord::RecordInvalid
+    # Something bad happened. Probably, the RegEx in lib/py_lint_adapter.rb didn't work.
+    Raven.extra_context(testrun: testrun, linter_result: linter_result)
+  end
+end

app/models/submission.rb

@@ -53,6 +53,11 @@ class Submission < ApplicationRecord
     collect_files.detect(&:main_file?)
   end
 
+  def file_by_name(file_path)
+    # expects the full file path incl. file extension
+    collect_files.detect { |file| file.filepath == file_path }
+  end
+
   def normalized_score
     ::NewRelic::Agent.add_custom_attributes({unnormalized_score: score})
     if !score.nil? && !exercise.maximum_score.nil? && (exercise.maximum_score > 0)

db/migrate/20201026184633_create_linter_checks.rb

@@ -0,0 +1,19 @@
+class CreateLinterChecks < ActiveRecord::Migration[5.2]
+  def change
+    create_table :linter_checks do |t|
+      t.string :name, null: false
+      t.string :code, null: false
+      t.string :severity
+    end
+
+    create_table :linter_check_runs do |t|
+      t.references :linter_check, null: false
+      t.string :scope
+      t.integer :line
+      t.text :result
+      t.references :testrun, null: false
+      t.references :file, null: false
+      t.timestamps
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_10_19_090123) do
+ActiveRecord::Schema.define(version: 2020_10_26_184633) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -265,6 +265,26 @@ ActiveRecord::Schema.define(version: 2020_10_19_090123) do
     t.datetime "updated_at"
   end
 
+  create_table "linter_check_runs", force: :cascade do |t|
+    t.bigint "linter_check_id", null: false
+    t.string "scope"
+    t.integer "line"
+    t.text "result"
+    t.bigint "testrun_id", null: false
+    t.bigint "file_id", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["file_id"], name: "index_linter_check_runs_on_file_id"
+    t.index ["linter_check_id"], name: "index_linter_check_runs_on_linter_check_id"
+    t.index ["testrun_id"], name: "index_linter_check_runs_on_testrun_id"
+  end
+
+  create_table "linter_checks", force: :cascade do |t|
+    t.string "name", null: false
+    t.string "code", null: false
+    t.string "severity"
+  end
+
   create_table "lti_parameters", id: :serial, force: :cascade do |t|
     t.integer "external_users_id"
     t.integer "consumers_id"

lib/py_lint_adapter.rb

@@ -1,6 +1,6 @@
 class PyLintAdapter < TestingFrameworkAdapter
   REGEXP = /Your code has been rated at (-?\d+\.?\d*)\/(\d+\.?\d*)/
-  ASSERTION_ERROR_REGEXP = /^(.*?\.py):(\d+):.*?\([^,]*?,\ ([^,]*?),([^,]*?)\)\ (.*?)$/
+  ASSERTION_ERROR_REGEXP = /^(.*?\.py):(\d+):(.*?)\(([^,]*?), ([^,]*?),([^,]*?)\) (.*?)$/
 
   def self.framework_name
     'PyLint'
@@ -21,19 +21,22 @@ class PyLintAdapter < TestingFrameworkAdapter
     begin
       assertion_error_matches = Timeout.timeout(2.seconds) do
         output[:stdout].scan(ASSERTION_ERROR_REGEXP).map do |match|
-          file_name = match.first.strip
+          {
-          line_no = match.second.strip
+            file_name: match[0].strip,
-          test = match.third.strip
+            line: match[1].to_i,
-          # e.g. function name, nil if outside of a function. Not always available
+            severity: match[2].strip,
-          context = match.fourth.strip.presence
+            code: match[3].strip,
-          description = match.fifth.strip
+            name: match[4].strip,
-          {test: test, description: description, context: context, line: line_no, file: file_name}
+            # e.g. function name, nil if outside of a function. Not always available
+            scope: match[5].strip.presence,
+            result: match[6].strip
+          }
         end || []
       end
     rescue Timeout::Error
       assertion_error_matches = []
     end
-    concatenated_errors = assertion_error_matches.map { |result| "#{result[:test]}: #{result[:description]}" }.flatten
+    concatenated_errors = assertion_error_matches.map { |result| "#{result[:name]}: #{result[:result]}" }.flatten
     {count: count, failed: failed, error_messages: concatenated_errors, detailed_linter_results: assertion_error_matches}
   end
 end