boostorg / core

Boost Core Utilities
132 stars 87 forks source link

Added BOOST_TEST_ALL_EQ macro to compare container contents #26

Closed breese closed 7 years ago

breese commented 7 years ago

@glenfe Good idea. Patch updated.

pdimov commented 7 years ago

When the sizes differ, it would be nice if we output them.

The loop doesn't look correct to me. You should add a test for the fail case, something like

int main()
{
    vector<int> x, y;
    x.push_back( 1 ); x.push_back( 2 ); x.push_back( 3 ); x.push_back( 4 );
    y.push_back( 1 ); y.push_back( 3 ); y.push_back( 2 ); y.push_back( 4 );
    BOOST_TEST_ALL_EQ( x.begin(), x.end(), y.begin(), y.end() );

    boost::report_errors();

    return boost::detail::test_errors() != 1; // more, when more cases are added
}
pdimov commented 7 years ago

No, the fail test shouldn't be run-fail, it should be run.

breese commented 7 years ago

@pdimov I am not sure why it should be run.

With run-fail I get a failed-as-expected, and with run I get a noisy failed testing.capture-output.

pdimov commented 7 years ago

Well I used != for a reason. We want to test that the number of failures matches what we expect.

pdimov commented 7 years ago

return errors != 5 is the same as return errors == 5? 0: 1, that is, when we get 5 errors - which we expect - the test passes, and when we don't get 5 errors, it fails.

glenfe commented 7 years ago

It should probably be just implemented in terms of Input iterators instead of Forward iterators. It makes it more useful (and losing the distance check at the start to fail faster is fine).

You also want to output test_output_impl(first_it) and test_output_impl(second_it) like the other functions do.

breese commented 7 years ago

@pdimov What confuses me is that you suggest that my failing test cases should be handled differently than the other failing lightweight_test test cases. I do not mind, and I can easily make the changes, but I just want to make sure first.

breese commented 7 years ago

@glenfe The distance check was primarily a safe-guard for std::mismatch, but since that is no longer used, I have changed the forward iterators to input iterators.

Differing sizes are now reported when we reach the end of one of the containers.

pdimov commented 7 years ago

The traditional way would require a file per test. I should've made the other fail tests use this technique too, but that didn't occur to me at the time.

pdimov commented 7 years ago

There should probably be a limit for the number of differences that are printed, for the case where the sizes are large. Print the first 8 for example and exit the loop?

breese commented 7 years ago

@pdimov I agree about the upper limit. Patch added.

pdimov commented 7 years ago

This looks good.

In the documentation, "containers" probably should say "sequences" now, and since the test is nominally positive, it might be better to call it something like lightweight_test_all_eq_test instead, and I think we'll be done here. Thanks Bjørn.

pdimov commented 7 years ago

Perfect. Thanks again. Will merge shortly.