Re: Exception tests

[shi...@gmail.com]

Dear Balazs.Dan,

New code review comments by Zhanyong have been published.
Please go to http://codereview.appspot.com/2663 to read them.

Message:
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.

Details:

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.

Sincerely,

Your friendly code review daemon (http://codereview.appspot.com/).

[Balaz...@gmail.com]

Dear Balazs.Dan,

New code review comments by Balazs.Dan have been published.

Please go to http://codereview.appspot.com/2663 to read them.

Details:

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..

[Zhanyong Wan (λx.x x)]

On Fri, Aug 1, 2008 at 5:48 PM, <Balaz...@gmail.com> wrote:
>
> Dear Balazs.Dan,
>
> New code review comments by Balazs.Dan have been published.
> Please go to http://codereview.appspot.com/2663 to read them.
>
> Details:
>
> 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..

Yeah, I figured that's what you did it for, but it doesn't seem to be
used. In the new macros you use const char* instead of String to hold
the message. Did I miss something?

>
> Sincerely,
>
> Your friendly code review daemon (http://codereview.appspot.com/).
>

--
Zhanyong

[Balázs Dán]

Hmm, you are right!

I've mixed this patch with my other one (for unhandled exceptions) which uses this trick.. well, i'm using both.

[Zhanyong Wan (λx.x x)]

Hi, Balázs,

Just want to touch base with you: are you still interested in
finishing this? Thanks!

2008/8/1 Balázs Dán <balaz...@gmail.com>:

--
Zhanyong

[Balázs Dán]

Hi Zhanyong,

 

Yes, I would like but I'm on holiday now and usually far from my computer.. :)

I will finish it before the end of next week.. hopefully.

 

Regards,

Balazs

 

[Zhanyong Wan (λx.x x)]

Hi, Balazs,

2008/8/23 Balázs Dán <balaz...@gmail.com>:

> Hi Zhanyong,
>
> Yes, I would like but I'm on holiday now and usually far from my computer..
> :)
> I will finish it before the end of next week.. hopefully.

There's no rush. I just wanted to know your plan. Enjoy your vacation!

--
Zhanyong