You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Chug Rolke <cr...@redhat.com> on 2013/07/23 20:19:52 UTC
Review Request 12875: C++ Broker ACL self tests using misguided try-except
patterns for signalling self.fail
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12875/
-----------------------------------------------------------
Review request for qpid.
Bugs: qpid-5010
https://issues.apache.org/jira/browse/qpid-5010
Repository: qpid
Description
-------
Acl.py self tests issue self.fail() calls if an expected exception is not thrown. A proposed fix for two patterns is shown in this review.
Essentially if the expected exception is thrown then pass. If some other exception is thrown then show it's details. If no exception is thrown then issue the self.fail() that was in the try block. Finally, where necessary, run the cleanup handling to recover from the exception.
Diffs
-----
trunk/qpid/cpp/src/tests/acl.py 1506161
Diff: https://reviews.apache.org/r/12875/diff/
Testing
-------
There are 222 self.fail() calls that need to be examined and two of them are changed in this review. The rest need scrutiny and when fixed may reveal failures in the code as in QPID-5011.
Thanks,
Chug Rolke
Re: Review Request 12875: C++ Broker ACL self tests using misguided
try-except patterns for signalling self.fail
Posted by Alan Conway <ac...@redhat.com>.
> On July 25, 2013, 1:54 p.m., Alan Conway wrote:
> > trunk/qpid/cpp/src/tests/acl.py, line 1254
> > <https://reviews.apache.org/r/12875/diff/2/?file=326059#file326059line1254>
> >
> > What's wrong with the original code? I've used this pattern myself, often. I don't see any benefit to visiting every use of fail() in the tests!!
Just looked at the JIRA: the example there IS wrong, because "catch Exception" will catch the exception thrown by fail(). But this code is correct:
except qpid.session.SessionException, e
will not catch the exception thrown by fail().
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12875/#review23841
-----------------------------------------------------------
On July 23, 2013, 6:19 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12875/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 6:19 p.m.)
>
>
> Review request for qpid.
>
>
> Bugs: qpid-5010
> https://issues.apache.org/jira/browse/qpid-5010
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Acl.py self tests issue self.fail() calls if an expected exception is not thrown. A proposed fix for two patterns is shown in this review.
>
> Essentially if the expected exception is thrown then pass. If some other exception is thrown then show it's details. If no exception is thrown then issue the self.fail() that was in the try block. Finally, where necessary, run the cleanup handling to recover from the exception.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/tests/acl.py 1506161
>
> Diff: https://reviews.apache.org/r/12875/diff/
>
>
> Testing
> -------
>
> There are 222 self.fail() calls that need to be examined and two of them are changed in this review. The rest need scrutiny and when fixed may reveal failures in the code as in QPID-5011.
>
>
> Thanks,
>
> Chug Rolke
>
>
Re: Review Request 12875: C++ Broker ACL self tests using misguided
try-except patterns for signalling self.fail
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12875/#review23841
-----------------------------------------------------------
trunk/qpid/cpp/src/tests/acl.py
<https://reviews.apache.org/r/12875/#comment47672>
What's wrong with the original code? I've used this pattern myself, often. I don't see any benefit to visiting every use of fail() in the tests!!
- Alan Conway
On July 23, 2013, 6:19 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12875/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 6:19 p.m.)
>
>
> Review request for qpid.
>
>
> Bugs: qpid-5010
> https://issues.apache.org/jira/browse/qpid-5010
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Acl.py self tests issue self.fail() calls if an expected exception is not thrown. A proposed fix for two patterns is shown in this review.
>
> Essentially if the expected exception is thrown then pass. If some other exception is thrown then show it's details. If no exception is thrown then issue the self.fail() that was in the try block. Finally, where necessary, run the cleanup handling to recover from the exception.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/tests/acl.py 1506161
>
> Diff: https://reviews.apache.org/r/12875/diff/
>
>
> Testing
> -------
>
> There are 222 self.fail() calls that need to be examined and two of them are changed in this review. The rest need scrutiny and when fixed may reveal failures in the code as in QPID-5011.
>
>
> Thanks,
>
> Chug Rolke
>
>