gradescope / gradescope-utils

Python package for building Gradescope autograders
https://gradescope-utils.readthedocs.io/en/latest/
MIT License
34 stars 25 forks source link

buffer argument not respected in JSONTestRunner #13

Closed mnoukhov closed 4 years ago

mnoukhov commented 4 years ago

In json_test_runner.py, the buffer argument is not being passed to the TestResult class because it is not an argument in JSONTestResult and therefore not passed to the super().__init__. This means that the buffer value for JSONTestRunner is not respected.

The correct change is to add buffer to the arguments of JSONTestResult.__init__ and pass it to TestResult in the super().__init__ function

ibrahima commented 4 years ago

Hmm, so I guess this was not actually completely true, because we are setting result.buffer over here: https://github.com/gradescope/gradescope-utils/blob/master/gradescope_utils/autograder_utils/json_test_runner.py#L156 (this is the same thing the built in TextTestRunner does: https://github.com/python/cpython/blob/master/Lib/unittest/runner.py#L156)

I think if it seemed like buffer was not being respected, perhaps it was because you were not writing the test results directly to a file because the example was not doing that? I think if you incorporate this change it should actually work as expected: https://github.com/gradescope/autograder_samples/pull/25

Given the above, I am not sure if #14 is still necessary. I kind of see the need, but I think it makes more sense to add a new option with a different name, but it also seems like you might be able to achieve what you want via the above changes.

mnoukhov commented 4 years ago

Oh sorry, I realize that my updated comments were only in the PR not here.

Yes, buffer is indeed respected but just wasn't feasible with the stdout to json that was being done in run_autograder.

The change you've made by writing the stream to a file is actually preferable, though. I just assumed (incorrectly) that you needed to use stdout for some reason. I've updated my own template code (https://github.com/mnoukhov/gradescope-autograder-template) to write to file and this simplifies things! Thanks.

ibrahima commented 4 years ago

Ah yeah, the only reason it was writign to stdout is because it was easier to run locally that way. But otherwise writing to a file directly is just a safer way to do it.