boostorg / core

Boost Core Utilities
132 stars 87 forks source link

new test backend based on predicates #39

Closed HDembinski closed 5 years ago

HDembinski commented 5 years ago

See #38

The test backend uses predicates now, which reduces code duplication and makes the system more flexible. I implemented BOOST_TEST_CLOSE using this new flexibility (also added tests).

BOOST_TEST_CLOSE uses the predicate close_to suggested by @breese.

pdimov commented 5 years ago

It seems to me that converting the second argument to the type of the first would break BOOST_TEST_GT('x', -1), BOOST_TEST_EQ(nullptr, p), BOOST_TEST_NE(&y, &x) where Y derives from X, and so on.

breese commented 5 years ago

@glenfe close_to is not really restricted to unit-tests. It is useful wherever you have to compare floats for equality, e.g.

  if (close_to()(number, 0.4)) { /* almost 0.4 */ }

It would even make sense to propose it for inclusion in <functional>.

glenfe commented 5 years ago

@breese When or if a Boost library has a use for it, it can be moved out of the detail namespace, and then properly documented (and tested independently of the Lightweight test macros) as part of Boost.Core.

HDembinski commented 5 years ago

I think the new version addresses all the concerns, let me know if I forgot something.

pdimov commented 5 years ago

This looks sensible and is very close to what I envisaged as a design. Would you please rename the predicates along the lines of lw_test_eq instead of the generic equal_to so as not to pollute detail, and perhaps split this PR into two, one refactoring the existing functionality into predicate form, a separate one introducing close_to?

HDembinski commented 5 years ago

Ok, will do. I was am aware of the "one-topic per PR" rule, but then considered the two patches to be so closely related, that I put them in one. Also to help with the discussion, so that it is clear where this is going.

HDembinski commented 5 years ago

I suppose you also want a separate PR for line 209, where I corrected a mistake in the output print.

glenfe commented 5 years ago

I suppose you also want a separate PR for line 209, where I corrected a mistake in the output print.

No, you can include that in the current PR. :)

HDembinski commented 5 years ago

Ok, that's a relief :)

pdimov commented 5 years ago

As a matter of fact yes I do. -)

The reason to split separate things into their own PRs is not to adhere to a rule, but to allow them to be merged separately without being tied up together (and then reverted separately if need be.) This way straightforward changes can be applied immediately without waiting for the rest to crystallize.

Could you move the BinaryPredicate to be the first argument of test_with_impl? Thanks.

HDembinski commented 5 years ago

@pdimov the predicate is also the last argument in test_all_with_impl. If I do as you suggest, it is inconsistent between test_with_impl and test_all_with_impl

HDembinski commented 5 years ago

Otherwise I think I see why you request this. It is simply easier to see whether the correct predicate is used for a given BOOST_TEST... call when it is the first argument. But then we perhaps should also change test_all_with_impl in the same way.

pdimov commented 5 years ago

Yes, I understand why you followed the style of the existing test_all_with_impl, but I still prefer the predicate first in this case. Let's leave test_all_with_impl as is.

pdimov commented 5 years ago

Instead of BinaryPredicate::op(), pred.op() please. Makes no difference here, of course, but in principle, the op may depend on state. (The anti_op for close_to(0.001) could for instance return !=[within 0.001].)

Could also rebase on develop to pick up the new tests I added.

HDembinski commented 5 years ago

That is a great idea, I would like that for close_to. Also I see now that I unnecessarily restricted the code, blocking possible future extensions.

HDembinski commented 5 years ago

Thank you everyone for the merge and review!