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.fos...@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?)
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 998887)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -152,7 +152,8 @@
expected_stderr,
expected_exit,
url, revision, propname,
- old_propval, propval):
+ old_propval, propval,
+ want_error):
"""Run atomic-ra-revprop-change helper and check its output and exit code.
Transforms OLD_PROPVAL and PROPVAL into a skel.
For HTTP, the default HTTP library is used."""
@@ -173,7 +174,8 @@
make_proplist_skel_part(KEY_NEW_PROPVAL, propval))
exit_code, out, err = main.run_atomic_ra_revprop_change(url, revision,
- propname, skel)
+ propname, skel,
+ want_error)
verify.verify_outputs("Unexpected output", out, err,
expected_stdout, expected_stderr)
verify.verify_exit_code(message, exit_code, expected_exit)
Index: subversion/tests/cmdline/svntest/main.py
===================================================================
--- subversion/tests/cmdline/svntest/main.py (revision 998887)
+++ subversion/tests/cmdline/svntest/main.py (working copy)
@@ -641,7 +641,7 @@
0, 0, None, '--subdirs', path)
return [line.strip() for line in stdout_lines if not line.startswith("DBG:")]
-def run_atomic_ra_revprop_change(url, revision, propname, skel):
+def run_atomic_ra_revprop_change(url, revision, propname, skel, want_error):
"""Run the atomic-ra-revprop-change helper, returning its exit code, stdout,
and stderr. For HTTP, default HTTP library is used."""
# use spawn_process rather than run_command to avoid copying all the data
@@ -652,7 +652,7 @@
# This passes HTTP_LIBRARY in addition to our params.
return run_command(atomic_ra_revprop_change_binary, True, False,
url, revision, propname, skel,
- options.http_library)
+ options.http_library, want_error and 1 or 0)
# Chmod recursively on a whole subtree
Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
===================================================================
--- subversion/tests/cmdline/atomic-ra-revprop-change.c (revision 998887)
+++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working copy)
@@ -41,13 +41,18 @@
#define KEY_NEW_PROPVAL "value"
#define USAGE_MSG \
- "Usage: %s URL REVISION PROPNAME VALUES_SKEL HTTP_LIBRARY\n" \
+ "Usage: %s URL REVISION PROPNAME VALUES_SKEL HTTP_LIBRARY WANT_ERROR\n" \
"\n" \
"VALUES_SKEL is a proplist skel containing pseudo-properties '%s' \n" \
"and '%s'. A pseudo-property missing from the skel is interpreted \n" \
- "as unset.\n"
+ "as unset.\n" \
+ "\n" \
+ "WANT_ERROR is 1 if the propchange is expected to fail due to the atomicity,"\
+ "and 0 if it is expected to succeed. If the expectation matches reality," \
+ "the exit code shall be zero.\n"
+
/* implements svn_auth_simple_prompt_func_t */
static svn_error_t *
aborting_simple_prompt_func(svn_auth_cred_simple_t **cred,
@@ -117,6 +122,8 @@
SVN_ERR(svn_config_create(&servers, pool));
svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL,
SVN_CONFIG_OPTION_HTTP_LIBRARY, http_library);
+ svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL,
+ SVN_CONFIG_OPTION_NEON_DEBUG_MASK, getenv("SVN_NEON_DEBUG_MASK") /* "131" */);
/* Populate CONFIG. */
config = apr_hash_make(pool);
@@ -134,12 +141,14 @@
const svn_string_t *propval,
const svn_string_t *old_value,
const char *http_library,
+ svn_boolean_t want_error,
apr_pool_t *pool)
{
svn_ra_callbacks2_t *callbacks;
svn_ra_session_t *sess;
apr_hash_t *config;
svn_boolean_t capable;
+ svn_error_t *err;
SVN_ERR(svn_ra_create_callbacks(&callbacks, pool));
SVN_ERR(construct_auth_baton(&callbacks->auth_baton, pool));
@@ -152,15 +161,34 @@
SVN_RA_CAPABILITY_ATOMIC_REVPROPS,
pool));
if (capable)
- SVN_ERR(svn_ra_change_rev_prop2(sess, revision, propname,
- &old_value, propval, pool));
+ {
+ err = svn_ra_change_rev_prop2(sess, revision, propname,
+ &old_value, propval, pool);
+
+ if (want_error && err
+ /* ### change the error code */
+ && svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
+ {
+ /* Expectation was matched. Get out. */
+ fputs("<<<revprop 'flower' has unexpected value>>>", stderr);
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
+ else if (! want_error && ! err)
+ /* Expectation was matched. Get out. */
+ return SVN_NO_ERROR;
+ else if (want_error && ! err)
+ return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
+ "An error was expected but not seen");
+ else
+ /* A real (non-SVN_ERR_BAD_PROPERY_VALUE) error. */
+ return svn_error_return(err);
+ }
else
/* Running under --server-minor-version? */
return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
"Server doesn't advertise "
"SVN_RA_CAPABILITY_ATOMIC_REVPROPS");
-
- return SVN_NO_ERROR;
}
/* Parse SKEL_CSTR according to the description in USAGE_MSG. */
@@ -194,8 +222,9 @@
svn_string_t *old_propval;
const char *http_library;
char *digits_end = NULL;
+ svn_boolean_t want_error;
- if (argc != 6)
+ if (argc != 7)
{
fprintf(stderr, USAGE_MSG, argv[0], KEY_OLD_PROPVAL, KEY_NEW_PROPVAL);
exit(1);
@@ -216,6 +245,7 @@
propname = argv[3];
SVN_INT_ERR(extract_values_from_skel(&old_propval, &propval, argv[4], pool));
http_library = argv[5];
+ want_error = !strcmp(argv[6], "1");
if ((! SVN_IS_VALID_REVNUM(revision)) || (! digits_end) || *digits_end)
SVN_INT_ERR(svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
@@ -223,7 +253,7 @@
/* Do something. */
err = change_rev_prop(url, revision, propname, propval, old_propval,
- http_library, pool);
+ http_library, want_error, pool);
if (err)
{
svn_handle_error2(err, stderr, FALSE, "atomic-ra-revprop-change: ");
Index: subversion/tests/cmdline/prop_tests.py
===================================================================
--- subversion/tests/cmdline/prop_tests.py (revision 998887)
+++ subversion/tests/cmdline/prop_tests.py (working copy)
@@ -2010,8 +2010,8 @@
if svntest.main.server_has_atomic_revprop():
expected_stderr = ".*revprop 'flower' has unexpected value.*"
svntest.actions.run_and_verify_atomic_ra_revprop_change(
- None, None, expected_stderr, 1, repo_url, 0, 'flower',
- not_the_old_value, proposed_value)
+ None, None, expected_stderr, 0, repo_url, 0, 'flower',
+ not_the_old_value, proposed_value, True)
else:
expect_old_server_fail(not_the_old_value, proposed_value)
@@ -2019,7 +2019,7 @@
if svntest.main.server_has_atomic_revprop():
svntest.actions.run_and_verify_atomic_ra_revprop_change(
None, None, [], 0, repo_url, 0, 'flower',
- yes_the_old_value, proposed_value)
+ yes_the_old_value, proposed_value, False)
else:
expect_old_server_fail(yes_the_old_value, proposed_value)
