Revert "report_errors with expected failures (#51)"

[Lastique]

This reverts commit 02041f6c9f94d9e5d69f7c36f08624e1001143a0 and https://github.com/boostorg/core/pull/51.

Reason for revert: The change allows for incorrect result code returned from the process if the number of failures is a multiple of 256. This may result in test failures being taken as success by the parent process.

[pdimov]

This should fail all tests that check for the correct number of failures, as mentioned. How about we apply https://github.com/boostorg/core/commit/eba08e936a807e0cae03c0d50160457f578b71aa instead.

[Lastique]

I'd rather have a different solution that allows to obtain the number of failures without hacks. Either a separate function (get_failure_count) or by adding an optional argument to report_errors (e.g. int report_errors(int& failure_count);). We can think of a better solution in due time, but for now and especially for 1.71 I'd rather revert to a bug-free version first.

This should fail all tests that check for the correct number of failures

Do you know which tests these are?

[pdimov]

What are the remaining bugs in this version?

Hans's original suggestion was for return report_errors(expected); instead of the current return report_errors() == expected? 0: 1;

I gave this a fair bit of thought; what made me choose the latter was the observation that I already had return report_errors() == expected; in a number of places, that is, I already intuitively expected it to return the number of failures. This "worked" only because the two errors canceled out.

[Lastique]

Hans's original suggestion was for return report_errors(expected); instead of the current return report_errors() == expected? 0: 1;

This solves the original problem, so I'm fine with it. I just think get_failure_count would be more flexible.

I gave this a fair bit of thought; what made me choose the latter was the observation that I already had return report_errors() == expected; in a number of places, that is, I already intuitively expected it to return the number of failures.

Maybe we can think of some sort of protection from mistakes like that? E.g. return a wrapper class that has comparison operators = delete and can be converted to int?

In the meantime, do you have objections if I merge this PR? We can create a new PR for the final solution.

[pdimov]

Yes, I do have objections.

What bugs remain after https://github.com/boostorg/core/commit/eba08e936a807e0cae03c0d50160457f578b71aa?

[Lastique]

What bugs remain after eba08e9?

It returns the wrong value if there are more than 255 failures.

[pdimov]

It's significantly less wrong than always returning 1.

[Lastique]

Well, no, given that report_errors did not return the number of failures (until #51). That someone assumed it did was a user error.

[Lastique]

Anyway, I don't see why we should argue about making it more or less "wrong" when we can just make it "right".

[pdimov]

Well, no, given that report_errors did not return the number of failures

Arguably a bug, because the documentation has always stated it did.

[Lastique]

The test in Boost.Core are broken even if you assume report_errors return the number of failures.

[Lastique]

Arguably a bug, because the documentation has always stated it did.

The docs also say to use return boost::report_errors(); in main. So report_errors returning the number of failures contradicts that advice. I say, stop pretending that report_errors returns the number of errors.

[breese]

POSIX confirms the 8-bit limitation.

[pdimov]

The test in Boost.Core are broken even if you assume report_errors return the number of failures.

How are they broken?

[Lastique]

@pdimov Could you look at https://github.com/Lastique/core/commit/a4c98cebaabfd0bb11c52cd73234efa02a9c5c50?

How are they broken?

They contain return boost::report_errors() == test_cases;, which means returning 1 when there are no errors. Yes, they are run-fail in the Jamfile, but that is counter-intuitive.

[pdimov]

Counter-intuitive is not the same as broken.

[Lastique]

Counter-intuitive is not the same as broken.

True. I was writing "broken" before I found out they are run-fail. Still worth fixing.

[glenfe]

@Lastique a4c98ce seems overkill. Instead of return boost::report_errors() == N it could be something like:

BOOST_REQUIRE_ERRORS(N);
return boost::report_errors();

Where report_errors() is reverted to its old behavior (returning 0 or 1) and the documentation changed to reflect that this is what it does.

This also allows us to do things like:

{
    // test things
    BOOST_REQUIRE_ERRORS(N);
    // test things
    BOOST_REQUIRE_ERRORS(M);
    return boost::report_errors();
}

The alternative spelling of BOOST_REQUIRE_ERRORS(N) could easily be something like BOOST_REQUIRE(boost::test_errors() == N).

(The function test_errors() already exists and is currently in boost::detail namespace but could easily be for public consumption in the boost namespace).

[Lastique]

@glenfe BOOST_REQUIRE_ERRORS can't be a check because it will just add/not add to the error count while the intention is to return 0 from main when the total number of errors matches some condition.

Also, I updated that commit and created https://github.com/boostorg/core/pull/62 with it, you can comment there.

[glenfe]

@Lastique Couldn't BOOST_REQUIRE_ERRORS(N) reset the error count to 0 if the error count is equal to N, otherwise, set the error count to 1? That way, BOOST_REQUIRE_ERRORS(5); return boost::report_errors(); would result in returning 0 from main when we expected the error count at that point to be 5.

[Lastique]

It could, but then it would affect the final report on the console (it would say 0 or 1 errors detected instead of the actual number). This could be worked around too, but this is yet more hackery with the counter. I like my approach better because it is more straightforward and as a bonus flags all tests that are currently broken.

[Lastique]

@pdimov Do you still object to merging this PR (i.e. to revert report_errors to pre-#51 state)? We can proceed with our discussions about a better solution later.

If you still object, please tell why.

[pdimov]

Yes, I do object.

There's nothing wrong with the current state.

After 1.71 ships, I'll extract lightweight_test into its own module, and leave the Core version for you to screw up as you please. But it's late for that now.

[Lastique]

Feel free to update the 1.71 release notes so that the happy users at least know what they're in for. Or you can tell me what to add in https://github.com/boostorg/website/pull/433.

Personally, I think ignoring the obvious introduced bug is unacceptable.

[HDembinski]

Please do not make lightweight_tests into a separate module. I think it is great that we have minimal testing capabilities for Boost-internal use directly in Boost.Core. It is perfectly in line with the mission statement for Boost.Core - to provide a common foundation of tools for other Boost libraries - and it helps to keep the dependencies for other libs, such as Boost.Histogram small.

[pdimov]

As lwt started moving beyond just one simple BOOST_TEST macro and started attracting feature contributions, it would be better placed as a separate module anyway, with an improved documentation. I've wanted to do this for a while. Plus, testing shouldn't require pulling the whole Core, even though that's not really a practical problem at the moment.