From 655402af52708cd8dfb999bbfdecc3826c742a00 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Fri, 18 Oct 2024 14:28:44 +0200 Subject: [PATCH 1/4] Fallback to textdiffer when csv parsing fails --- .../renderers/feedback_table_renderer.rb | 26 ++++++++++++++----- .../submissions_controller_test.rb | 9 +++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/app/helpers/renderers/feedback_table_renderer.rb b/app/helpers/renderers/feedback_table_renderer.rb index fd3b877468..a017d850a9 100644 --- a/app/helpers/renderers/feedback_table_renderer.rb +++ b/app/helpers/renderers/feedback_table_renderer.rb @@ -319,17 +319,31 @@ def messages(msgs) end end + def render_accepted(builder, t) + if t[:format] == 'csv' + begin + return CsvDiffer.render_accepted(builder, t[:expected]) if CsvDiffer.limited_columns?(t[:expected]) + rescue CSV::MalformedCSVError + # default to text if parsing failed + end + end + TextDiffer.render_accepted(builder, t[:expected]) + end + def differ(t) - if t[:format] == 'csv' && CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) - CsvDiffer - else - TextDiffer + if t[:format] == 'csv' + begin + return CsvDiffer.new(t[:generated], t[:expected]) if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) + rescue CSV::MalformedCSVError + # default to text if parsing failed + end end + TextDiffer.new(t[:generated], t[:expected]) end def test_accepted(t) @builder.div(class: 'test-accepted') do - differ(t).render_accepted(@builder, t[:generated]) + render_accepted(@builder, t) end end @@ -340,7 +354,7 @@ def test_failed(t) end def diff(t) - differ = differ(t).new(t[:generated], t[:expected]) + differ = differ(t) @builder.div(class: "diffs show-#{@diff_type}") do @builder << differ.split @builder << differ.unified diff --git a/test/controllers/submissions_controller_test.rb b/test/controllers/submissions_controller_test.rb index de18639095..0fc3209508 100644 --- a/test/controllers/submissions_controller_test.rb +++ b/test/controllers/submissions_controller_test.rb @@ -477,4 +477,13 @@ def expected_score_string(*args) assert_response :ok end + + test 'Should be able to render ' do + submission = create :wrong_submission, code: 'select * from track where trackid = 1610', + result: '{"accepted":false,"status":"wrong","description":"Test failed","annotations":[{"text":"An SQL query should end with a semicolon","type":"info","row":1,"rows":1,"column":null,"columns":null,"externalUrl":null}],"groups":[{"description":"Result Validation","badgeCount":2,"groups":[{"accepted":false,"groups":[{"description":"Compare results","accepted":false,"tests":[{"expected":"\"title\"\\n\"Grace\"\\n\"Monster\"\\n\"The Greatest Hits\"\\n\"Throwing Copper\"\\n\"Toward the Within\"\\n\"Troublegum\"\\n\"Worst Case Scenario\"","format":"csv","messages":[{"format":"callout-danger","description":"Incorrect row count"}],"generated":"\"title\",\"trackid\",\"genre\",\"albumid\",\"artistid\",\"tracknumber\"\\n\"\"So Fast, So Numb\"\",\"1610\",\"Rock\",\"523\",\"229\",\"12\"","accepted":false}]}]},{"accepted":true,"groups":[{"description":"Column Types","accepted":true,"tests":[{"expected":"\"column\",\"type\"\\n\"title\",\"STRING\"","format":"csv","generated":"\"column\",\"type\"\\n\"title\",\"STRING\"\\n\"trackid\",\"INTEGER\"\\n\"genre\",\"STRING\"\\n\"albumid\",\"INTEGER\"\\n\"artistid\",\"INTEGER\"\\n\"tracknumber\",\"INTEGER\"","accepted":true}]}]},{"accepted":false,"groups":[{"description":"Order By","accepted":false,"tests":[{"expected":"correct order","generated":"incorrect order","accepted":false}]}]}]},{"description":"Query Result","badgeCount":0,"groups":[{"accepted":true,"groups":[{"description":"Full result","accepted":true,"tests":[{"expected":"","format":"csv","generated":"\"title\",\"trackid\",\"genre\",\"albumid\",\"artistid\",\"tracknumber\"\\n\"\"So Fast, So Numb\"\",\"1610\",\"Rock\",\"523\",\"229\",\"12\"","accepted":true}]}]}]}],"messages":[{"format":"html","description":"\\u003cstrong\\u003eWorker:\\u003c/strong\\u003e ixion","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eMemory usage:\\u003c/strong\\u003e 40.61 MiB","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003ePrepare:\\u003c/strong\\u003e 0.13 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eRuntime:\\u003c/strong\\u003e 0.92 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eResult construction:\\u003c/strong\\u003e 0.02 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eFinalize:\\u003c/strong\\u003e 0.01 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eTotal time:\\u003c/strong\\u003e 1.14 seconds","permission":"zeus"}]}' + sign_in submission.user + get submission_path(submission) + + assert_response :ok + end end From b04b434caece9644166e14b29e2235e7c16ba5da Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Fri, 18 Oct 2024 14:47:21 +0200 Subject: [PATCH 2/4] Make test more descriptive --- test/controllers/submissions_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/submissions_controller_test.rb b/test/controllers/submissions_controller_test.rb index 0fc3209508..a362ac2e5b 100644 --- a/test/controllers/submissions_controller_test.rb +++ b/test/controllers/submissions_controller_test.rb @@ -478,7 +478,7 @@ def expected_score_string(*args) assert_response :ok end - test 'Should be able to render ' do + test 'Should be able to render submission with malformed csv' do submission = create :wrong_submission, code: 'select * from track where trackid = 1610', result: '{"accepted":false,"status":"wrong","description":"Test failed","annotations":[{"text":"An SQL query should end with a semicolon","type":"info","row":1,"rows":1,"column":null,"columns":null,"externalUrl":null}],"groups":[{"description":"Result Validation","badgeCount":2,"groups":[{"accepted":false,"groups":[{"description":"Compare results","accepted":false,"tests":[{"expected":"\"title\"\\n\"Grace\"\\n\"Monster\"\\n\"The Greatest Hits\"\\n\"Throwing Copper\"\\n\"Toward the Within\"\\n\"Troublegum\"\\n\"Worst Case Scenario\"","format":"csv","messages":[{"format":"callout-danger","description":"Incorrect row count"}],"generated":"\"title\",\"trackid\",\"genre\",\"albumid\",\"artistid\",\"tracknumber\"\\n\"\"So Fast, So Numb\"\",\"1610\",\"Rock\",\"523\",\"229\",\"12\"","accepted":false}]}]},{"accepted":true,"groups":[{"description":"Column Types","accepted":true,"tests":[{"expected":"\"column\",\"type\"\\n\"title\",\"STRING\"","format":"csv","generated":"\"column\",\"type\"\\n\"title\",\"STRING\"\\n\"trackid\",\"INTEGER\"\\n\"genre\",\"STRING\"\\n\"albumid\",\"INTEGER\"\\n\"artistid\",\"INTEGER\"\\n\"tracknumber\",\"INTEGER\"","accepted":true}]}]},{"accepted":false,"groups":[{"description":"Order By","accepted":false,"tests":[{"expected":"correct order","generated":"incorrect order","accepted":false}]}]}]},{"description":"Query Result","badgeCount":0,"groups":[{"accepted":true,"groups":[{"description":"Full result","accepted":true,"tests":[{"expected":"","format":"csv","generated":"\"title\",\"trackid\",\"genre\",\"albumid\",\"artistid\",\"tracknumber\"\\n\"\"So Fast, So Numb\"\",\"1610\",\"Rock\",\"523\",\"229\",\"12\"","accepted":true}]}]}]}],"messages":[{"format":"html","description":"\\u003cstrong\\u003eWorker:\\u003c/strong\\u003e ixion","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eMemory usage:\\u003c/strong\\u003e 40.61 MiB","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003ePrepare:\\u003c/strong\\u003e 0.13 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eRuntime:\\u003c/strong\\u003e 0.92 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eResult construction:\\u003c/strong\\u003e 0.02 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eFinalize:\\u003c/strong\\u003e 0.01 seconds","permission":"zeus"},{"format":"html","description":"\\u003cstrong\\u003eTotal time:\\u003c/strong\\u003e 1.14 seconds","permission":"zeus"}]}' sign_in submission.user From ca9b09e019da3d1c1d3ec64e109e9c3174e0188f Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Mon, 21 Oct 2024 10:54:06 +0200 Subject: [PATCH 3/4] Use an abstraction and make falthrough clearer --- .../renderers/feedback_table_renderer.rb | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/app/helpers/renderers/feedback_table_renderer.rb b/app/helpers/renderers/feedback_table_renderer.rb index a017d850a9..567a03b48b 100644 --- a/app/helpers/renderers/feedback_table_renderer.rb +++ b/app/helpers/renderers/feedback_table_renderer.rb @@ -319,31 +319,27 @@ def messages(msgs) end end - def render_accepted(builder, t) + def with_differ_class(t) if t[:format] == 'csv' begin - return CsvDiffer.render_accepted(builder, t[:expected]) if CsvDiffer.limited_columns?(t[:expected]) - rescue CSV::MalformedCSVError - # default to text if parsing failed - end - end - TextDiffer.render_accepted(builder, t[:expected]) - end - - def differ(t) - if t[:format] == 'csv' - begin - return CsvDiffer.new(t[:generated], t[:expected]) if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) + if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) + yield(CsvDiffer) + else + yield(TextDiffer) + end rescue CSV::MalformedCSVError - # default to text if parsing failed + yield(TextDiffer) end + else + yield(TextDiffer) end - TextDiffer.new(t[:generated], t[:expected]) end def test_accepted(t) @builder.div(class: 'test-accepted') do - render_accepted(@builder, t) + with_differ_class(t) do |differ_class| + differ_class.render_accepted(@builder, t[:expected]) + end end end @@ -354,10 +350,12 @@ def test_failed(t) end def diff(t) - differ = differ(t) - @builder.div(class: "diffs show-#{@diff_type}") do - @builder << differ.split - @builder << differ.unified + with_differ_class(t) do |differ_class| + differ = differ_class.new(t[:generated], t[:expected]) + @builder.div(class: "diffs show-#{@diff_type}") do + @builder << differ.split + @builder << differ.unified + end end end From 2d8742541d3c8dca5e582f38319d4a97c132f8c7 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Mon, 21 Oct 2024 16:47:29 +0200 Subject: [PATCH 4/4] Reduce the nummer of else clauses --- app/helpers/renderers/feedback_table_renderer.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/helpers/renderers/feedback_table_renderer.rb b/app/helpers/renderers/feedback_table_renderer.rb index 567a03b48b..30e32bd4b3 100644 --- a/app/helpers/renderers/feedback_table_renderer.rb +++ b/app/helpers/renderers/feedback_table_renderer.rb @@ -322,17 +322,13 @@ def messages(msgs) def with_differ_class(t) if t[:format] == 'csv' begin - if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) - yield(CsvDiffer) - else - yield(TextDiffer) - end + return yield(CsvDiffer) if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) rescue CSV::MalformedCSVError - yield(TextDiffer) + # use the default differ end - else - yield(TextDiffer) end + + yield(TextDiffer) end def test_accepted(t)