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.

Sign up for GitHub

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

Jump to bottom

Use "status" prop and other misc improvements #27

Merged
merged 11 commits into from
Sep 28, 2022
Merged

Use "status" prop and other misc improvements #27

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c825062
Mark failing tests with "status" key
isaacl Sep 18, 2022
16a4ad0
Fix status for partial scores
isaacl Sep 18, 2022
e14dcf7
fix score key error
isaacl Sep 18, 2022
078c116
Fix error concat
isaacl Sep 18, 2022
44f5755
Adjust msg based on testing
isaacl Sep 18, 2022
312c09f
Adjust msg spacing
isaacl Sep 18, 2022
59d9534
better failure display
isaacl Sep 18, 2022
b78a4eb
Update json_test_runner.py
isaacl Sep 20, 2022
aad3b60
Revert test failure output format change
ibrahima Sep 27, 2022
eceabb0
Ensure that output is always a string
ibrahima Sep 27, 2022
964305a
Add back empty output check
isaacl Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions gradescope_utils/autograder_utils/json_test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def getTags(self, test):
return getattr(getattr(test, test._testMethodName), '__tags__', None)

def getWeight(self, test):
return getattr(getattr(test, test._testMethodName), '__weight__', 0.0)
return getattr(getattr(test, test._testMethodName), '__weight__', None)

def getScore(self, test):
return getattr(getattr(test, test._testMethodName), '__score__', None)
Expand Down Expand Up @@ -65,31 +65,43 @@ def getOutput(self):
return out

def buildResult(self, test, err=None):
passed = (err == None)

failed = err is not None
weight = self.getWeight(test)
tags = self.getTags(test)
number = self.getNumber(test)
visibility = self.getVisibility(test)
hide_errors_message = self.getHideErrors(test)
score = self.getScore(test)
if score is None:
score = weight if passed else 0.0

output = self.getOutput()
if err:
if hide_errors_message:
output += hide_errors_message
else:
output += "Test Failed: {0}\n".format(err[1])
if output:
# create a double newline
if output.endswith('\n'):
output += '\n'
else:
output += '\n\n'
output += f'Fail: {err[1] or "<no msg>"}\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split to #29

ibrahima marked this conversation as resolved.
Show resolved Hide resolved
result = {
"name": self.getDescription(test),
"score": score,
"max_score": weight,
}
if score is not None or weight is not None:
if weight is None:
weight = 0.0
if score is None:
score = 0.0 if failed else weight
result["score"] = score
result["max_score"] = weight
# Also mark failure if points are lost
failed |= score < weight

result["status"] = "failed" if failed else "passed"

if tags:
result["tags"] = tags
if output and len(output) > 0:
if output:
result["output"] = output
if visibility:
result["visibility"] = visibility
Expand Down Expand Up @@ -185,7 +197,7 @@ def run(self, test):

total_score = 0
for test in self.json_data["tests"]:
total_score += test["score"]
total_score += test.get("score", 0.0)
self.json_data["score"] = total_score

if self.post_processor is not None:
Expand Down