testCacheManager: use cppunit exception tests

[kinkie]

Do not hand-roll tests for exception-throwing code

If possible, please apply similar changes to TestRandomUuid::testInvalidIds() and check main() in test_http_range.cc

[kinkie]

If possible, please apply similar changes to TestRandomUuid::testInvalidIds() and check main() in test_http_range.cc

Noted and added to backlog

[kinkie]

If possible, please apply similar changes to TestRandomUuid::testInvalidIds() and check main() in test_http_range.cc

Noted and added to backlog

testRandomUuid done in PR #1814 , test_http_range is not applicable (it's simply reporting on unexpected exceptions, rather than enforcing that an exception is thrown)

[rousskov]

test_http_range is not applicable (it's simply reporting on unexpected exceptions ...)

I disagree that test_http_range is not applicable. Its main() is not just reporting (that it is dying). It is exiting with an error code on any thrown exception:

    {
        ...
    } catch (const std::exception &e) {
        printf("Error: dying from an unhandled exception: %s\n", e.what());
        return EXIT_FAILURE;
    } catch (...) {
        printf("Error: dying from an unhandled exception.\n");
        return EXIT_FAILURE;
    }

Would not EXIT_FAILURE fail "make check"?

... rather than enforcing that an exception is thrown

AFAICT, test_http_range is enforcing (via EXIT_FAILURE) that an exception is not thrown. Just like CPPUNIT_ASSERT_NO_THROW() code added in this PR. Am I missing something?

[kinkie]

test_http_range is not applicable (it's simply reporting on unexpected exceptions ...)

I disagree that test_http_range is not applicable. Its main() is not just reporting (that it is dying). It is exiting with an error code on any thrown exception:

    {
        ...
    } catch (const std::exception &e) {
        printf("Error: dying from an unhandled exception: %s\n", e.what());
        return EXIT_FAILURE;
    } catch (...) {
        printf("Error: dying from an unhandled exception.\n");
        return EXIT_FAILURE;
    }

Would not EXIT_FAILURE fail "make check"?

... rather than enforcing that an exception is thrown

AFAICT, test_http_range is enforcing (via EXIT_FAILURE) that an exception is not thrown. Just like CPPUNIT_ASSERT_NO_THROW() code added in this PR. Am I missing something?

Of course EXIT_FAILURE makes "make check" fail. So does not catching an exception.
The difference with other test cases is that here we are not validating that some code
raises an exception we would expect it to do; here we are saying "if an exception is thrown, fail the test".
Duh, every test does that implicitly.

Now, I didn't write this code, but I can see it was added in commit a0b3b22 with this message: "Range: header testing may thor exceptions which were not caught by the test binary. Could lead to difficulty debugging exception errors."

Which matches my interpretation of the code: "if the exception code follows the standard, print the message, it might be helpful".

In other words, I think that the best thing we can do here is to just remove the exception block and reduce this test to the default case: unhandled exceptions terminate() and signal failure to the test harness

[rousskov]

The last response appears to be based on testInvalidUrl() changes. They are a red herring in this discussion because test_http_range code is not similar to testInvalidUrl(). test_http_range is similar to testValidUrl(). Hence, the last response does not help me understand why test_http_range case was declared inapplicable when this PR handles the same "make sure the exception is not thrown (and report it if it is)" case inside PR's CacheManagerInternals::testValidUrl().

I think that the best thing we can do here is to just remove the exception block and reduce this test to the default case: unhandled exceptions terminate() and signal failure to the test harness

AFAICT, that proposed approach goes against the inapplicable claim, CacheManagerInternals::testValidUrl() changes in this PR, and commit a0b3b22 concerns. Please do not interpret this paragraph as implying that the proposed approach is necessarily wrong.

Let's continue this discussion in new #1816 that is dedicated to test_http_range changes.

[squidadm]

queued for backport to v6