added general floating point near comparison

[helloworld922]

The EXPECT_NEAR and ASSERT_NEAR macros expect double parameters, which potentially means loss of precision or for types with greater accuracy than double, or compile errors for types not implicitly convertible to double (gcc/intel quad precision long double, Boost.MultiPrecision, etc.).

This is essentially the same fix as the patch I submitted in issue #890. I fixed a problem where I called the wrong absolute value funciton (abs vs. fabs).

Side note: I don't know why the original bug report was closed without comment.

[gennadiycivil]

Please also add unit test with clearly reproducible behaviour

[helloworld922]

I added unit tests which test EXPECT_NEAR/ASSERT_NEAR for long double and a "custom real" type

[gennadiycivil]

Just to give you an update, during internal testing this change breaks a number of existing tests. it nay be a while before I can get to it

[daravi]

Can we rename the functions and the commit to be more general? Maybe call it "ordered type near comparison". I would like to be able to compare types that define a comparison operator. For example:

using namespace std::chrono;
EXPECT_NEAR(my_obj->getTimeSinceLastUpdate(), system_clock::now(), seconds(1));

[gennadiycivil]

Closing , looks abandoned

[helloworld922]

Your last message to me was to wait on you to fix internal testing, specifically "it may be a while before I can get to it"

Has this issue with internal testing been resolved?

[gennadiycivil]

Apologies, I did not make it clear. The internal testing with existing millions of tests blocks this change in this form.

[helloworld922]

What needs to be modified with this commit so this type of change could be pulled?

[chiphogg]

This is a great question. How can this change be moved forward? It would fix a pretty frustrating oversight in the design of EXPECT_NEAR.

[skorokhod]

Any news on this?
I'm looking for some way to compare chrono::time_points with some precision bit it looks like gtest has nothing to deal with it.

[chiphogg]

Let me suggest a way forward based on my admittedly imperfect information. 🙂

It seems very likely to me that if EXPECT_NEAR were being written today, and the author had these two implementations to choose from, they would choose the form introduced in this PR over the form now on master, every time. That's my explicit assumption; please let me know if I'm mistaken! But it seems clear to me that "compare types directly and natively" is strictly better than "implicitly convert everything to a double"... especially considering the generic name EXPECT_NEAR.

If I'm right, the blocking tests are the equivalent of "behaviour which depends on a bug". If so, then to fix this situation, we need to remove the dependency on the bug. One way to do this would be for Google to add another macro which preserves the current behaviour---say, EXPECT_NEAR_DOUBLE---internally in their monorepo. You could then use rosie to migrate the offending tests to this alternative. That should unblock this PR.

Other repositories "in the wild" may also depend on this bug, and this patch would break them. I think this is OK because it would only break them when they consciously choose to upgrade their googletest version. It's very common to need to deal with slight behaviour changes when upgrading to a new version of a dependency.

@gennadiycivil (as previous participant in this thread) and @mattcalabrese-google (as assignee of #890): WDYT about this analysis, and the approach I suggest? Have I missed anything important? Let's see if we can figure out a way forward.

[daravi]

Failing that I suggest introducing a new macro such as EXPECT_APPROX or some other name with the more logical behaviour.

[chiphogg]

I like that solution, especially if EXPECT_NEAR is also deprecated in future googletest versions. The advantage is that it would also avoid breaking any existing users. The downside is that EXPECT_APPROX is a slightly worse name, and I'd rather pair the best behaviour with the best name... but, very often, the perfect is the enemy of the good. 🙂

@gennadiycivil, @mattcalabrese-google: we now have two viable solutions that can unblock this work.

1. Migrate Google's dubious usages of EXPECT_NEAR to a new, well-named macro that preserves the present behaviour.
2. Provide a new macro---probably EXPECT_APPROX---which does what users expect; probably deprecate EXPECT_NEAR also.

Are you folks able to weigh in on which of these approaches you would prefer?

[chiphogg]

Hi! Friendly ping. 🙂 I appreciate that folks are surely busy, but I thought I'd check in to see if we can move the conversation forward.

• Does Google consider the present, behaviour of EXPECT_NEAR (i.e., restricting to arguments that can be converted implicitly to double) to be desirable?
• If not: does Google plan to fix it? And if so: which above approach would Google prefer, if either?

Thanks!

[chiphogg]

Hi! Happy 2024. Can @gennadiycivil, @mattcalabrese-google, or anyone else at Google please respond to the discussion starting in this comment?

[chiphogg]

For folks who have been following the conversation here, where it has mainly been taking place: I decided to put my latest ping on the issue page #890, because an open issue is a better place for conversation than a (temporarily? 🙏) closed PR. 🙂