report_errors with expected failures

[HDembinski]

Allow test to pass when it has exactly N expected failures. This is useful for lightweight_test_test5.cpp to actually test for the correct number of errors, and maybe useful elsewhere.

[pdimov]

The return should be outside the if, and the documentation needs to be updated.

[HDembinski]

One of appveyor builds didn't run. Could you try to restart it?

[pdimov]

On reflection, I'm not entirely sure whether it won't be better to just return the number of errors from report_errors, as the test implies.

[HDembinski]

I think it is worse. This was the old code:

return report_errors() == 10

It only "worked" because report_errors() returned 1, in contradiction to documentation. But even if it did return 10, then this would just have the opposite of the desired effect. If report_errors() returns the number of errors, then one must write report_errors() != 10. That is quite awkward.

The mistake was already made once, which shows that this is not a good design.

[pdimov]

Yes, with == 10 it needs to be a run-fail test. Which incidentally we want, because we can see the output of the failed tests.

[glenfe]

report_errors() returning the number of errors seems better to me.

[Lastique]

Given that in most tests we have return boost::report_errors(); in the end of main(), I think this change is not correct. The returned value will get truncated to 8 bits, and in the shell the calling process may receive 0 result code while there are a multiple of 256 failed tests.

I think, there should be a separate API that allows to obtain the number of failures.

[Lastique]

I propose to revert this change for 1.71.

[HDembinski]

The returned value will get truncated to 8 bits, and in the shell the calling process may receive 0 result code while there are a multiple of 256 failed tests.

Could you please point me to the documentation which shows that the return type int is truncated to char?

My original proposal was to pass the expected number of errors as an argument to boost::report_errors, which would side-step this issue.

[pdimov]

and in the shell the calling process may receive 0 result code while there are a multiple of 256 failed tests.

I'm willing to take that risk.

[Lastique]

Could you please point me to the documentation which shows that the return type int is truncated to char?

Not char, rather uint8_t. For example, it follows from WEXITSTATUS.

You can test yourself:

int main()
{
    return -1;
}

$ ./return_code ; echo $?
255

I'm willing to take that risk.

Umm, I'd much rather not. We have tons of tests in Boost like that, and who knows how many outside.

[HDembinski]

@Lastique (why don't you use your real name?): You want to take a feature away instead of amending it, that is like demolishing a house because you don't like the color of the walls. You only revert things when they are wrong beyond repair or when it is really urgent to avoid harm. Neither is the case here. This feature solves an issue, namely that a test with several expected failures would pass even if at least one of the expected failures happened. It did not detect how many of the expected failures actually happened. In contrast to the highly contrived situation that you are describing, this is one that I actually encountered while developing Boost.Core.

[Lastique]

You want to take a feature away instead of amending it, that is like demolishing a house because you don't like the color of the walls.

No, I want to revert the commit that introduced a bug. I still want to implement what you want but do it the right way. The revert part is important and IMO should happen faster since we're on the brink of 1.71 and I don't want a broken version of lightweight_test.hpp to be released. Better release the old version of the header without the feature than the new one that breaks users' tests.