assertEquals(Set, Set)

[Endre Stølsvik]

Hi!

The assertEquals(Set, Set) doesn't behave as Set.equals(set).

Is this intentional?

Calling assertEqual(...) with two Sets as arguments "hits" the
assertEqual(Collection, Collection), which seems to be made for arrays
and/or lists, in that all text there refer to "Arrays not equal" but
at the same time "Lists differ at ..", and that the implementation
takes the iterator for both collections, and steps them
simultaneously, failing on any mismatch, which IMO isn't quite
set-correct.

I find this suboptimal, as I specifically want to use Set logic in
some tests (unordered equals), and List logic in others (ordered
equals).

There's a method assertEqualsNoOrder(Object[], Object[]). This seems
along the lines of what I want (although I still see the above as a
bug), but there is no corresponding assertEqualsNoOrder(Collection,
Collection). The method could possibly be used as a workaround, but
upon inspection, I can't seem to understand how that method should
work either: The method seems to make use of the idea that Set-equals
is unordered (it makes two HashSets and sticks the array elements in
them, then calls assertEquals), which we just showed isn't the case.

Upon testing with a couple String[] {"a", "b", "c"} and String[] {"c",
"a", "b"}, I find that these actually does assert with
assertEqualsNoOrder. However, upon inspection, the HashSet "magically"
keeps these in the same iteration order, but this is obviously just
because of a spurious combination of small size, and the inner
workings of HashSet. Thus, the system should fail on a test where the
hashCode is bad, and on two much larger arrays which are
unordered-equal, but have different permutations.

Bad hash code, fails:
----------------
import org.testng.Assert;
import org.testng.annotations.Test;

public class XTestNGequalsNoOrder_BadHashCode {
@Test
public void test() {
BadHashCode a = new BadHashCode();
BadHashCode b = new BadHashCode();
BadHashCode c = new BadHashCode();
BadHashCode[] array1 = new BadHashCode[] { a, b, c };
BadHashCode[] array2 = new BadHashCode[] { a, b, c };
Assert.assertEqualsNoOrder(array1, array2);

// Fails:
array2 = new BadHashCode[] { c, b, a };
Assert.assertEqualsNoOrder(array1, array2);
}

private static class BadHashCode {
static int count;
int _count = count++;

@Override
public int hashCode() {
return 1;
}

@Override
public String toString() {
return "" + _count;
}
}
}

Random permutation of larger number of elements, fails:
----------------
import java.util.ArrayList;
import java.util.Collections;

import org.testng.Assert;
import org.testng.annotations.Test;

public class XTestnGequalsNoOrder_ManyRandom {
static int NUM = 2000;

@Test
public void test() {
ArrayList<String> source = new ArrayList<String>();
for (int i = 0; i < NUM; i++) {
source.add(i + ":" + Math.random());
}
String[] array1_sorted = source.toArray(new String[NUM]);
String[] array2_sorted = source.toArray(new String[NUM]);

Collections.shuffle(source);
String[] array1_random = source.toArray(new String[NUM]);

Collections.shuffle(source);
String[] array2_random = source.toArray(new String[NUM]);

Assert.assertEquals(array1_sorted, array2_sorted);
Assert.assertEqualsNoOrder(array1_sorted, array2_sorted);

// Fails:
Assert.assertEqualsNoOrder(array1_random, array2_random);
}
}

Quick suggestion for assertEquals(Set, Set, msg) (which thus should
fix both problems at once), or possibly instead for
assertEqualsNoOrder(Collection, Collection, msg) (in which case the
assertEqualsNoOrder(Object[], Object[], msg) must use this). If my
analysis is deemed correct, then please feel free to use it:
----------------
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Set;

import org.testng.Assert;
import org.testng.annotations.Test;

public class XTestNGequalsSetSet {

static int NUM = 2000;

@Test
public void test() {
ArrayList<String> source = new ArrayList<String>();
for (int i = 0; i < NUM; i++) {
source.add(i + ":" + Math.random());
}

List<String> list1 = new ArrayList<String>(source);
List<String> list2 = new ArrayList<String>(source);
Assert.assertEquals(list1, list2);

Collections.shuffle(list1);
Collections.shuffle(list2);
Set<String> set1 = new HashSet<String>(list1);
Set<String> set2 = new HashSet<String>(list2);

assertEquals(set1, set2, null);

// ========== The following shall fail: ============

list1.remove(500);
// list2.remove(500);

set1 = new HashSet<String>(list1);
set2 = new HashSet<String>(list2);

assertEquals(set1, set2, null);
}

/**
* Asserts that two Sets contain the same elements, but
potentially in different order. If they do not, an
* AssertionError, with the given message, is thrown.
*
* @param actual the actual Set of elements
* @param expected the expected Set of elements
* @param message the assertion error message
*/
static public void assertEquals(Set actual, Set expected, String message) {
if (actual == expected) return;

if ((actual == null && expected != null) || (actual != null &&
expected == null)) {
if (actual == null) {
Assert.fail((message != null ? message + ": " : "") +
"Actual is null, while expected is '" + expected
+ "'.");
}
Assert.fail((message != null ? message + ": " : "") +
"Expected is null, while actual is '" + expected
+ "'.");
}

Iterator actIt = actual.iterator();
int i = 0;
// To avoid dependency on the elements' hashCode()
List expectedArray = new ArrayList(expected);
while (actIt.hasNext()) {
Object a = actIt.next();
if (!expectedArray.contains(a)) {
Assert.fail((message != null ? message + ": " : "") +
"Sets differ at iterator position #" + i
+ ": Actual (size:" + actual.size() + ")
contains element '" + a
+ "' which doesn't exist in expected (size:" +
expected.size() + ").");
}
i++;
}
if (actual.size() != expected.size()) {
Iterator expIt = expected.iterator();
i = 0;
List actualArray = new ArrayList(actual);
while (expIt.hasNext()) {
Object e = expIt.next();
if (!actualArray.contains(e)) {
Assert.fail((message != null ? message + ": " :
"") + "Sets differ at iterator position #" + i
+ ": Expected (size:" + expected.size() +
") contains element '" + e
+ "' which doesn't exist in actual (size:"
+ actual.size() + ").");
}
i++;
}
}
}
}

Kind regards,
Endre.

[philv...@gmail.com]

I filed a related issue the other day as http://code.google.com/p/testng/issues/detail?id=48
but, I see now that I should have filed it on the JIRA site.

A failing testcase for assertEqualsNoOrder is:

String[] rto1 = { "boolean", "BigInteger", "List",};
String[] rto2 = { "List", "BigInteger", "boolean",};
assertEqualsNoOrder(rto1, rto2);

There's also an open bug that mentions that

String[] rto1 = { "a", "a", "b",};
String[] rto2 = { "a", "b", "b",};
assertEqualsNoOrder(rto1, rto2);

will yield a false positive.

I imagine the project owners are as hesitant as I would be to add a
bunch of overloaded collections methods to Assert. Perhaps it would
be good to add a separate "AssertCollections" class that could contain
these sorts of assertions. Even if this isn't integrated into TestNG,
I would still be interested developing a set of these asserts that
would have more correct semantics than the existing asserts.

--Phil

[Cédric Beust ♔]

On Sat, Feb 14, 2009 at 10:57 AM, philv...@gmail.com <philv...@gmail.com> wrote:

I imagine the project owners are as hesitant as I would be to add a
bunch of overloaded collections methods to Assert.

Right on, but maybe not for the reason you think.  I don't mind the number of methods on that class, but I don't want to break existing code, and there might already be TestNG users relying on the current assert behavior for sets.  Since this can be easily solved by implementing the assert that matches your expectations, it's probably better to just roll your own...

--

Cédric

[Cédric Beust ♔]

On Sat, Feb 14, 2009 at 10:57 AM, philv...@gmail.com <philv...@gmail.com> wrote:

I imagine the project owners are as hesitant as I would be to add a
bunch of overloaded collections methods to Assert.

[philv...@gmail.com]

Cédric, et al., would accept into the core TestNG code a separate
class only covering the collections which clearly defined its
semantics? I'm interested in doing this, but I guess the first thing
to determine is whether to do it under the umbrella of TestNG or as a
separate thing of my own. The subtly-broken semantics of
assertEqualsNoOrder definitely need something done, but it's not clear
if that's a simple (though possibly breaking to existing code) fix of
HashSet to TreeSet, or leaving the method implementation as-is,
deprecating it, and directing users to a more sematically-correct
class for collections.

I've gotten a lot of value out of TestNG in the last year and
genuinely want to contribute to make it better, but I have no
illusions I know "better" than the long-term contributors, which is
why I'm trying to be careful in my suggestions and guidance-seeking.

--Phil

On Feb 14, 12:08 pm, Cédric Beust ♔ <cbe...@google.com> wrote:
> On Sat, Feb 14, 2009 at 10:57 AM, philvar...@gmail.com <philvar...@gmail.com

>
> > wrote:
> > I imagine the project owners are as hesitant as I would be to add a
> > bunch of overloaded collections methods to Assert.
>
> Right on, but maybe not for the reason you think.  I don't mind the number
> of methods on that class, but I don't want to break existing code, and there
> might already be TestNG users relying on the current assert behavior for
> sets.  Since this can be easily solved by implementing the assert that
> matches your expectations, it's probably better to just roll your own...
>
> --

> ***Cédric
> *

[Putrycz, Erik]

What about improving assertEquals(Collection, Collection) and make a special case for sets? Endre has a good case about verifying the order.

Erik.

[Cédric Beust ♔]

Hi Phil,

Sure, I'd be happy to include a CollectionAssert class or similar, I'm just wondering if there would be enough methods in this class to justify the effort?

--

Cédric

[Endre Stølsvik]

On Feb 14, 9:08 pm, Cédric Beust ♔ <cbe...@google.com> wrote:
> On Sat, Feb 14, 2009 at 10:57 AM, philvar...@gmail.com <philvar...@gmail.com

>
> > wrote:
> > I imagine the project owners are as hesitant as I would be to add a
> > bunch of overloaded collections methods to Assert.
>
> Right on, but maybe not for the reason you think.  I don't mind the number
> of methods on that class, but I don't want to break existing code, and there
> might already be TestNG users relying on the current assert behavior for
> sets.  Since this can be easily solved by implementing the assert that
> matches your expectations, it's probably better to just roll your own...

How can Set,Set-equals be ordered? This is obviously a fault, plain
and simple. Maybe it is a legacy that's difficult to fix now, but it
is an obvious fault nonetheless. The text strings in the method
assertEquals(Collection, Collection) points rather heavily to some
"compaction" of code that went somewhat overboard.

But even then, the assertEqualsNoOrder(Object[], Object[]) is
obviously /utter broken/! There is simply no good reason for a method
to do something that's pretty much the opposite to what its name state
that it should do! The logic the method embodies now is actually ..
nonexistent! And what is even more obvious, is that this method
expected the equals of Sets to behave non-insanely.

Failing the inclusion of assertEquals(Set, Set) that works as obvious,
why wouldn't you insert a method assertEqualsNoOrder(Collection,
Collection)? And of course javadoc the extremely non-obvious behavior
of how Set-Set equality works with TestNG.

But anyways, I'm outta here. I converted all my tests to JUnit 4.5
yesterday, which took all of a couple of hours, and now I'm back with
the mainstream! The runner is nicer, there is no stupid XML to handle,
running my entire project is as easy as right-clicking the project
node and select "run as JUnit Test", and that new "assertThat" logic
including the Hamcrest library was lovely.

Thanks for the new ideas, though! I guess JUnit "NG" looked over to
the TestNG camp for ideas!

Endre.

[Endre Stølsvik]

On Feb 14, 7:57 pm, "philvar...@gmail.com" <philvar...@gmail.com>
wrote:
> I filed a related issue the other day ashttp://code.google.com/p/testng/issues/detail?id=48

> but, I see now that I should have filed it on the JIRA site.
>
> A failing testcase for assertEqualsNoOrder is:
>
> String[] rto1 = { "boolean", "BigInteger", "List",};
> String[] rto2 = {  "List", "BigInteger", "boolean",};
> assertEqualsNoOrder(rto1, rto2);
>
> There's also an open bug that mentions that
>
> String[] rto1 = { "a", "a", "b",};
> String[] rto2 = {  "a", "b", "b",};
> assertEqualsNoOrder(rto1, rto2);
>
> will yield a false positive.

Wow! How obvious! And how BAD!


I just checked through the 49 bug reports (where exactly _none_ have
had any progress since its start), and found this rather obvious one:
"assertEquals(Set, Set) has incorrect implementation", posted Nov 26,
2008:
http://code.google.com/p/testng/issues/detail?id=40

The one comment on this bug says a bit:

" I reported this almost 2 years ago but the main developers then
didn't understand the
seriousness in it.

The problem is that the assertEquals(Collection,Collection) checks
order. Something
that should NOT be done on a Collection as many Collections lack order
(eg Sets, and
Bags) and order is not part of the Collection interface (you cannot
sort a
Collection, but you can sort a List).

My proposal was to change the current method signature to assertEquals
(List, List)
and make a new assertEquals(Collection, Collection) which would only
break faulty
legacy tests (at least would be happy if buggy code failed tests).

Note since assertEqualsNoOrder uses assertEquals(Collection,
Collection) it is flawed
to so don't use that either. "

So, don't hold your breath waiting for TestNG to behave sanely.

Endre.

[Endre Stølsvik]

On Feb 16, 7:49 pm, Endre Stølsvik <stols...@gmail.com> wrote:
> On Feb 14, 7:57 pm, "philvar...@gmail.com" <philvar...@gmail.com>
> wrote:
>
>

>  http://code.google.com/p/testng/issues/detail?id=40

This bug from 20/Feb/06 07:47 obviously has the wrong resolution.
Set.equals(set) logic should OBVIOUSLY be what the "assertEquals" use,
none other. The JavaDoc for Set.equals(...) states rather plain and
straightforward: "Returns true if the specified object is also a set,
the two sets have the same size, and every member of the specified set
is contained in this set (or equivalently, every member of this set is
contained in the specified set). This definition ensures that the
equals method works properly across different implementations of the
set interface.")
http://jira.opensymphony.com/browse/TESTNG-36

And then we have
http://jira.opensymphony.com/browse/TESTNG-229
http://jira.opensymphony.com/browse/TESTNG-228

So, as mentioned, don't hold your breath. Or breathe freely, using the
obvious competitor.

Endre.

[Cédric Beust ♔]

On Sat, Feb 14, 2009 at 10:57 AM, philv...@gmail.com <philv...@gmail.com> wrote:

I filed a related issue the other day as http://code.google.com/p/testng/issues/detail?id=48
but, I see now that I should have filed it on the JIRA site.

A failing testcase for assertEqualsNoOrder is:

String[] rto1 = { "boolean", "BigInteger", "List",};
String[] rto2 = {  "List", "BigInteger", "boolean",};
assertEqualsNoOrder(rto1, rto2);

There's also an open bug that mentions that

String[] rto1 = { "a", "a", "b",};
String[] rto2 = {  "a", "b", "b",};
assertEqualsNoOrder(rto1, rto2);

will yield a false positive.

You're right, this is clearly a bug, I just fixed it in the trunk.

Thanks, Phil, let me know if you see anything else...

--

Cédric

[Endre Stølsvik]

On Feb 16, 8:19 pm, Cédric Beust ♔ <cbe...@google.com> wrote:
> On Sat, Feb 14, 2009 at 10:57 AM, philvar...@gmail.com <philvar...@gmail.com

>
>
>
> > wrote:
> > I filed a related issue the other day as
> >http://code.google.com/p/testng/issues/detail?id=48
> > but, I see now that I should have filed it on the JIRA site.
>
> > A failing testcase for assertEqualsNoOrder is:
>
> > String[] rto1 = { "boolean", "BigInteger", "List",};
> > String[] rto2 = {  "List", "BigInteger", "boolean",};
> > assertEqualsNoOrder(rto1, rto2);
>
> > There's also an open bug that mentions that
>
> > String[] rto1 = { "a", "a", "b",};
> > String[] rto2 = {  "a", "b", "b",};
> > assertEqualsNoOrder(rto1, rto2);
>
> > will yield a false positive.
>
> You're right, this is clearly a bug, I just fixed it in the trunk.

Which bug did you fix?! See, Phil demonstrated TWO bugs. Both of them
preexisting as bug reports already.

Endre.

[Cédric Beust ♔]

2009/2/16 Endre Stølsvik <stol...@gmail.com>

The first example worked right out of the box for me, only the second one was incorrect.

Sync the trunk and take a look at test/src/test/asserttests/AssertTest.java.  Feel free to add failing test cases there and I'll fix them.

--

Cédric

[Endre Stølsvik]

> > > You're right, this is clearly a bug, I just fixed it in the trunk.
>
> > Which bug did you fix?! See, Phil demonstrated TWO bugs. Both of them
> > preexisting as bug reports already.
>
> The first example worked right out of the box for me, only the second one
> was incorrect.

Did you read the post that opened this thread, or run the two failing
test cases I wrote and included there?!

Endre.

[Cédric Beust ♔]

2009/2/16 Endre Stølsvik <stol...@gmail.com>

Yes.  Did you try to run these tests with my fix?

--

Cédric

[Endre Stølsvik]

> > > > > You're right, this is clearly a bug, I just fixed it in the trunk.
>
> > > > Which bug did you fix?! See, Phil demonstrated TWO bugs. Both of them
> > > > preexisting as bug reports already.
>
> > > The first example worked right out of the box for me, only the second one
> > > was incorrect.
>
> > Did you read the post that opened this thread, or run the two failing
> > test cases I wrote and included there?!
>
> Yes.  Did you try to run these tests with my fix?

No. Your mention of taking my tests into consideration must have
slipped by me.

Endre.

PS: What is the point of requiring people that want to view the
archives of these google groups to be subscribers? When they are
already indexed by both nabble and markmail?

[Cédric Beust ♔]

2009/2/16 Endre Stølsvik <stol...@gmail.com>

PS: What is the point of requiring people that want to view the
archives of these google groups to be subscribers? When they are
already indexed by both nabble and markmail?

Didn't realize it was so, I fixed it.

--

Cédric