Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use default system encoding when reading test result JSON-file (Python3) #104

Merged
merged 2 commits into from
Apr 3, 2020

Conversation

jesseanttila-cai
Copy link
Contributor

The Python 3 test runner uses .tmc_test_results.json to report its results, the contents of which are written in the default system encoding. The corresponding parser on the Java side expects the content to be utf-8 encoded, which causes a parsing error on Windows systems when non-ASCII characters appear in the test results.

This patch solves the issue by reading the test result file using the default system encoding, which should match the one it was written in. The alternative solution of always having the Python 3 test runner write its results using the utf-8 encoding would require all exercises to be updated.

Finally, this solution could be made more robust by using an encoding detection library to find a compatible encoding, although that approach will result in false matches for some bytestrings.

Copy link
Contributor

@Redande Redande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Row 54 has incorrect indentation, leading to checkstyle failing in Travis. Could you indent the row to level 16 from level 12 please?

@Redande
Copy link
Contributor

Redande commented Apr 2, 2020

Maven checkstyle failed due to row 54 in the modified file. Could you indent the row correctly please? You can run mvn checkstyle:check in terminal to see possible checkstyle errors.

Travis also failed due to log length exceeding the limit. There's been a fix to not log as much output, which was merged to master. Could you rebase this branch to include that fix so that Travis passes please?

Otherwise this looks good, and sounds like a good fix to the parsing error. Thank you for offering this fix!

@Redande Redande merged commit aa39ebe into testmycode:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants