[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Add regression test for issue #3686:executable flag not correctly set on merge

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 18 Jan 2011 19:08:14 +0200

Daniel Becroft wrote on Tue, Jan 18, 2011 at 07:13:12 +1000:
> Hi guys,
>
> I was looking through the "Getting Involved" section of the Subversion
> website, and decided to start by writing a regression test for an
> issue, starting with 3686 (executable flag not correctly set on
> merge). This issue was already assigned to someone, so apologies if
> it's already being worked on.
>

Welcome aboard :-).

As a note for next time, though, when an issue is assigned to someone,
it would be better to ask them (or dev@) whether they're working on it
before starting to work on it yourself. I'm CCing Blair now for this
reason.

> I've attached the patch, and included it below as well. A few questions I have:
>
> 1) I couldn't find a precedent for examining the OS's executable flag,
> so my approach might be less than ideal. Should I add a test to
> prop_tests.py to check that setting svn:executable also sets the
> executable bit?

No, the test you're just adding already does just that. Just copy the
two asserts above the "# commit r2" line to after the comment and you're
done :-)

> 2) The problem also exists under the '--reintegrate' scenario, even
> after a subsequent commit. Would it be better to include that case
> here, or move the entire test to the merge_reintegrate.py suite?
>

I'm not sure we need a separate test for the --reintegrate case;
it seems reasonable to assume it does (and will) share the 'set +x
permission' logic with normal merges.

As to location, if I wrote this I'd probably put it in prop_tests or
special_tests along with the other svn:executable tests. (That's
somewhat of a bikeshed, though, and I don't feel strongly about this.)

> Regards,
> Daniel Becroft
>
>
> [[[
> Add regression test for issue #3686.
>
> This issue involves the executable flag being lost during a merge of a
> binary file with the 'svn:executable' property set.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_change_to_file_with_executable(sbox)) New test case.

Drop the signature, and add a colon:

    (merge_change_to_file_with_executable): New test case.

> ]]]

(you've attached the patch both inline and as an attachment, I'll review
only the latter)

> Index: subversion/tests/cmdline/merge_tests.py
> ===================================================================
> --- subversion/tests/cmdline/merge_tests.py (revision 1059656)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -16316,6 +16316,78 @@
> "Subtree merge under working merge produced the wrong mergeinfo",
> '/A/C/nu:9', [], 'pg', SVN_PROP_MERGEINFO, nu_COPY_path)
>
> +
> +#----------------------------------------------------------------------
> +# Test for issue #3686 'executable flag not correctly set on merge'
> +# See http://subversion.tigris.org/issues/show_bug.cgi?id=3686
> +def merge_change_to_file_with_executable(sbox):
> + "executable flag is maintained during binary merge"
> +
> + # Scenario: When merging a change to a binary file with the 'svn:executable'
> + # property set, the file is not marked as 'executable'. After commit, the
> + # executable bit is set correctly.
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + trunk_url = sbox.repo_url + '/A/B/E'
> +
> + alpha_path = os.path.join(wc_dir, "A", "B", "E", "alpha")
> + beta_path = os.path.join(wc_dir, "A", "B", "E", "beta")
> +
> + # Force one of the files to be a binary type
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:mime-type', 'application/octet-stream',
> + alpha_path)
> +
> + # Set the 'svn:executable' property on both files
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:executable', 'ON',
> + beta_path)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:executable', 'ON',
> + alpha_path)
> +
> + # Verify the executable bit has been set before committing
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +

No. Please use 'assert' for bugs in the test itself, and something else
('raise svntest.Failure' is the most generic) for bugs in the code being
tested. Example:

     struct test_case {
       const char *input;
       int *exit_code;
     } test_cases[] = { ... };

     for (i = 0; i < NUMBER_OF_TESTS; i++)
       {
         struct test_case t = test_cases[i];
         SVN_ERR_ASSERT(t.exit_code);
         SVN_TEST_ASSERT(system(t.input) == *t.exit_code);
       }

> + # Commit change (r2)
> + sbox.simple_commit()
> +
> + # Create the branch
> + svntest.actions.run_and_verify_svn(None, None, [], 'cp',
> + trunk_url,
> + sbox.repo_url + '/branch',
> + '-m', "Creating the Branch")
> +
> + # Modify the files + commit (r3)
> + svntest.main.file_append(alpha_path, 'appended alpha text')
> + svntest.main.file_append(beta_path, 'appended beta text')
> + sbox.simple_commit()
> +
> + svntest.actions.run_and_verify_svn(None, None, [], 'switch',
> + sbox.repo_url + '/branch',
> + wc_dir)

A comment before this line would be useful, in my opinion.

> +
> + # Recalculate the paths
> + alpha_path = os.path.join(wc_dir, "alpha")
> + beta_path = os.path.join(wc_dir, "beta")
> +
> + # Merge the changes across
> + svntest.actions.run_and_verify_svn(None, None, [], 'merge',
> + trunk_url, wc_dir)
> +
> + # Verify the executable bit has been set
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +

As expected, the test passes if you remove these two lines.

> + # Commit (r4)
> + sbox.simple_commit()
> +
> + # Verify the executable bit has been set
> + assert os.access(alpha_path, os.X_OK)
> + assert os.access(beta_path, os.X_OK)
> +
> ########################################################################
> # Run the tests
>
> @@ -16507,6 +16579,7 @@
> merge_with_os_deleted_subtrees,
> no_self_referential_or_nonexistent_inherited_mergeinfo,
> XFail(subtree_merges_inherit_invalid_working_mergeinfo),
> + XFail(SkipUnless(merge_change_to_file_with_executable, svntest.main.is_posix_os)),
> ]
>
> if __name__ == '__main__':

In short: +1 to commit, assuming you replace the asserts with something
else per my earlier comment.

(you aren't a committer yet, though, so please re-send the patch)
Received on 2011-01-18 18:12:41 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.