Fixed TODO to improve unordered string comparision #186
Conversation
| @@ -148,6 +148,17 @@ public void testEntrySet() { | |||
| "{0=0, 1=1, 2=2, 3=3, 4=4, 5=5, 6=6, 7=7, 8=8, 9=9}", entrySet.toString()); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Please use @Test(expected=....) to simplify the test when it should pass when an exception is thrown.
There was a problem hiding this comment.
Will incorporate these changes
| Arrays.sort(actualTokenized); | ||
| Arrays.sort(expectedTokenized); | ||
| if (Arrays.equals(actualTokenized, expectedTokenized)) { | ||
| Assert.assertTrue(msg + ": " + expectedTokenized + "=/= " + actualTokenized, true); |
There was a problem hiding this comment.
assertTrue(message, true) can never fail. What is the goal of this assert?
There was a problem hiding this comment.
It seems you want to use assertArraysEquals.
There was a problem hiding this comment.
sure. we will incorportate this
|
|
||
| protected void assertEqualstUnordered(String msg, String expected, String actual) { |
There was a problem hiding this comment.
Where is assertEqualsUnordered used?
There was a problem hiding this comment.
The class HashMapTest utilizes this function in the following test method
smokeTest
testKeySet
testValues
kavvya97
force-pushed
the
nondex_todo_fixAssertEqualsUnordered
branch
from
d01c6b6 to
c1f2556
Compare
| @@ -148,6 +148,12 @@ public void testEntrySet() { | |||
| "{0=0, 1=1, 2=2, 3=3, 4=4, 5=5, 6=6, 7=7, 8=8, 9=9}", entrySet.toString()); | |||
| } | |||
|
|
|||
| @Test(expected = AssertionError.class) | |||
| public void testAssertEqualsUnorderedFailsWithDuplicates() { | |||
| this.assertEqualstUnordered("the strings are not a permutation of each other", | |||
There was a problem hiding this comment.
Please rename to assertEqualsUnordered (without t).
|
|
||
| protected void assertEqualstUnordered(String msg, String expected, String actual) { | ||
| String trimmed = expected.substring(1, expected.length() - 1); | ||
| String[] actualTokenized = this.formatString(actual); |
There was a problem hiding this comment.
Is formatString the best name? The method seems to be splitting a collection into elements. Even the input is not an arbitrary msg.
kavvya97
force-pushed
the
nondex_todo_fixAssertEqualsUnordered
branch
from
c1f2556 to
b17b73c
Compare
| Assert.assertEquals(msg + ": " + expected + " =/= " + actual, expected.length(), actual.length()); | ||
| String trimmed = expected.substring(1, expected.length() - 1); | ||
| private String[] trimAndSplitStrings(String msg) { | ||
| String trimmed = msg.substring(1, msg.length() - 1); | ||
| String[] elems = trimmed.split(","); |
There was a problem hiding this comment.
Can it split(", ") and then not even require the loop for trim()?
There was a problem hiding this comment.
yes that should work. Will update the PR
| @@ -46,7 +48,7 @@ protected void assertParameterized(T ds, Object derived, String str) { | |||
| case FULL: | |||
| String tempStr = derived.toString(); | |||
| Assert.assertNotEquals("FULL is improperly running", str, tempStr); | |||
| this.assertEqualstUnordered("Does not match permutation", str, tempStr); | |||
| this.assertEqualsUnordered("Does not match permutation", str, tempStr); | |||
There was a problem hiding this comment.
Any idea why they ever called it Equalst? Was it a misspelling or some joke about EqualsString? Does the Git log show anything useful?
There was a problem hiding this comment.
Hello Professor, I checked the git blame and from that I can infer that it was initially called as such. this was the first time when the function was commited/created
d4f6e03#diff-b6d9573a5585dde09208a701b942a04b7e8017e4916381997baa33cf6d32edabR38
kavvya97
force-pushed
the
nondex_todo_fixAssertEqualsUnordered
branch
from
b17b73c to
f1d52fd
Compare
Setup
Description
This PR addresses the following TODO in the test utility function:
https://github.com/kavvya97/NonDex/blob/8843b436802af4a0850f63d6ae0f231164dafe78/nondex-test/src/test/java/edu/illinois/nondex/core/AbstractCollectionTest.java#L63
The function
assertEqualstUnorderedchecks whether the contents of two data structures are equal without considering the order of the elements. Currently, it converts the content of both structures into string form and checks if the strings have the same length. If they do, it tokenizes one of them by splitting on commas to obtain multiplekey=valueelements. Then, it checks if each element is present in the other string as a substring.https://github.com/kavvya97/NonDex/blob/8843b436802af4a0850f63d6ae0f231164dafe78/nondex-test/src/test/java/edu/illinois/nondex/core/AbstractCollectionTest.java#L66
However, this approach doesn't account for duplicates or substrings in the data structures. For example, consider the two strings:
"{0=0, 0=0, 1=1, 2=2}"and"{0=0, 1=1, 2=2, 1=1}". These two strings should not be considered equal, but the current test considers them equal.Fix
Both strings are tokenized in the same manner as described above. The tokenized strings are then sorted and checked for equality. This approach will detect cases where duplicates differ between the two strings.
Verification
assertEqualstUnorderedfails this test.