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

Fix how binary files are handled during external grading #1489

Merged
merged 1 commit into from Apr 21, 2019

Conversation

nwalters512
Copy link
Contributor

For text files, eagerly decoding the base64 contents to a string was fine. For binary files (tar archives, specifically) that breaks things. We now decode the files into a Buffer and write that directly to disk.

@trombonekenny
Copy link
Contributor

Is this a good place to hook in a test case for external grading that the supplied file actually matches what's seen on the grader?

@nwalters512
Copy link
Contributor Author

We should definitely have a test case for that, though I'm still not sure what the best way to do that kind of integration test for external grading would be. Testing async things is tricky.

@nwalters512
Copy link
Contributor Author

To be fair, we could probably just test buildDirectory directly if we can get the right data in place beforehand.

@nwalters512
Copy link
Contributor Author

Scratch that, the function doesn't even read from the database! I'll pretend like I thought ahead to make it easily unit-testable. Let me see if I can write some tests for this.

Copy link
Contributor

@trombonekenny trombonekenny left a comment

Choose a reason for hiding this comment

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

lgtm

@nwalters512
Copy link
Contributor Author

I'll punt the tests to another branch, don't know how long those will take me and I want to ship this soon for STAT 430.

@nwalters512 nwalters512 merged commit 8352c3f into master Apr 21, 2019
@nwalters512 nwalters512 deleted the fix-binary-files-external-grading branch April 21, 2019 19:30
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