Equality test should not require arguments to be orderable

[jgfoster]

cpp/unittest/Compare.h: 13 and 14 seem to test equality in terms of ordering. I'd like to be able to compare two objects that understand equality without implying an order ("Is Adam < Eve?").

/Users/jfoster/code/Arduino/LiquidCrystal/vendor/bundle/ruby/2.6.0/gems/arduino_ci-0.3.0/cpp/unittest/Compare.h:13:71: error: invalid operands to binary expression ('const BitCollector' and 'const std::__1::vector<int, std::__1::allocator<int> >')
  inline static bool equal(const A &a,const B &b)       { return (!(a < b)) && (!(b < a)); }
                                                                    ~ ^ ~
/Users/jfoster/code/Arduino/LiquidCrystal/vendor/bundle/ruby/2.6.0/gems/arduino_ci-0.3.0/cpp/unittest/Compare.h:107:107: note: in instantiation of member function 'Compare<BitCollector, std::__1::vector<int, std::__1::allocator<int> > >::equal' requested here
template <typename A, typename B> bool compareEqual(      const A &a, const B &b) { return Compare<A, B>::equal(      a, b); }
                                                                                                          ^
/Users/jfoster/code/Arduino/LiquidCrystal/test/Common.cpp:143:3: note: in instantiation of function template specialization 'compareEqual<BitCollector, std::__1::vector<int, std::__1::allocator<int> > >' requested here
  assertEqual(pinValues, expected);
  ^

In particular, I'd like to implement operator==() in my subclass of DataStreamObserver so that I can encapsulate the comparison and not have to do it in every test. The work-around isn't too hard, but this seems like an unnecessary limitation.

[ianfixes]

This is something that has come up before (see #141) and I don't have a great answer. It's something that I adopted from another Arduino testing library: https://github.com./mmurdoch/arduinounit/blob/master/src/ArduinoUnitUtility/Compare.h

The implementation on this (as you can see) is a macro, and my understanding from last time I looked into this relates to things like floating-point numbers, in which equality is a difficult thing to capture.

I never dove into the subtlety of this topic and would welcome the advice of someone more knowledgable, or at least the set of use cases that show why it should be one way or the other.

[jgfoster]

I'd be surprised if this approach improved the situation. Dealing with float representations is vastly, hugely, mind-bogglingly hard. I'd expect that if two floats were not equal, then one would be greater than the other. One common approach is to have a dedicated assertEquals() function which takes an additional delta parameter to describe the maximum amount of rounding error which is acceptable. And for those that want to do such comparisons without adding any new code, there is assertLess(abs(x-y),eps).

I feel that the current approach adds no value and prevents otherwise useful equality comparisons.

[ianfixes]

I tried to dig into this a little bit, and I'm not sure I have clear idea of how to move forward.

In my previous reply, I had forgotten that the dependency on inequality operators is what allows us to simply define a larger set of assert functions than we otherwise could: https://github.com./Arduino-CI/arduino_ci/blob/master/cpp/unittest/Assertion.h#L33-L42

It sounds like the trick here is come up with an alternate macro for raw equality / inequality (i.e. one not based on < and >) so they can exist alongside each other. That's as simple as coming up with reasonable names, I think. (I'd repurpose assertEqual immediately with ==, but it has to somehow align with assertLessOrEqual. I don't want to get in a situation where assertLess, assertLessOrEqual, and assertEqual aren't in agreement by virtue of being based on conflicting measures of equality.)

The workaround in the meantime is just to assertTrue(x == y) if you really want a order-less equality test.

[ianfixes]

The workaround in the meantime is just to assertTrue(x == y) if you really want a order-less equality test.

After some thought I'm very tempted to say that this is the correct course of action.

What I'm struggling to come up with is a use case where the un-orderable things you're comparing will fill out the error message in the macro properly. In other words, I can't imagine a type for which the error message would read something like "Expected the value of x but instead got the value of y"... unless those values are just random pointers, which wouldn't convey any useful information in the error.

Can you think of an instance where that wouldn't be the case?

[jgfoster]

It seems to me that there are two questions here:

1. Should orderability be required to test for equality?
2. Should objects that might not describe themselves in a useful manner be allowed to compare themselves?

It appears to me that you are suggesting that objects that are orderable typically have good string representations, so can be allowed to compare for equality.

It seems to me that these issues are sufficiently unrelated so that one should not influence the other. It seems to me that an error message of "Expected Adam but got Eve" is more useful than "Expected true but got false". Yes, objects that might get printed should have a way to describe themselves, but that issue exists anyway, and will manifest itself in many situations.

How will we explain this later?
Q: Why can't we compare x and y for equality?
A: Because if they aren't equal, you might sometimes get an error message that doesn't describe the objects very well.
Q: So, instead we always get an error message that tells us nothing about the objects?
A: Yes!

It seems to me that if an object understands equality comparison, then we should be able to test it for equality.

[ianfixes]

It appears to me that you are suggesting that objects that are orderable typically have good string representations, so can be allowed to compare for equality.

That's part of it. On one hand, anything that implements an ostream::operator << would be printed out nicely -- "Expected Adam but got Eve", as you say.

But on the other hand, "Expected Adam but got Adam" wouldn't be helpful either. So my reasoning is that for the string representation to be useful in the first place, one would have to know in advance that unequal objects would produce unequal string representations.

The other points that I'm considering when it comes to this are

• Bjarne Stroustrup's article about why no default implementation of == is (was) provided by the compiler

  The killer argument for default comparisons is not actually convenience, but the fact that people get their equality operators wrong. They forget to compare all elements (especially when updating a struct with a new member). Also, given inheritance, a X::operator==(const X&) is easy to misuse (whether virtual or not). We get slicing (b==d will work even if b and d are of different classes in a hierarchy, and usually give a wrong answer). If people define operator==() as a member; then the left- and right-hand operand obey different conversion rules. Also, defining the semantics for == and != is relatively simple, but there are subtleties about other operators, such as <=; does its meaning involve < and == or < and !? Contrary to “common knowledge” there seems to be little difference in performance.

  Lots of people joined the discussions and the discussions went all over the place. How do we handle pointers? (we don’t), how do we handle mutable members?, should we use static reflection?, etc., etc., At times, I thought that we would get nothing. That wouldn’t be a tragedy because default comparisons are not on my (mostly mythical) top-20 list of desirable improvements to C++.

• StackOverflow answer about NaN equality

  double n = std::numeric_limits<double>::quiet_NaN();
  
  std::cout << "NaN == NaN: " << (n == n) << '\n'; // false
  std::cout << "NaN < NaN: " << (n < n) << '\n';   // false
  std::cout << "NaN > NaN: " << (n > n) << '\n';   // false
  std::cout << "NaN != NaN: " << (n != n) << '\n'; // true

I still haven't made up my mind but I was surprised how deep this particular rabbit hole went.

[jgfoster]

[Although I'm continuing the discussion, I can live with the work-around and don't consider this a "hill to die on"!]

As you have noted, equality comparison can be a complicated topic and is domain-specific. While some domains, particularly rational numbers, have well-accepted definitions for equality, after that it gets murky. While strings are, in some sense, orderable, is "Ian" == "IAN"? Yes, if you are doing case-insensitive comparisons (for a person name lookup). What about diacritical marks? Is "resume" the same as "résumé"? Yes, if you are doing a dictionary lookup, or simple sorting.

And, yes, if the string representation of an object doesn't include all the fields involved in an equality comparison, then you could get something like "Expected Adam but got Adam", which is not perfect (but still much more informative than "Expected true but got false"). While these are all interesting philosophical topics (a deep rabbit hole indeed!), they are still tangential to the underlying issue.

We allow assertEqual(x, y) only if x and y are orderable. But if they are not orderable, then we don't allow it and you are left with assertTrue(x == y). All the interesting discussions about equality remain, we are just forced to use a different syntax that is less informative. It seems to me that if the second test passes then the first test should pass as well. If I've defined operator==() on my objects, then an equality test should not require me to also implement < and >. And if I did (say, sorting on age, height, weight, IQ, or credit score), we could still end up with "Expected Adam but got Adam", so the readability ambiguity is unrelated to orderability.

If potential ambiguity is a reason to disfavor assertEqual(), then why not remove it altogether? Depending on the font, you could find "Expected 'lan' but got 'Ian'" to be confusing (l vs. I). Does the orderability of strings make this confusion more acceptable? I don't think so!

You are providing a testing framework to developers. Issues of how to define equality are the responsibility of those developers, but should not matter to the testing framework. If I can test for x == y then I should be allowed to test for equality of (x, y).

Orderability should not be a determining factor in whether to allow equality comparison.

[ianfixes]

I can live with the work-around

Please don't get the wrong impression, I'm very happy to be having this discussion and it's causing me to check quite a few of my own assumptions (in a good way)

You are providing a testing framework to developers

This is my main guiding principle here, I want to make sure that any changes I might make to the framework are very durable. I was going to have to explore this topic in depth no matter what.

But I think I got to the bottom of the issue on my end -- the framework is reporting a == b when under the hood it's really (a - b) == 0. I'm working on at least making these macros honest (between their name, the text they produce, and their actual implementation) and making space for both "double equals" equality and "float/epsilon-based" equality.

[ianfixes]

I'm curious for your thoughts in PR #238 , which adds the ability to compare equality when there isn't a total ordering (greater than / less than operators) in the class.

It's mostly an alignment of the operator used, the text printed to the screen, and the name of the function itself. I kept the original ones too, with more descriptive names. Given that this is a reimplementation of a builtin function, I'm trying to ensure that the behavior would be the same in all cases. Please let me know if you can think of other use cases I should be testing.

[jgfoster]

I've added comments to the PR. I like aligning the name, operator, and string. I don't think of anything else (but I didn't think too hard since others can be added later as needed).