Jon Foster wrote on Mon, Sep 20, 2010 at 12:39:31 +0100:
> Hi,
>
> Daniel Shahaf wrote:
> > Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
> > > Hi,
> > >
> > > This patch changes the tests for the atomic-revprop feature, so that
> > > they test the error number rather than parsing the error text.
> > > This doesn't work with Serf or Neon yet - they're still TODO. This
> > > gives you a way to test Serf & Neon patches. (You might like to
> > > hold off on committing this until Serf and Neon are done).
> > >
> > > The patch is against the atomic-revprop branch, and requires
> > > Patch 1 to be applied first. It doesn't apply to trunk (I don't
> > > think the tests have been merged to trunk?)
> > >
> >
> > On trunk there are only the libsvn_fs and libsvn_repos parts. The
> > libsvn_ra parts, and the associated Python tests, are only on the
> > branch.
> >
> > > [[[
> > > Change atomic-revprop tests to look at the error number rather
> > > than parsing the error text.
> > >
> > > * subversion/tests/cmdline/atomic-ra-revprop-change.c:
> > > (main): When printing an error, check if it's
> SVN_ERR_BAD_OLD_VALUE
> > > and print a special message if it is.
> > >
> > > * subversion/tests/cmdline/prop_tests.py:
> > > (FAILS_WITH_BPV): Look for the special message.
> > >
> > > Patch by: Jon Foster <jon.foster_at_cabot.co.uk>
> > > ]]]
> > >
> >
> > So, this is trying to capture the current ra_dav situation, where the
> > err->message is correct but err->apr_err isn't?
>
> Correct.
>
Okay. Perhaps this could have been called out more explicitly in the
log message --- i.e., have it say not only *what* the patch does, but
also *why* it does that.
> > (and will make the test fail over ra_neon/ra_serf)
>
> Correct.
>
> > In other news, I've been thinking about moving the entire validation
> > logic to the C helper; that is, to have it get an extra argv argument
> > saying whether the revpropchange should pass or fail, and test by
> itself
> > that it passed/failed as expected; and the Python tests would always
> > expect it to exit(0). What do you think?
>
> Sounds like a good idea.
>
Okay. I started a patch in that way at a time; I've touched it up now
a tiny bit and I'm attaching it. Let me know what you think...
(I've tested it when applied on top of your patch 3/3.)
> > > Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
> > > ===================================================================
> > > --- subversion/tests/cmdline/atomic-ra-revprop-change.c
> (revision 998620)
> > > +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
> copy)
> > > @@ -226,6 +226,8 @@
> > > http_library, pool);
> > > if (err)
> > > {
> > > + if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
> > > + fprintf(stderr, "atomic-ra-revprop-change failed due to
> incorrect
> >
> > Or even more directly:
> >
> > - if (err)
> > + if (err && err->apr_err == SVN_ERR_BAD_OLD_VALUE)
> >
>
> Wouldn't that leak an error if the error isn't SVN_ERR_BAD_OLD_VALUE?
>
Yes, it would.
> Also, the current* design requires us to use svn_error_has_cause()
> because the error is wrapped inside an error with a different code.
Yup, I've noticed this in the verbose test output I've pasted
elsethread, but you beat me to replying with the correction :)
> (FWIW, I think the RA layer shouldn't do that - I think we should
> change svn_ra_change_rev_prop() so that the simple test of
> err->apr_err works. But that's not urgent, and could even be
> postponed to 1.8).
First of all, agreed that this is small change; I think it's more
important to be able to promise that the error situation will be
/recognizable/ then whether the recognition will be with or without
svn_error_has_cause().
For reference, this is the error chain currently (with your patch 3/3):
[[[
subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002)
subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002)
atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is
+non-existent
subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008)
atomic-ra-revprop-change: At least one property change failed; repository is unchanged
subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014)
subversion/libsvn_ra_neon/util.c:236: (apr_err=125014)
atomic-ra-revprop-change: Error setting property 'flower':
revprop 'flower' has unexpected value in filesystem
]]]
>
> Kind regards,
>
> Jon
>
> (* Unless I missed something and the design's been changed?)
Received on 2010-09-20 14:15:26 CEST