-
Notifications
You must be signed in to change notification settings - Fork 1
Assertion string tweaks #148
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
base: main
Are you sure you want to change the base?
Changes from all commits
7b12d5f
ee01689
2b67ae5
7c10277
ab3fbce
c72ef47
ea2036d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import difflib | ||
| import math | ||
| import re | ||
| import unittest | ||
|
|
@@ -11,6 +12,13 @@ class Assertions(unittest.TestCase): | |
| The primary differentiation factor of this is that it formats the outputs in a nicer way for both gradescope and the | ||
| local autograder | ||
| """ | ||
| RED_BG: str = u"\u001b[41m" | ||
| RED_COLOR: str = u"\u001b[31m" | ||
| GREEN_BG: str = u"\u001b[42m" | ||
| RESET_COLOR: str = u"\u001b[0m" | ||
|
|
||
| DIFF_MAX_CHARACTERS = 200 | ||
|
|
||
| def __init__(self, testResults): | ||
| super().__init__(testResults) | ||
| self.addTypeEqualityFunc(str, self.assertMultiLineEqual) | ||
|
|
@@ -51,14 +59,65 @@ def _convertStringToList(outputLine: str) -> list[str]: | |
|
|
||
| @staticmethod | ||
| def _raiseFailure(shortDescription: str, expectedObject: object, actualObject: object, msg: Optional[str]): | ||
| errorMsg = f"Incorrect {shortDescription}.\n" + \ | ||
| f"Expected {shortDescription}: {expectedObject}\n" + \ | ||
| f"Your {shortDescription} : {actualObject}" | ||
| errorMsg = f"Incorrect {shortDescription}.\n" | ||
| if expectedObject is not None and actualObject is not None and isinstance(expectedObject, str) and isinstance(actualObject, str): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are currently no tests covering this if statement. You need to verify that we dont attempt to use the diff stuff when we arent comparing string to string. |
||
| diffLog = Assertions._highlightStringDifferences(expectedObject, actualObject) | ||
| errorMsg += f"Expected {shortDescription}: {expectedObject}\n" | ||
| errorMsg += f"Your {shortDescription} : {actualObject}\n" | ||
| errorMsg += f"Diff Log {shortDescription}: {diffLog}{Assertions.RED_COLOR}" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not all terminal environments (read: gradescope) support this type of output. There is a way to add it to gradescope, but that will require a change in the HybridJSONTestRunner lib. |
||
| else: | ||
| errorMsg += f"Expected {shortDescription}: {expectedObject}\n" + \ | ||
| f"Your {shortDescription} : {actualObject}" | ||
| if msg: | ||
| errorMsg += "\n\n" + str(msg) | ||
|
Comment on lines
71
to
72
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this interact with the message changes you made later? |
||
|
|
||
| raise AssertionError(errorMsg) | ||
|
|
||
| @staticmethod | ||
| def _highlightStringDifferences(expected: str, actual: str) -> str: | ||
| """Return diff log strings with differences highlighted.""" | ||
| RED_BG = Assertions.RED_BG | ||
| GREEN_BG = Assertions.GREEN_BG | ||
| RESET_COLOR = Assertions.RESET_COLOR | ||
|
|
||
| matcher = difflib.SequenceMatcher(None, expected, actual) | ||
|
|
||
| diffLog = [] | ||
| # maxChars = min(Assertions.DIFF_MAX_CHARACTERS, len(actual), len(expected)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove comment. |
||
| maxChars = Assertions.DIFF_MAX_CHARACTERS | ||
| visibleCount = 0 | ||
|
|
||
| for tag, i1, i2, j1, j2 in matcher.get_opcodes(): | ||
| if visibleCount >= maxChars: | ||
| break | ||
|
|
||
| if tag == 'equal': | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think i might prefer pattern matching here - also - are there constants defined for these tags inside the diff tools lib? |
||
| for ch in expected[i1:i2]: | ||
| if visibleCount >= maxChars: | ||
| break | ||
| diffLog.append(f"{RESET_COLOR}{GREEN_BG}{ch}{RESET_COLOR}") | ||
| visibleCount += 1 | ||
|
Comment on lines
+96
to
+99
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are repeating this block a lot, break it out into a helper function |
||
| elif tag == 'replace': | ||
| for ch in actual[j1:j2]: | ||
| if visibleCount >= maxChars: | ||
| break | ||
| diffLog.append(f"{RESET_COLOR}{RED_BG}{ch}{RESET_COLOR}") | ||
| visibleCount += 1 | ||
| elif tag == 'delete': | ||
| for ch in expected[i1:i2]: | ||
| if visibleCount >= maxChars: | ||
| break | ||
| diffLog.append(f"{RESET_COLOR}{RED_BG}{ch}{RESET_COLOR}") | ||
| visibleCount += 1 | ||
| elif tag == 'insert': | ||
| for ch in actual[j1:j2]: | ||
| if visibleCount >= maxChars: | ||
| break | ||
| diffLog.append(f"{RESET_COLOR}{RED_BG}{ch}{RESET_COLOR}") | ||
| visibleCount += 1 | ||
|
|
||
| return ''.join(diffLog) | ||
|
|
||
| @staticmethod | ||
| def _convertIterableFromString(expected, actual): | ||
| for i in range(len(expected)): | ||
|
|
@@ -82,9 +141,16 @@ def _convertIterableFromString(expected, actual): | |
| return actual | ||
|
|
||
| def _assertIterableEqual(self, expected, actual, msg: Optional[str] = None): | ||
| errorMsg = msg if msg else None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is doing nothing - msg will already be none if its not defined. |
||
|
|
||
| for i in range(len(expected)): | ||
| if expected[i] != actual[i]: | ||
| self._raiseFailure("output", expected[i], actual[i], msg) | ||
| if isinstance(expected[i], str): | ||
| errorMsg = f"Expected output line {i+1} does not match your output line {i+1}" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only display the last error message. If we do something like this were we are asserting in a loop, we prolly want to make it display a list of all the lines that dont match |
||
| if msg: | ||
| errorMsg += f"\n\n" + str(msg) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will add the message over and over again. |
||
|
|
||
| self._raiseFailure("output", expected[i], actual[i], errorMsg) | ||
|
|
||
| @staticmethod | ||
| def findPrecision(x: float): | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are missing a bunch of tests. Please add tests for the msg changes that you made. Please add tests covering delete and insert cases for the diff stuff. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| import re | ||
|
|
||
| from autograder_platform.TestingFramework.Assertions import Assertions | ||
|
|
||
|
|
||
|
|
@@ -49,6 +51,42 @@ def testAssertMultilineEqualFailure(self): | |
| with self.assertRaises(AssertionError): | ||
| self.assertMultiLineEqual("this\nis\na\nof\nlines", "this\nis\na\nof\nline") | ||
|
|
||
| def testAssertMultilineEqualFailureDiffLog(self): | ||
| expectedMsg= "Diff Log output: \x1b[0m\x1b[42ma\x1b[0m\x1b[0m\x1b[42mb\x1b[0m\x1b[0m\x1b[41mX\x1b[0m\x1b[0m\x1b[42md\x1b[0m\x1b[0m\x1b[42me\x1b[0m\x1b[31m" | ||
| with self.assertRaises(AssertionError) as ex: | ||
| self.assertMultiLineEqual("abcde", "abXde") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you only checking the multiline equal? |
||
|
|
||
| actualMsg = str(ex.exception) | ||
| self.assertIn(expectedMsg, actualMsg) | ||
|
|
||
| def testAssertMultilineEqualFailureDiffLogTruncatesAt200Chars(self): | ||
| expected = "b" * 205 | ||
| actual = "a" * 205 | ||
|
|
||
| with self.assertRaises(AssertionError) as ex: | ||
| self.assertMultiLineEqual(expected, actual) | ||
|
|
||
| actualMsg = str(ex.exception) | ||
| match = re.search(r"Diff Log output: (.*)$", actualMsg, re.DOTALL) | ||
| self.assertIsNotNone(match) | ||
|
|
||
| diff_log = re.sub(r"\x1b\[[0-9;]*m", "", match.group(1)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for everyone's sanity, you need to either explain this regex in a comment, or assert in a different way. |
||
| self.assertEqual(len(diff_log), 200) | ||
|
|
||
| def testAssertMultilineEqualFailureDiffLogTruncatesToActualLength(self): | ||
| expected = "b" * 100 | ||
| actual = "a" * 50 | ||
|
|
||
| with self.assertRaises(AssertionError) as ex: | ||
| self.assertMultiLineEqual(expected, actual) | ||
|
|
||
| actualMsg = str(ex.exception) | ||
| match = re.search(r"Diff Log output: (.*)$", actualMsg, re.DOTALL) | ||
| self.assertIsNotNone(match) | ||
|
|
||
| diff_log = re.sub(r"\x1b\[[0-9;]*m", "", match.group(1)) | ||
| self.assertEqual(len(diff_log), 50) | ||
|
|
||
| def testAssertFailureWithMsg(self): | ||
| expectedMsg = "doubles aren't ints" | ||
| with self.assertRaises(AssertionError) as ex: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i almost want to make the diff output configurable. IE you set a flag in the assertions class to enable it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default can be on ofc