firstdraft / appdev_template

A Rails template for generating homework projects
0 stars 1 forks source link

Use Oj to parse JSON in JsonOutputFormatter #120

Closed jelaniwoods closed 4 years ago

jelaniwoods commented 4 years ago

This is Part 1 of the changes that will fix the intended error. The grade_runner is responsible for the other part.

There have been a few strange issues where a utf-8 character, like an emoji, will break the JSON formatting of the rspec output. It's inconsistent, which is bad, but seems to consistently fail on Gitpod, which is all that matters. Create a workspace from this repo and run rspec --format j will fail. Since the Ruby JSON conversion fails with

`encode': "\xF0" from ASCII-8BIT to UTF-8 (Encoding::UndefinedConversionError)

You can reproduce this error by creating a Gitpod workspace from this repo and running rails grade or rspec --format j or rspec --format JsonOutputFormatter.

This appears to be an issue in the JSON parser of Ruby itself, which is why our custom JSON formatter also fails. I don't have time to look into it more deeply, but am interested in learning of the true cause.

In particular, the default rspec JSON formatter has a close method that does

def close(_notification)
  output.write(@output_hash.to_json)
end

and that to_json will cause the encoding error.

Many attempts to change or force encoding did not seem to work, but simply using oj 🍊🧃did. By substituting to_json with Oj.dump, UTF characters are parsed without issue. This seems okay to do, since grades already uses oj for stuff.

The proposed change

def close(_notification)
  output.write  Oj.dump @output_hash
end

should prevent any UTF-8 conversion errors triggered from running rspec --format JsonOutputFormatter.

Any conversion errors are triggered from grade_runner and are resolved here https://github.com/firstdraft/grade_runner/pull/35.

jelaniwoods commented 4 years ago

Maybe this is worth looking into: https://stackoverflow.com/questions/9342222/rspec-have-link-with-accent-words#9342357

jelaniwoods commented 4 years ago

I intend to merge this today if there is no additional feedback.