You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2010/06/28 18:47:56 UTC
Re: svn commit: r958630 - in /subversion/trunk/subversion:
libsvn_ra_local/split_url.c tests/libsvn_ra_local/ra-local-test.c
This is just compounding the problem.
The test suite should NEVER cause an assertion. You simply are not
allowed to pass those arguments into the functions. That is why the
assertion is there. Neither the test suite, nor third parties should
pass those bad arguments. Period.
If you want to convert the assertions into checked failures, and
return an error on those failures... fine. But that is different from
an assertion. The assertion means "you should NOT have done this."
That isn't a testable case. That is flatly a wrong programming construction.
-g
On Mon, Jun 28, 2010 at 12:40, <ju...@apache.org> wrote:
> Author: julianfoad
> Date: Mon Jun 28 16:40:52 2010
> New Revision: 958630
>
> URL: http://svn.apache.org/viewvc?rev=958630&view=rev
> Log:
> Remove some code that was just to help the test suite, now that the test
> suite can catch SVN_ERR_ASSERT failures (since r958583).
>
> * subversion/libsvn_ra_local/split_url.c
> (svn_ra_local__split_URL): Don't bother checking the URL before calling
> svn_uri_get_dirent_from_file_url().
>
> * subversion/tests/libsvn_ra_local/ra-local-test.c
> (split_url_syntax): Accept an assertion failure as an alternative to an
> "illegal URL" error when testing a malformed URL.
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_local/split_url.c
> subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_local/split_url.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/split_url.c?rev=958630&r1=958629&r2=958630&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_local/split_url.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_local/split_url.c Mon Jun 28 16:40:52 2010
> @@ -40,13 +40,6 @@ svn_ra_local__split_URL(svn_repos_t **re
> const char *repos_root_dirent;
> svn_stringbuf_t *urlbuf;
>
> - /* Verify that the URL is well-formed (loosely) */
> - /* First, check for the "file://" prefix, to avoid assertion in tests */
> - if (strncmp(URL, "file://", 7) != 0)
> - return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> - _("Local URL '%s' does not contain 'file://' "
> - "prefix"), URL);
> -
> SVN_ERR(svn_uri_get_dirent_from_file_url(&repos_dirent, URL, pool));
>
> /* Search for a repository in the full path. */
>
> Modified: subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c?rev=958630&r1=958629&r2=958630&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c Mon Jun 28 16:40:52 2010
> @@ -148,7 +148,7 @@ split_url_syntax(apr_pool_t *pool)
>
> /* Use only single slash after scheme */
> apr_err = try_split_url("file:/path/to/repos", pool);
> - if (apr_err != SVN_ERR_RA_ILLEGAL_URL)
> + if (apr_err != SVN_ERR_RA_ILLEGAL_URL && apr_err != SVN_ERR_ASSERTION_FAIL)
> return svn_error_create
> (SVN_ERR_TEST_FAILED, NULL,
> "svn_ra_local__split_URL failed to catch bad URL (slashes)");
>
>
>
Re: svn commit: r958630 - in /subversion/trunk/subversion:
libsvn_ra_local/split_url.c tests/libsvn_ra_local/ra-local-test.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Jul 1, 2010 at 09:47, Julian Foad <ju...@wandisco.com> wrote:
>...
> That is fine for now but, if we think about possible futures, this is
> not necessarily the way it has to be.
>
> When an API definition says (or implies) that a valid URL is required,
> this means (or implies) that an invalid URL argument leads to undefined
> behaviour. That is, if an application passes such an argument to the
> API, it cannot expect a good result for any definition of "good", nor
> can it expect that the library will throw an assertion failure, or
> crash, or rely on any kinds of behaviour whatsoever. We already know
> this.
>
> We define our own development and testing strategy. We *could* decide
> that our APIs will catch certain groups of invalid inputs in a certain
> way (at least in debug builds), and we *could* decide that we will have
> tests that ensure we have remembered to catch those invalid inputs in
> that way. That would be a valid engineering practice, and if we did
> then the test suite by definition *would* be allowed to pass "invalid"
> arguments. (The non-URL argument would be classed as "invalid" with
> respect to the API promises that apply to regular API users, but as a
> "valid" test case within the confines of our chosen testing practices.)
>
> However, we haven't decided to do our development that way. Therefore I
> removed the part of the test where it expected the undefined behaviour
> to be a particular behaviour.
Agreed on all parts!
Yes, one day we could define the expectation, then test for that. But
that will be another huge batch of work :-P
Cheers,
-g
Re: svn commit: r958630 - in /subversion/trunk/subversion:
libsvn_ra_local/split_url.c tests/libsvn_ra_local/ra-local-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-06-28, Greg Stein wrote:
> This is just compounding the problem.
OK, yes, this was wrong. In r959648 I removed this part of this test
(the part where it passes an invalid URL "file:/path/to/repos"). The
test no longer expects an assertion failure.
> The test suite should NEVER cause an assertion. You simply are not
> allowed to pass those arguments into the functions. That is why the
> assertion is there. Neither the test suite, nor third parties should
> pass those bad arguments. Period.
That is fine for now but, if we think about possible futures, this is
not necessarily the way it has to be.
When an API definition says (or implies) that a valid URL is required,
this means (or implies) that an invalid URL argument leads to undefined
behaviour. That is, if an application passes such an argument to the
API, it cannot expect a good result for any definition of "good", nor
can it expect that the library will throw an assertion failure, or
crash, or rely on any kinds of behaviour whatsoever. We already know
this.
We define our own development and testing strategy. We *could* decide
that our APIs will catch certain groups of invalid inputs in a certain
way (at least in debug builds), and we *could* decide that we will have
tests that ensure we have remembered to catch those invalid inputs in
that way. That would be a valid engineering practice, and if we did
then the test suite by definition *would* be allowed to pass "invalid"
arguments. (The non-URL argument would be classed as "invalid" with
respect to the API promises that apply to regular API users, but as a
"valid" test case within the confines of our chosen testing practices.)
However, we haven't decided to do our development that way. Therefore I
removed the part of the test where it expected the undefined behaviour
to be a particular behaviour.
- Julian
> If you want to convert the assertions into checked failures, and
> return an error on those failures... fine. But that is different from
> an assertion. The assertion means "you should NOT have done this."
>
> That isn't a testable case. That is flatly a wrong programming construction.
>
> -g
>
> On Mon, Jun 28, 2010 at 12:40, <ju...@apache.org> wrote:
> > Author: julianfoad
> > Date: Mon Jun 28 16:40:52 2010
> > New Revision: 958630
> >
> > URL: http://svn.apache.org/viewvc?rev=958630&view=rev
> > Log:
> > Remove some code that was just to help the test suite, now that the test
> > suite can catch SVN_ERR_ASSERT failures (since r958583).
> >
> > * subversion/libsvn_ra_local/split_url.c
> > (svn_ra_local__split_URL): Don't bother checking the URL before calling
> > svn_uri_get_dirent_from_file_url().
> >
> > * subversion/tests/libsvn_ra_local/ra-local-test.c
> > (split_url_syntax): Accept an assertion failure as an alternative to an
> > "illegal URL" error when testing a malformed URL.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_ra_local/split_url.c
> > subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c
> >
> > Modified: subversion/trunk/subversion/libsvn_ra_local/split_url.c
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/split_url.c?rev=958630&r1=958629&r2=958630&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_local/split_url.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_local/split_url.c Mon Jun 28 16:40:52 2010
> > @@ -40,13 +40,6 @@ svn_ra_local__split_URL(svn_repos_t **re
> > const char *repos_root_dirent;
> > svn_stringbuf_t *urlbuf;
> >
> > - /* Verify that the URL is well-formed (loosely) */
> > - /* First, check for the "file://" prefix, to avoid assertion in tests */
> > - if (strncmp(URL, "file://", 7) != 0)
> > - return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> > - _("Local URL '%s' does not contain 'file://' "
> > - "prefix"), URL);
> > -
> > SVN_ERR(svn_uri_get_dirent_from_file_url(&repos_dirent, URL, pool));
> >
> > /* Search for a repository in the full path. */
> >
> > Modified: subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c
> > URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c?rev=958630&r1=958629&r2=958630&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c (original)
> > +++ subversion/trunk/subversion/tests/libsvn_ra_local/ra-local-test.c Mon Jun 28 16:40:52 2010
> > @@ -148,7 +148,7 @@ split_url_syntax(apr_pool_t *pool)
> >
> > /* Use only single slash after scheme */
> > apr_err = try_split_url("file:/path/to/repos", pool);
> > - if (apr_err != SVN_ERR_RA_ILLEGAL_URL)
> > + if (apr_err != SVN_ERR_RA_ILLEGAL_URL && apr_err != SVN_ERR_ASSERTION_FAIL)
> > return svn_error_create
> > (SVN_ERR_TEST_FAILED, NULL,
> > "svn_ra_local__split_URL failed to catch bad URL (slashes)");
> >
> >
> >