search

gradescope / gradescope-utils

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

Fix `buffer` argument not being respected #14

Closed mnoukhov closed 4 years ago

mnoukhov commented 5 years ago

Address #13

I apologize this fix is untested and very rushed but I hope it's right

ibrahima commented 5 years ago

Thanks for the PR! Unfortunately I don't have time to test it right now and can't merge it until it is tested, so if it is important to you I'd appreciate it if you could test it out when you get a chance. I do think that it worked initially but maybe I wasn't testing the right thing.

The code does make sense though.

mnoukhov commented 5 years ago

I've had some more time to look into this and I think I've got a handle on what the buffer argument means for the JSONTestRunner, I've described it below and correct me if I'm wrong.

For JSONTestRunner, I think the idea is that if there is no buffer and there is no error, to still capture output the stdout. If there is no buffer and there is an error, then to output both stdout and stderr. Now if there is a buffer, the only difference is you don't output stdout when there is no error.

To do this with setup that outputs a json, I think it's necessary to always capture the stdout and stderr (JSONTestResult.buffer = True) and then to selectively include them in the json or not. Outputting a json, JSONTestResult.buffer = False would mean no output in any case since you're not capturing it and adding it to the json.

mnoukhov commented 5 years ago

I've updated the behaviour of buffer to reflect my ideas above by adding an argument buffer_output that represents the actual buffering we need for json. Tested it and it looks like it's working!

ibrahima commented 4 years ago

Ah so the issue was not that buffer isn't being respected, it's that buffer=False didn't do what you expected. I guess in general we probably want to always buffer standard output and decide whether or not to include it in the test output based on a different argument, which is what you ended up doing? The thing is that the behavior of the buffer argument is just standard unittest behavior so it's not really anything this library is doing right or wrong per se.

mnoukhov commented 4 years ago

That's about right, yeah. The thing is self.buffer needs to be true if you're capturing the output as you're doing it now, so it's not really a useful variable. If you'd like, I can change the code to remove if self.buffer and the buffer variable in general and rename buffer_output to buffer.

ibrahima commented 4 years ago

Ah that's fine, it's just carried over from the parent class anyway. I think there might be a better name for it than buffer_output though, I'll have to think about it a bit. Thanks!