Exceptions
balazs.dan
As I see there is no support for exception tests.
I know you dont like exceptions as your style guide says, but we
mortal beings have to use it sometimes. :) Especially on windows where
hard to avoid it.. I think it would be useful for many people.
So is there any plan to add exception tester macros? Such as
EXPECT_THROW, ASSERT_THROW, EXPECT_NO_THROW and so on. Of course, I
can create a patch also. ;)
And I have a problem with exception handling in gtest!
If a tested function throws one and you expect a boolean value, then
gtest wont tell you where is the problem exactly. Here is an example:
bool ThrowAnException()
{
throw 0;
}
TEST(Test, Exception)
{
EXPECT_TRUE(ThrowAnException());
}
Result is:
[ RUN ] Test.Exception
unknown file: Failure
Exception thrown with code 0xe06d7363 in the test body.
[ FAILED ] Test.Exception
It could report the source if you surround the test code with a try
catch block. As I see the best place for this would be in GTEST_ASSERT
and GTEST_TEST_BOOLEAN macros. Because gtest must run with disabled
exceptions this try catch block should be controlled by a define.
What do you think?
Best regards,
Balazs
Zhanyong Wan (λx.x x)
On Fri, Jul 11, 2008 at 5:25 PM, balazs.dan <Balaz...@gmail.com> wrote:
>
> Hello Guys!
>
> As I see there is no support for exception tests.
> I know you dont like exceptions as your style guide says, but we
> mortal beings have to use it sometimes. :) Especially on windows where
> hard to avoid it.. I think it would be useful for many people.
Agreed.
> So is there any plan to add exception tester macros? Such as
> EXPECT_THROW, ASSERT_THROW, EXPECT_NO_THROW and so on. Of course, I
> can create a patch also. ;)
We don't have the time to do this now. It's great that you
volunteered! Before you actually write the code, would you please
describe the usage of the macros you want to add and give some
examples? Once we reach a consensus on the design, you can start to
work on the implementation and tests.
Also, would you please create an issue in the issue tracker for this?
> And I have a problem with exception handling in gtest!
> If a tested function throws one and you expect a boolean value, then
> gtest wont tell you where is the problem exactly. Here is an example:
> bool ThrowAnException()
> {
> throw 0;
> }
>
> TEST(Test, Exception)
> {
> EXPECT_TRUE(ThrowAnException());
> }
>
> Result is:
> [ RUN ] Test.Exception
> unknown file: Failure
> Exception thrown with code 0xe06d7363 in the test body.
> [ FAILED ] Test.Exception
>
> It could report the source if you surround the test code with a try
> catch block. As I see the best place for this would be in GTEST_ASSERT
> and GTEST_TEST_BOOLEAN macros. Because gtest must run with disabled
> exceptions this try catch block should be controlled by a define.
> What do you think?
Sounds good. I haven't had the time to think through this, but it may
be technically challenging or even impossible, as there are some other
constraints to be satisified:
- an assertion macro must behave like an atomic statement (e.g. you
can write code like "if (...) ASSERT_BLAH(...); else ...;").
- an assertion macro must be able to accept additional failure messages
via the << operator.
- an assertion macro must evaluate its arguments exactly once.
I'd encourage you to do some experiment to find out whether this is possible.
Thanks,
> Best regards,
> Balazs
>
--
Zhanyong
Keith Ray
EXPECT_THROWS(expression, expected_exception);
ASSERT_THROWS(expresssion, expected_exception);
and
EXPECT_NO_THROWS(expression);
ASSERT_NO_THROWS(expresssion);
--
C. Keith Ray, IXP Coach, Industrial Logic, Inc.
http://industriallogic.com 866-540-8336 (toll free)
Groove with our Agile Greatest Hits: http://www.industriallogic.com/elearning/
http://homepage.mac.com/keithray/blog/index.html
Zhanyong Wan (λx.x x)
> perhaps better macros names might be
> ASSERT_THROWS(expresssion, exception)
> ...
I don't have a strong opinion on THROW vs THROWS. I can see merits either way.
We should allow a statement (not just an expression) as the first argument.
The user should also be able to describe additional properties the
exception value is expected to have, maybe using a predicate or
matcher (as used in our mocking framework). I like matchers better,
which means we may want to delay this work until our C++ mocking
framework is open-sourced in September. This would lead to something
like:
EXPECT_THROW(statement, FooException* e, e->HasBarProperty()); //
e->HasBarProperty() must be true.
EXPECT_THROW(statement, ..., true); // Any exception will do.
Since this is C++, we also need to consider memory management if the
exception value is a pointer type. This means (arggh) the user of the
macro needs to be able to specify how to delete/free the exception
value. We could provide a simplified version that always use delete
to reclaim the memory, but a general mechansim should also be provided
(like ASSERT_DEATH vs ASSERT_EXIT), e.g.
EXPECT_THROW_EX(statement, FooException* e, e->HasBarProperty(), delete e);
All things considered, this may not be as clean/declarative as one
might have hoped. I would recommend to take the time to figure out a
good design instead of rushing to the implementation.
Thanks,
>> _______________________________________________
>> gunit-users mailing list
>> gunit...@google.com
>> https://mailman.corp.google.com/mailman/listinfo/gunit-users
>
>
>
> --
> Keith Ray - A member of your friendly, neighborhood Testability Corps.
>
> http://big.corp.google.com/~mstriebeck/testmercenaries/
>
--
Zhanyong
Keith Ray
>> ASSERT_NO_THROWS(expresssion);
>
> Sorry if I'm just confused. What's the benefit of ASSERT_NO_THROWS? If
> the expression throws, the test will fail even without the macro,
> right? I guess I can see the use of EXPECT_NO_THROWS to prevent the
> whole test from exiting when the expression throws.
While I expect a test that isn't explicitly documented to expect an
exception, to fail if an exception is thrown, sometimes I would want
to be more clear that something specifically doesn't throw an
exception by intention. ("test as spec")
I also note that boost has:
BOOST_WARN_NO_THROW
BOOST_CHECK_NO_THROW
BOOST_REQUIRE_NO_THROW
in its unit test support headers.
Zhanyong Wan (λx.x x)
> On Mon, Jul 14, 2008 at 8:02 PM, Zhanyong Wan (λx.x x) <w...@google.com>
> wrote:
>>
>> On Mon, Jul 14, 2008 at 10:29 AM, Keith Ray <keit...@google.com> wrote:
>> > perhaps better macros names might be
>> > EXPECT_THROWS(expression, exception),
>> > ASSERT_THROWS(expresssion, exception)
>> > ...
>>
>> I don't have a strong opinion on THROW vs THROWS. I can see merits either
>> way.
>>
>> We should allow a statement (not just an expression) as the first
>> argument.
>>
>> The user should also be able to describe additional properties the
>> exception value is expected to have, maybe using a predicate or
>> matcher (as used in our mocking framework). I like matchers better,
>> which means we may want to delay this work until our C++ mocking
>> framework is open-sourced in September. This would lead to something
>> like:
>>
>> EXPECT_THROW(statement, FooException* e, e->HasBarProperty()); //
>> e->HasBarProperty() must be true.
>> EXPECT_THROW(statement, ..., true); // Any exception will do.
>
> need for checking anything but the type of the exception. If I were using
> proposed EXPECT_THROW with 3 arguments I would create my own macro with 2
> arguments and use it instead.
>
> If one needs some complex checking of exception, including checks for
> properties and such, she can always write try/catch manually.
Good point. I was suggesting a 3-argument macro for the general case,
and agree that we should provide a 2-argument version for convenience.
Perhaps the general case that requires testing the property of the
exception is rare enough that we don't need to worry about it.
> try {
> DoSmthCrazy();
> FAIL();
> } catch (const std::exception& e) {
> EXPECT_EQ(e.what(), "crazy");
> }
Unrelated style nit: the two arguments to EXPECT_EQ should be
switched, by xUnit's convention. gtest's error messages are optimized
for that.
>> Since this is C++, we also need to consider memory management if the
>> exception value is a pointer type. This means (arggh) the user of the
>> macro needs to be able to specify how to delete/free the exception
>> value. We could provide a simplified version that always use delete
>> to reclaim the memory, but a general mechansim should also be provided
>> (like ASSERT_DEATH vs ASSERT_EXIT), e.g.
>>
>> EXPECT_THROW_EX(statement, FooException* e, e->HasBarProperty(), delete
>> e);
>
> You don't need to do that. In C++ exceptions are never thrown by pointer.
You can throw any copyable value, including a pointer. It is up to
the individual project to decide whether this is a good practice, but
C++ does not discriminate against that. In fact, this style is used
widely on Windows, e.g. in MFC.
>
> Roman.
--
Zhanyong
Zhanyong Wan (λx.x x)
> On Tue, Jul 15, 2008 at 7:48 PM, Zhanyong Wan (λx.x x) <w...@google.com>
>
>>
>> >> Since this is C++, we also need to consider memory management if the
>> >> exception value is a pointer type. This means (arggh) the user of the
>> >> macro needs to be able to specify how to delete/free the exception
>> >> value. We could provide a simplified version that always use delete
>> >> to reclaim the memory, but a general mechansim should also be provided
>> >> (like ASSERT_DEATH vs ASSERT_EXIT), e.g.
>> >>
>> >> EXPECT_THROW_EX(statement, FooException* e, e->HasBarProperty(),
>> >> delete
>> >> e);
>> >
>> > You don't need to do that. In C++ exceptions are never thrown by
>> > pointer.
>>
>> You can throw any copyable value, including a pointer. It is up to
>> the individual project to decide whether this is a good practice, but
>> C++ does not discriminate against that. In fact, this style is used
>> widely on Windows, e.g. in MFC.
>
> that does it that I'm aware of, and it was implemented even before
> exceptions were added to C++. Microsoft's exception handling support in MFC
> used dynamic type of an exception to figure out which 'catch' block should
> be used. That's why they decided to throw pointer in the first place. After
> standardization of exceptions Microsoft's libraries throw exceptions by
> value (e.g. ATL).
OK, so my experience is not typical. :)
I agree that our macros don't need to worry about exceptions thrown as
pointers. The user can always write his own try-catch if necessary.
> I think that the most common and most convenient EXPECT_THROW macro should
> take two arguments. You can always add _EX version with more arguments when
> you get a user request for it (or even better -- a patch). I would wait for
> someone to ask for such feature before adding it, because users might have
> some requirements that we are not aware of, therefore "preemptive
> implementation" may result in inconvenient interface for them.
>
> Note that this feature is absent from CppUnit and Boost.Test. IMHO, it
> indicates its usefulness from the user's perspective.
Agreed. Two-argument it is then.
Now, whoever wants to implement it, please make sure that it behaves
like a single statement and that we can stream additional messages to
it. See the implementation of death tests for an example.
Cheers,
> Roman.
--
Zhanyong
balazs.dan
Sorry for the late answer.
I've finished my experiments and I was able to modify GTEST_ASSERT and
GTEST_TEST_BOOLEAN macros and still satisfy all 3 constraints while
reports more information about the source of the error!
Before my modification my test was reported this way:
TEST(Test, Exception)
{
EXPECT_TRUE(ThrowAnException()) << "additional message";
[ RUN ] Test.Exception
unknown file: Failure
Exception thrown with code 0xe06d7363 in the test body.
[ FAILED ] Test.Exception
[ RUN ] Test.Exception
test.cc:113: Failure
Unhandled exception caught in: ThrowAnException()
[ FAILED ] Test.Exception
Is this format acceptable for you?
If yes, then I have to make some test test and i can submit the
patch..
And I've created exception testing macros also!
TEST(Test, Exception2)
{
EXPECT_THROW(ThrowAnException(), long) << "additional message";
}
will report:
[ RUN ] Test.Exception2
test.cc:123: Failure
Expected exception: "long" not thrown from: ThrowAnException()
[ FAILED ] Test.Exception2
Again, we should agree on a format..
Best regards,
Balazs
>
> - Show quoted text -
balazs.dan
Correctly:
[ RUN ] Test.Exception2
test.cc:123: Failure
Expected exception: "long" not thrown from: ThrowAnException()
[ FAILED ] Test.Exception2
Regards.
Zhanyong Wan (λx.x x)
Thanks for the hard work!
2008/7/16 balazs.dan <Balaz...@gmail.com>:
>
> Sorry, a copy-paste error.. :)
>
> Correctly:
> will report:
> [ RUN ] Test.Exception2
> test.cc:123: Failure
> Expected exception: "long" not thrown from: ThrowAnException()
> additional message
> [ FAILED ] Test.Exception2
>
> Regards.
>
>
> On Jul 16, 9:51 pm, "balazs.dan" <Balazs....@gmail.com> wrote:
>> Hello Zhanyong,
>>
>> Sorry for the late answer.
>> I've finished my experiments and I was able to modify GTEST_ASSERT and
>> GTEST_TEST_BOOLEAN macros and still satisfy all 3 constraints while
>> reports more information about the source of the error!
COOOOOL! Looking forward to the patch.
>> Before my modification my test was reported this way:
>> TEST(Test, Exception)
>> {
>> EXPECT_TRUE(ThrowAnException()) << "additional message";
>>
>> }
>>
>> [ RUN ] Test.Exception
>> unknown file: Failure
>> Exception thrown with code 0xe06d7363 in the test body.
>> [ FAILED ] Test.Exception
>>
>> Now it says:
>> [ RUN ] Test.Exception
>> test.cc:113: Failure
>> Unhandled exception caught in: ThrowAnException()
>> [ FAILED ] Test.Exception
>>
>> Is this format acceptable for you?
Is it possible to include the exception code in the message as before?
"Unhandled exception caught in: statement" reads like that the
statement *caught* the exception. Perhaps "Unhandled exception thrown
by: statement"?
Did you forget the additional message?
Otherwise it looks great!
>> If yes, then I have to make some test test and i can submit the
>> patch..
>>
>> And I've created exception testing macros also!
You're on a roll! :)
>> TEST(Test, Exception2)
>> {
>> EXPECT_THROW(ThrowAnException(), long) << "additional message";
>>
>> }
>>
>> will report:
>> [ RUN ] Test.Exception2
>> test.cc:123: Failure
>> Expected exception: "long" not thrown from: ThrowAnException()
>> [ FAILED ] Test.Exception2
Does this work when the second argument is ...?
The double quotes around "long" make it look like a string. Someone
may think that the message means a string with the value "long" is
expected. Perhaps
Expected exception of type long: not thrown by ThrowAnException()
?
Best,
--
Zhanyong
balazs.dan
> "Unhandled exception caught in: statement" reads like that the
> statement *caught* the exception. Perhaps "Unhandled exception thrown
> by: statement"?
> Did you forget the additional message?
way to stream it.. yet.
However, the original implementation also not displayed it! But
modified macros provide more information.
So the only difference in the result is: "unknown file: Failure" vs
"test.cc:113: Failure".
> Otherwise it looks great!
Thx.
> Does this work when the second argument is ...?
Balazs.
Zhanyong Wan (λx.x x)
>
> No, I didn't forget the additional message, but I have not found any
> way to stream it.. yet.
> However, the original implementation also not displayed it! But
> modified macros provide more information.
> So the only difference in the result is: "unknown file: Failure" vs
> "test.cc:113: Failure".
I was just wondering. It will be great to have the additional message
in the output. If that cannot be done, well it's still an improvement.
>> Does this work when the second argument is ...?
>
> No. But why would be that good? :)
To assert that the code will throw something but you don't know or
don't care about its type?
I wonder why it doesn't work. If you expand EXPECT_THROW(statement,
ex) to something like:
...
try { statement; }
catch(ex) {
...
}
...
it shouldn't matter whether ex is ... or a type.
--
Zhanyong
balazs.dan
1. The implementation contains another ... to catch all other
exceptions. And not allowed to use 2 ... catch at once.
2. I'm using "catch (exceptiontype const&)".
Balazs.
On Jul 17, 12:54 am, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
balazs.dan
> > No, I didn't forget the additional message, but I have not found any
> > way to stream it.. yet.
> > However, the original implementation also not displayed it! But
> > modified macros provide more information.
> > So the only difference in the result is: "unknown file: Failure" vs
> > "test.cc:113: Failure".
>
> I was just wondering. It will be great to have the additional message
> in the output. If that cannot be done, well it's still an improvement.
I'm not sure about side effects, because it is a bit tricky (as the
whole gtest :).
But, I have a bigger problem with this new exception handling!
There is a flag: gtest_catch_exceptions. It is useless after this
modification, because the modified macro will catch all exception
apart from this flag! I've tried to check this flag in TEST macros,
but it is not possible because you declared these flags in gtest-
internal-inl.h and the comment says:
"We don't want the users to modify these flags in the code, but want
Google Test's own unit tests to be able to access them. Therefore we
declare them here as opposed to in gtest.h."
Well, I dont wanna modify it..
Is there any other way to check this flag than
GTEST_FLAG(catch_exceptions)?
This question affects all my modifications and I need some help now..
Thx,
Balazs
Zhanyong Wan (λx.x x)
>
> Well, two reasons:
>
> 1. The implementation contains another ... to catch all other
> exceptions. And not allowed to use 2 ... catch at once.
You can nest two try-catch blocks:
try {
try { statement; } catch(ex) { ... }
} catch(...) { ... }
> 2. I'm using "catch (exceptiontype const&)".
I would think most exception objects are copyable, so
catch(exception_type) would be fine. In the rare case the exception
is not copyable, the user can always write
EXPECT_THROW(statement, const exception_type&);
Thanks,
--
Zhanyong
Zhanyong Wan (λx.x x)
>
>
>> > No, I didn't forget the additional message, but I have not found any
>> > way to stream it.. yet.
>> > However, the original implementation also not displayed it! But
>> > modified macros provide more information.
>> > So the only difference in the result is: "unknown file: Failure" vs
>> > "test.cc:113: Failure".
>>
>> I was just wondering. It will be great to have the additional message
>> in the output. If that cannot be done, well it's still an improvement.
>
> Now, I've found a way to display the additional message!
> I'm not sure about side effects, because it is a bit tricky (as the
> whole gtest :).
>
> But, I have a bigger problem with this new exception handling!
> There is a flag: gtest_catch_exceptions. It is useless after this
> modification, because the modified macro will catch all exception
> apart from this flag!
It's still useful: when it's false (the default), the user will get a
chance to attach a debugger (at least on Windows) to the process when
an exception is thrown. When it's true, gtest will handle the
exception and won't give the user a chance to debug.
I think the assertion macros should catch the exception only when the
flag is true. Is it possible?
> I've tried to check this flag in TEST macros,
> but it is not possible because you declared these flags in gtest-
> internal-inl.h and the comment says:
> "We don't want the users to modify these flags in the code, but want
> Google Test's own unit tests to be able to access them. Therefore we
> declare them here as opposed to in gtest.h."
>
> Well, I dont wanna modify it..
>
> Is there any other way to check this flag than
> GTEST_FLAG(catch_exceptions)?
You can add a read-only accessor bool GetCatchExceptionsFlag() to
include/gtest/internal/gtest-internal.h. Thanks,
> This question affects all my modifications and I need some help now..
>
> Thx,
> Balazs
>
--
Zhanyong
balazs.dan
Unhandled exception catching implemented and submitted a patch with
some comments here:
http://code.google.com/p/googletest/issues/detail?id=16
Regards,
Balazs
On Jul 17, 5:31 am, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
Zhanyong Wan (λx.x x)
>
> Hello Zhanyong,
>
> Unhandled exception catching implemented and submitted a patch with
> some comments here:
> http://code.google.com/p/googletest/issues/detail?id=16
Most wonderful. Thank you!
We changed our dev guideline a bit. Now instead of attaching the
patch to the issue, we want to upload the patch to
http://codereview.appspot.com/ such that it can be easily reviewed. I
have done this for your other two patches - have you had the chance to
look at people's comments on them?
I can upload this patch again, but since it seems you'll be a frequent
contributor, it makes sense for you to learn to do it yourself. What
do you think? The instructions can be found on
http://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
--
Zhanyong
balazs.dan
> We changed our dev guideline a bit. Now instead of attaching the
> look at people's comments on them?
modifications?
> I can upload this patch again, but since it seems you'll be a frequent
> contributor, it makes sense for you to learn to do it yourself. What
> do you think? The instructions can be found onhttp://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
Thx,
Balazs
Zhanyong Wan (λx.x x)
>
>
>> We changed our dev guideline a bit. Now instead of attaching the
>> patch to the issue, we want to upload the patch tohttp://codereview.appspot.com/such that it can be easily reviewed. I
>> have done this for your other two patches - have you had the chance to
>> look at people's comments on them?
>
> Yes, I saw. Should I submit a new patch with those suggested
> modifications?
Yes, if you don't mind. If you prefer, I can work on the existing
issues myself and submit a new patch later, but it will take a while.
To submit a new patch, go to http://codereview.appspot.com/2582, sign
in with your gmail account, and click on "Add Another Patch Set".
>
>> I can upload this patch again, but since it seems you'll be a frequent
>> contributor, it makes sense for you to learn to do it yourself. What
>> do you think? The instructions can be found onhttp://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
>
> Please, upload this one now and next time I will try to do it myself.
I have uploaded it to http://codereview.appspot.com/2612. People,
please take a look.
>
> Thx,
> Balazs
>
--
Zhanyong
balazs.dan
I can see "Publish+Mail Comments", "Start Review" on left side. And
"Patch Set 1", "Download raw patch set", "Messages" and so on on right
side.
Do I have enough rights to upload another patch set to an existing
issue?
Balazs
On Jul 21, 9:02 pm, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
>
> >> We changed our dev guideline a bit. Now instead of attaching the
> >> look at people's comments on them?
>
> > Yes, I saw. Should I submit a new patch with those suggested
> > modifications?
>
> Yes, if you don't mind. If you prefer, I can work on the existing
> issues myself and submit a new patch later, but it will take a while.
>
>
>
>
> >> I can upload this patch again, but since it seems you'll be a frequent
> >> contributor, it makes sense for you to learn to do it yourself. What
> >> do you think? The instructions can be found onhttp://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
>
> > Please, upload this one now and next time I will try to do it myself.
>
Zhanyong Wan (λx.x x)
>
> Zhanyong, I can't see any "Add Another Patch Set" option there!?
> I can see "Publish+Mail Comments", "Start Review" on left side. And
> "Patch Set 1", "Download raw patch set", "Messages" and so on on right
> side.
>
> Do I have enough rights to upload another patch set to an existing
> issue?
I'm new to this code review tool too, so I'm not sure. Did you see a
link "sign in" or "log in"? Did you sign in with your gmail account?
Thanks,
>
>
> Balazs
>
> On Jul 21, 9:02 pm, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
>> On Fri, Jul 18, 2008 at 7:23 PM, balazs.dan <Balazs....@gmail.com> wrote:
>>
>> >> We changed our dev guideline a bit. Now instead of attaching the
>> >> patch to the issue, we want to upload the patch tohttp://codereview.appspot.com/suchthat it can be easily reviewed. I
>> >> have done this for your other two patches - have you had the chance to
>> >> look at people's comments on them?
>>
>> > Yes, I saw. Should I submit a new patch with those suggested
>> > modifications?
>>
>> Yes, if you don't mind. If you prefer, I can work on the existing
>> issues myself and submit a new patch later, but it will take a while.
>>
>> To submit a new patch, go tohttp://codereview.appspot.com/2582, sign
>> in with your gmail account, and click on "Add Another Patch Set".
>>
>>
>>
>> >> I can upload this patch again, but since it seems you'll be a frequent
>> >> contributor, it makes sense for you to learn to do it yourself. What
>> >> do you think? The instructions can be found onhttp://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
>>
>> > Please, upload this one now and next time I will try to do it myself.
>>
>> I have uploaded it tohttp://codereview.appspot.com/2612. People,
>> please take a look.
>>
>>
>>
>> > Thx,
>> > Balazs
>>
>> --
>> Zhanyong
> _______________________________________________
> opensource-gtest mailing list
> opensour...@google.com
> https://mailman.corp.google.com/mailman/listinfo/opensource-gtest
>
--
Zhanyong
balazs.dan
On Jul 22, 3:13 am, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
> 2008/7/21 balazs.dan <Balazs....@gmail.com>:
>
>
> > Zhanyong, I can't see any "Add Another Patch Set" option there!?
> > I can see "Publish+Mail Comments", "Start Review" on left side. And
> > "Patch Set 1", "Download raw patch set", "Messages" and so on on right
> > side.
>
> > Do I have enough rights to upload another patch set to an existing
> > issue?
>
> I'm new to this code review tool too, so I'm not sure. Did you see a
> link "sign in" or "log in"? Did you sign in with your gmail account?
> Thanks,
>
>
>
>
>
>
>
> > Balazs
>
> > On Jul 21, 9:02 pm, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
> >> On Fri, Jul 18, 2008 at 7:23 PM, balazs.dan <Balazs....@gmail.com> wrote:
>
> >> >> We changed our dev guideline a bit. Now instead of attaching the
> >> >> look at people's comments on them?
>
> >> > Yes, I saw. Should I submit a new patch with those suggested
> >> > modifications?
>
> >> Yes, if you don't mind. If you prefer, I can work on the existing
> >> issues myself and submit a new patch later, but it will take a while.
>
> >> To submit a new patch, go tohttp://codereview.appspot.com/2582, sign
> >> in with your gmail account, and click on "Add Another Patch Set".
>
> >> >> I can upload this patch again, but since it seems you'll be a frequent
> >> >> contributor, it makes sense for you to learn to do it yourself. What
> >> >> do you think? The instructions can be found onhttp://code.google.com/p/rietveld/wiki/CodeReviewHelp. Thanks,
>
> >> > Please, upload this one now and next time I will try to do it myself.
>
> >> I have uploaded it tohttp://codereview.appspot.com/2612. People,
> >> please take a look.
>
> >> > Thx,
> >> > Balazs
>
> >> --
> >> Zhanyong
> > _______________________________________________
> > opensource-gtest mailing list
> >https://mailman.corp.google.com/mailman/listinfo/opensource-gtest
Zhanyong Wan (λx.x x)
>
> Yes, I saw and logged in.
I'm not sure if only the original uploader can upload new versions of the patch.
Can you download upload.py and try
upload.py -i 1234
(where 1234 is the issue number) in your svn directory? Or you can
send the patch to me and I'll upload it.
Thanks,
--
Zhanyong
balazs.dan
I've tried to use the upload.py to upload a new patch set for issue
2582, but it says:
Issue creation errors:
{'user': ["You (Balazs.Dan) don't own this issue (2582)"]}
Probably you can give me rights to change my previous patch sets..
Thx,
Balazs
On Jul 22, 4:11 am, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
> 2008/7/21 balazs.dan <Balazs....@gmail.com>:
>
>
>
> > Yes, I saw and logged in.
>
> I'm not sure if only the original uploader can upload new versions of the patch.
>
> Can you download upload.py and try
>
> upload.py -i 1234
>
> (where 1234 is the issue number) in your svn directory? Or you can
> send the patch to me and I'll upload it.
>
> Thanks,
>
>
>
>
>
>
>
> > On Jul 22, 3:13 am, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
> >> 2008/7/21 balazs.dan <Balazs....@gmail.com>:
>
> >> > Zhanyong, I can't see any "Add Another Patch Set" option there!?
> >> > I can see "Publish+Mail Comments", "Start Review" on left side. And
> >> > "Patch Set 1", "Download raw patch set", "Messages" and so on on right
> >> > side.
>
> >> > Do I have enough rights to upload another patch set to an existing
> >> > issue?
>
> >> I'm new to this code review tool too, so I'm not sure. Did you see a
> >> link "sign in" or "log in"? Did you sign in with your gmail account?
> >> Thanks,
>
> >> > Balazs
>
> >> > On Jul 21, 9:02 pm, "Zhanyong Wan (λx.x x)" <w...@google.com> wrote:
> >> >> On Fri, Jul 18, 2008 at 7:23 PM, balazs.dan <Balazs....@gmail.com> wrote:
>
> >> >> >> We changed our dev guideline a bit. Now instead of attaching the
Zhanyong Wan (λx.x x)
>
> Hello,
>
> I've tried to use the upload.py to upload a new patch set for issue
> 2582, but it says:
> Issue creation errors:
> {'user': ["You (Balazs.Dan) don't own this issue (2582)"]}
>
> Probably you can give me rights to change my previous patch sets..
Sorry for the hassle. I didn't see any place where I can make you an
owner of the issue, so I added you as a reviewer. If you still cannot
upload, just send the patch to me and I'll upload it. Thank,
--
Zhanyong
balazs.dan
Now both requested feature submitted to the code review site:
http://codereview.appspot.com/2682
http://codereview.appspot.com/2663
Regards,
Balazs