Exception tests

[Balazs.Dan, 2008-07-26 02:51:56]

Hello guys,

Here is a new patch for exception tests.

New macros are:
EXPECT_THROW(expression, expectedexcetion)
ASSERT_THROW(expression, expectedexcetion)
EXPECT_NO_THROW(expression)
ASSERT_NO_THROW(expression)

Please comment it!

Regards,
Balazs

http://codereview.appspot.com/2663/diff/1/4
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/2663/diff/1/4#newcode521
Line 521: if (::testing::internal::GetCatchExceptionsFlag()) { \
I'm not sure is this flag counts here or not.. the purpose of this macro is
testing an exception so controlling it with this flag maybe unnecessary.

[Zhanyong, 2008-08-01 23:56:38]

Hi, Balazs,

I apologize for the long delay!  Finally I found some time.  Please see the
comments.  Thanks.

Also there are many comments, most of them are minor issues, so I expect this to
be checked in soon!  Cheers.

http://codereview.appspot.com/2663/diff/1/5
File include/gtest/gtest.h (right):

http://codereview.appspot.com/2663/diff/1/5#newcode936
Line 936: // Exception assertions.
Also explain what each of them does?  In particular, say that ... can be used as
an exception type.

http://codereview.appspot.com/2663/diff/1/5#newcode937
Line 937: #define EXPECT_THROW(expression, expectedexcetion) \
Rename the arguments:

  expression => statement
  expectedexcetion => expected_exception

http://codereview.appspot.com/2663/diff/1/4
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/2663/diff/1/4#newcode142
Line 142: bool GetCatchExceptionsFlag();
This can be removed.  I think the exception assertion macros shouldn't care
about this flag.

http://codereview.appspot.com/2663/diff/1/4#newcode518
Line 518: #define GTEST_TEST_THROW(expression, expectedexcetion, fail) \
Rename:

 expression => statement
 expectedexcetion => expected_exception

http://codereview.appspot.com/2663/diff/1/4#newcode521
Line 521: if (::testing::internal::GetCatchExceptionsFlag()) { \
On 2008/07/26 02:51:56, Balazs.Dan wrote:
> I'm not sure is this flag counts here or not.. the purpose of this macro is
> testing an exception so controlling it with this flag maybe unnecessary.

This can be removed.

http://codereview.appspot.com/2663/diff/1/4#newcode522
Line 522: bool caught = false; \
All local variables should start with gtest_.

http://codereview.appspot.com/2663/diff/1/4#newcode529
Line 529: catch (...) {} \
Use nested try to allow this macro to be used with ..., i.e. support

  GTEST_TEST_THROW(expression, ..., fail)

http://codereview.appspot.com/2663/diff/1/4#newcode531
Line 531: gtest_msg = "Expected exception of type " #expectedexcetion \
The message should tell the user whether the statement didn't throw anything, or
it threw something of unexpected type.  Also, the format should match that of
EXPECT_EQ, e.g.

Expected: Foo() throws an exception of type Blah.
  Actual: it throws nothing.

or

Expected: Foo() throws an exception of type Blah.
  Actual: it throws a different type.

http://codereview.appspot.com/2663/diff/1/4#newcode542
Line 542: #define GTEST_TEST_NO_THROW(expression, fail) \
Rename expression to statement.

http://codereview.appspot.com/2663/diff/1/4#newcode545
Line 545: if (::testing::internal::GetCatchExceptionsFlag()) { \
Remove this.

http://codereview.appspot.com/2663/diff/1/4#newcode546
Line 546: bool caught = false; \
gtest_...

http://codereview.appspot.com/2663/diff/1/4#newcode551
Line 551: gtest_msg = "Unexpected exception thrown by " #expression; \
Expected: Foo() doesn't throw an exception.
  Actual: it throws.

http://codereview.appspot.com/2663/diff/1/3
File include/gtest/internal/gtest-string.h (right):

http://codereview.appspot.com/2663/diff/1/3#newcode239
Line 239: operator bool() { return true; }
Why this?

http://codereview.appspot.com/2663/diff/1/6
File src/gtest.cc (right):

http://codereview.appspot.com/2663/diff/1/6#newcode209
Line 209: bool GetCatchExceptionsFlag() { return GTEST_FLAG(catch_exceptions); }
This can be removed.

http://codereview.appspot.com/2663/diff/1/2
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/2663/diff/1/2#newcode2290
Line 2290: throw 1;
This won't compile if exceptions are disabled in the compiler options.

Make sure the exception assertion tests are done only when exceptions are
enabled.

http://codereview.appspot.com/2663/diff/1/2#newcode2797
Line 2797: testing::GTEST_FLAG(catch_exceptions) = true;
Remove this.

http://codereview.appspot.com/2663/diff/1/2#newcode2799
Line 2799: EXPECT_THROW(ThrowAnInteger(), int);
Add a test where the expected exception type is ....

Add a test where the inner statement is not an expression.

Add syntax tests to make sure EXPECT_THROW behaves like a single statement.

Add tests to make sure EXPECT_THROW runs the statement exactly once.

http://codereview.appspot.com/2663/diff/1/2#newcode2800
Line 2800: EXPECT_NONFATAL_FAILURE(EXPECT_THROW(ThrowAnInteger(), bool),
Add test where the statement doesn't throw.

http://codereview.appspot.com/2663/diff/1/2#newcode2801
Line 2801: "Expected exception of type bool not thrown by ThrowAnInteger()");
80 columns.

http://codereview.appspot.com/2663/diff/1/2#newcode2805
Line 2805: TEST(AssertionTest, EXPECT_NO_THROW) {
Most of the comments for the above test applies here.

http://codereview.appspot.com/2663/diff/1/2#newcode2812
Line 2812: 
We also need the same tests for ASSERT_*.

http://codereview.appspot.com/2663/diff/1/2#newcode4327
Line 4327: EXPECT_NONFATAL_FAILURE(EXPECT_THROW(ThrowAnInteger(), bool) <<
"expected failure",
80 columns.

http://codereview.appspot.com/2663/diff/1/2#newcode4329
Line 4329: EXPECT_FATAL_FAILURE(ASSERT_THROW(ThrowAnInteger(), bool) <<
"expected failure",
The same.

http://codereview.appspot.com/2663/diff/1/2#newcode4338
Line 4338: EXPECT_NONFATAL_FAILURE(EXPECT_NO_THROW(ThrowAnInteger()) <<
"expected failure",
Ditto.

[Balazs.Dan, 2008-08-02 00:48:55]

http://codereview.appspot.com/2663/diff/1/3
File include/gtest/internal/gtest-string.h (right):

http://codereview.appspot.com/2663/diff/1/3#newcode239
Line 239: operator bool() { return true; }
Because this way I can create a local String variable in an if statement. This
is another trick to be able to stream a message with/into this macro..

[Balazs.Dan, 2008-09-06 00:38:28]

Patch improved after Zhanyong's comments.
I believe const& exceptions are the best way to handle exceptions in tests, but
I've created a new macro for ... tests. Hope this will satisfy everyone :)

[Zhanyong, 2008-09-10 19:12:47]

This looks FANTASTIC!  You've done a very thorough job.  Just some nits. 
Thanks!

http://codereview.appspot.com/2663/diff/41/43
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/2663/diff/41/43#newcode555
Line 555: gtest_msg = "Expected: " #statement " throws an exception of type " \
You can eliminate the gtest_caught_any variable by moving the code here inside
the catch(...) body.

http://codereview.appspot.com/2663/diff/41/43#newcode556
Line 556: #expected_exception ".\n  Actual: throws a different type."; \
For consistency with ASSERT_NO_THROW, add "it " before "throws".

http://codereview.appspot.com/2663/diff/41/43#newcode561
Line 561: #expected_exception ".\n  Actual: throws nothing."; \
The same.

http://codereview.appspot.com/2663/diff/41/42
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/2663/diff/41/42#newcode2459
Line 2459: 
Can we have a test to make sure that the implementation does
catch(expected_exception const&) instead of catch(const expected_exception&)?

E.g.

  ASSERT_THROW(Foo(), const char*);

http://codereview.appspot.com/2663/diff/41/42#newcode3003
Line 3003: // Tests EXPECT_THROW
Style nit: append a period (.) to the end of the sentence.

The same for the other comments.

http://codereview.appspot.com/2663/diff/41/42#newcode3004
Line 3004: TEST(AssertionTest, EXPECT_THROW) {
AssertiionTest => ExpectTest

http://codereview.appspot.com/2663/diff/41/42#newcode3015
Line 3015: TEST(AssertionTest, EXPECT_NO_THROW) {
The same.

http://codereview.appspot.com/2663/diff/41/42#newcode3023
Line 3023: TEST(AssertionTest, EXPECT_ANY_THROW) {
The same.

[Balazs.Dan, 2008-09-11 01:31:45]

Partially done, but I dont understand your test request..

http://codereview.appspot.com/2663/diff/41/42
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/2663/diff/41/42#newcode2459
Line 2459: 
I'm not sure how to make this test!? It will catch both.

Eg. ASSERT_THROW(Foo(), const char*) will catch these:
throw (const char*) "string";
throw (char* const) "string";
throw (const char* const) "string";

[Zhanyong, 2008-09-11 03:58:46]

http://codereview.appspot.com/2663/diff/442/642
File include/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/2663/diff/442/642#newcode774
Line 774: goto GTEST_CONCAT_TOKEN(gtest_label_testnothrow_, __LINE__); \
Change the label name to gtest_label_testanythrow_?