Use "status" prop and other misc improvements #27
Conversation
Use new "status" key to mark failing tests red, even if they are 0 weight.
isaacl
force-pushed
the
addStatus
branch
4 times, most recently
from
c9aee18
to
a58cafe
Compare
isaacl
changed the title
|
Hi @isaacl, thanks for your contribution! We'll have to take a look at it in the near future. As a broad note, since this is the "default" public library for Gradescope autograders, we do need to make sure that any changes make sense for everybody, so we will be thinking about these changes from that perspective. |
| output += '\n' | ||
| else: | ||
| output += '\n\n' | ||
| output += f'Fail: {err[1] or "<no msg>"}\n' |
There was a problem hiding this comment.
Could you revert this output change for now? Thanks! I am not sure if it makes sense generally and it would be good to isolate this change from the test status change in any case. I think adding the newlines after the test output is helpful, but I would prefer it if you could change the format of the error message back to "Test Failed: {0}\n".format(err[1]) for now.
Also, the f'string' formatting is a Python 3 feature, and the library should currently work on Python 2 otherwise so it'd be nice to maintain that. I know Python 2 is EOL and people should upgrade, but sometimes it feels like there's not a lot of time for that in education, and I wouldn't want to disrupt a Python-2-using instructor if we don't have to.
| output += f'Fail: {err[1] or "<no msg>"}\n' | |
| output += "Test Failed: {0}\n".format(err[1]) |
Also, I was under the impression that the unittest assertions generally always have some error message, does the <no msg> thing occur in practice? In any case I would expect that the autograder author should ideally be providing more helpful error messages anyway. If anything, I would prefer it to just say "Test Failed." if there is no message in err[1] rather than "Test Failed: <no msg>", it feels cleaner that way.
Incidentally, with the unittest library you can set the longMessage attribute on a test case to override the default error message formatting, so you could use that to tweak the output as well. You'd still have the "Test Failed" part, but then you could remove the default "assertion failed" message and add in extra whitespace, etc. But that wouldn't help with adding newlines between the other output and the test failure message.
There was a problem hiding this comment.
Happy to split this into separate PR.
I think "Fail: " or "Failed: " is easier to parse than "Test Failed: ".
Students need to differentiate the prefix from the actual failure message and I think the space in "Test Failed" makes such differentiation harder.
There was a problem hiding this comment.
Also, I was under the impression that the unittest assertions generally always have some error message, does the thing occur in practice?
I've never seen it, just was coding defensively since err itself can be None and wasn't sure whether err[1] could similarly be None
|
Also I think we still want "if output" even if output is a str It avoids a double newline if output string is empty. |
Ah good point, I didn't realize an empty string was Falsey in Python. I thought it was just None, 0, and False. |
Use new "status" key to mark failing tests red, even if they are 0 weight.