You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/12/27 19:49:51 UTC
Re: svn commit: r1053140 -
/subversion/trunk/subversion/tests/libsvn_subr/error-test.c
blair@apache.org wrote on Mon, Dec 27, 2010 at 19:04:39 -0000:
> Author: blair
> Date: Mon Dec 27 19:04:39 2010
> New Revision: 1053140
>
> URL: http://svn.apache.org/viewvc?rev=1053140&view=rev
> Log:
> Add a unit test for svn_error_purge_tracing() that asserts that
> SVN_ERR_ASSERT() fails when an error chain with only tracing errors is
> passed in, which shouldn't happen in practice.
>
> * subversion/tests/libsvn_subr/error-test.c
> (test_error_purge_tracing):
> Only if SVN_ERR__TRACING is defined, build an error chain with
> only tracing errors and pass it to svn_error_purge_tracing(),
> asserting that it returns a new error chain with a
> SVN_ERR_ASSERTION_FAIL error.
>
> Modified:
> subversion/trunk/subversion/tests/libsvn_subr/error-test.c
>
> Modified: subversion/trunk/subversion/tests/libsvn_subr/error-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/error-test.c?rev=1053140&r1=1053139&r2=1053140&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_subr/error-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_subr/error-test.c Mon Dec 27 19:04:39 2010
> @@ -111,8 +111,54 @@ test_error_purge_tracing(apr_pool_t *poo
> "Tracing link found after purging the "
> "following chain:");
> }
> -
> svn_error_clear(err);
> +
> +#ifdef SVN_ERR__TRACING
> + /* Make an error chain containing only tracing errors and check that
> + svn_error_purge_tracing() asserts on it. */
> + {
> + svn_error_t err_copy, err2_copy;
> + svn_error_malfunction_handler_t orig_handler;
> +
> + /* For this test, use a random error status. */
> + err = svn_error_create(SVN_ERR_BAD_UUID, NULL, SVN_ERR__TRACED);
> + err = svn_error_return(err);
> +
> + /* Register a malfunction handler that doesn't call abort() to
> + check that a new error chain with a SVN_ERR_ASSERTION_FAIL is
> + returned. */
> + orig_handler =
> + svn_error_set_malfunction_handler(svn_error_raise_on_malfunction);
> + err2 = svn_error_purge_tracing(err);
> + svn_error_set_malfunction_handler(orig_handler);
> +
> + err_copy = *err;
> +
> + if (err2)
> + {
> + /* If err2 does share the same pool as err, then make a copy
> + of err2 before err is cleared. */
> + err2_copy = *err2;
> +
> + svn_error_clear(err);
> +
> + /* The returned error is only safe to clear if this assertion
> + holds, otherwise it has the same pool as the original
> + error. */
> + SVN_ERR_ASSERT(err_copy.pool != err2_copy.pool);
> +
> + svn_error_clear(err2);
> +
> + SVN_ERR_ASSERT(SVN_ERR_ASSERTION_FAIL == err2_copy.apr_err);
> + }
> + else
> + {
> + svn_error_clear(err);
> + SVN_ERR_ASSERT(err2);
The last assert will trigger whenever the else{} block is reached, is
that your intention?
Also: are you aware of SVN_TEST_ASSERT()?
Also: formally, if you just got an SVN_ERR_ASSERTION_FAIL, the API
contract doesn't allow you to continue calling into svn libraries. In
other words, unless you verify that the "expected" assertion was
triggered, some day we might change the code, the test will hit some
other assertion --- and will happily ignore it :(
I'm not sure how exactly to represent the assertion in terms of
svn_error_t chains such that the test would be able to verify that
the 'right' assertion was triggered.
> + }
> + }
> +#endif
> +
> return SVN_NO_ERROR;
> }
>
>
>
Re: svn commit: r1053140 - /subversion/trunk/subversion/tests/libsvn_subr/error-test.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/27/10 1:20 PM, Daniel Shahaf wrote:
> Blair Zajac wrote on Mon, Dec 27, 2010 at 12:47:23 -0800:
>> On 12/27/10 11:49 AM, Daniel Shahaf wrote:
>>> Also: formally, if you just got an SVN_ERR_ASSERTION_FAIL, the API
>>> contract doesn't allow you to continue calling into svn libraries. In
>>> other words, unless you verify that the "expected" assertion was
>>> triggered, some day we might change the code, the test will hit some
>>> other assertion --- and will happily ignore it :(
>>
>> Where is this documented? That doesn't make sense in a multi-threaded
>> context as one would have to stop all threads.
>>
>
> I'm not sure where it's documented.
>
> The closeest I find in the API docs is that svn_error_malfunction_handler_t's
> docs allow the handler to choose between never returning and returning
> SVN_ERR_ASSERTION_FAIL.
>
> Perhaps someone else has a better API/dev@ reference...
>
>> There's always a chance we may catch another error if we change the code,
>> don't know if there's much to do about that.
>>
>
> Yeah, but my point was: what if after changing the code the test
> *unintentionally* hits an assertion? i.e., not the "No non-tracing
> links" assertion, but some other assertion.
We could assert that the file line number matches the expected line. I'm doing
this now.
Found a bug in svn_error_raise_on_malfunction() when doing this.
Blair
Re: svn commit: r1053140 -
/subversion/trunk/subversion/tests/libsvn_subr/error-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Mon, Dec 27, 2010 at 12:47:23 -0800:
> On 12/27/10 11:49 AM, Daniel Shahaf wrote:
>> Also: formally, if you just got an SVN_ERR_ASSERTION_FAIL, the API
>> contract doesn't allow you to continue calling into svn libraries. In
>> other words, unless you verify that the "expected" assertion was
>> triggered, some day we might change the code, the test will hit some
>> other assertion --- and will happily ignore it :(
>
> Where is this documented? That doesn't make sense in a multi-threaded
> context as one would have to stop all threads.
>
I'm not sure where it's documented.
The closeest I find in the API docs is that svn_error_malfunction_handler_t's
docs allow the handler to choose between never returning and returning
SVN_ERR_ASSERTION_FAIL.
Perhaps someone else has a better API/dev@ reference...
> There's always a chance we may catch another error if we change the code,
> don't know if there's much to do about that.
>
Yeah, but my point was: what if after changing the code the test
*unintentionally* hits an assertion? i.e., not the "No non-tracing
links" assertion, but some other assertion.
> Blair
Re: svn commit: r1053140 - /subversion/trunk/subversion/tests/libsvn_subr/error-test.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/27/10 11:49 AM, Daniel Shahaf wrote:
> blair@apache.org wrote on Mon, Dec 27, 2010 at 19:04:39 -0000:
>> Author: blair
>> Date: Mon Dec 27 19:04:39 2010
>> New Revision: 1053140
>>
>> URL: http://svn.apache.org/viewvc?rev=1053140&view=rev
>> Log:
>> Add a unit test for svn_error_purge_tracing() that asserts that
>> SVN_ERR_ASSERT() fails when an error chain with only tracing errors is
>> passed in, which shouldn't happen in practice.
>>
>> * subversion/tests/libsvn_subr/error-test.c
>> (test_error_purge_tracing):
>> Only if SVN_ERR__TRACING is defined, build an error chain with
>> only tracing errors and pass it to svn_error_purge_tracing(),
>> asserting that it returns a new error chain with a
>> SVN_ERR_ASSERTION_FAIL error.
>>
>> Modified:
>> subversion/trunk/subversion/tests/libsvn_subr/error-test.c
>>
>> Modified: subversion/trunk/subversion/tests/libsvn_subr/error-test.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/error-test.c?rev=1053140&r1=1053139&r2=1053140&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/tests/libsvn_subr/error-test.c (original)
>> +++ subversion/trunk/subversion/tests/libsvn_subr/error-test.c Mon Dec 27 19:04:39 2010
>> @@ -111,8 +111,54 @@ test_error_purge_tracing(apr_pool_t *poo
>> "Tracing link found after purging the "
>> "following chain:");
>> }
>> -
>> svn_error_clear(err);
>> +
>> +#ifdef SVN_ERR__TRACING
>> + /* Make an error chain containing only tracing errors and check that
>> + svn_error_purge_tracing() asserts on it. */
>> + {
>> + svn_error_t err_copy, err2_copy;
>> + svn_error_malfunction_handler_t orig_handler;
>> +
>> + /* For this test, use a random error status. */
>> + err = svn_error_create(SVN_ERR_BAD_UUID, NULL, SVN_ERR__TRACED);
>> + err = svn_error_return(err);
>> +
>> + /* Register a malfunction handler that doesn't call abort() to
>> + check that a new error chain with a SVN_ERR_ASSERTION_FAIL is
>> + returned. */
>> + orig_handler =
>> + svn_error_set_malfunction_handler(svn_error_raise_on_malfunction);
>> + err2 = svn_error_purge_tracing(err);
>> + svn_error_set_malfunction_handler(orig_handler);
>> +
>> + err_copy = *err;
>> +
>> + if (err2)
>> + {
>> + /* If err2 does share the same pool as err, then make a copy
>> + of err2 before err is cleared. */
>> + err2_copy = *err2;
>> +
>> + svn_error_clear(err);
>> +
>> + /* The returned error is only safe to clear if this assertion
>> + holds, otherwise it has the same pool as the original
>> + error. */
>> + SVN_ERR_ASSERT(err_copy.pool != err2_copy.pool);
>> +
>> + svn_error_clear(err2);
>> +
>> + SVN_ERR_ASSERT(SVN_ERR_ASSERTION_FAIL == err2_copy.apr_err);
>> + }
>> + else
>> + {
>> + svn_error_clear(err);
>> + SVN_ERR_ASSERT(err2);
>
> The last assert will trigger whenever the else{} block is reached, is
> that your intention?
Yes, because err2 should never be equal to SVN_NO_ERROR. I'm just managing the
error chain here.
> Also: are you aware of SVN_TEST_ASSERT()?
Nope, I've updated the code to use SVN_TEST_ASSERT() in r1053174.
> Also: formally, if you just got an SVN_ERR_ASSERTION_FAIL, the API
> contract doesn't allow you to continue calling into svn libraries. In
> other words, unless you verify that the "expected" assertion was
> triggered, some day we might change the code, the test will hit some
> other assertion --- and will happily ignore it :(
Where is this documented? That doesn't make sense in a multi-threaded context
as one would have to stop all threads.
There's always a chance we may catch another error if we change the code, don't
know if there's much to do about that.
Blair