On Sun, Jun 14, 2009 at 08:53:19AM +0800, yellow.flying wrote:
> Hey Stefan,
>
> I am sorry. I can understand what you mean now. I rewrite the patch
> commit_multiple_wc, and after this patch is submitted, I will write another
> patche-- commit_multiple_wc_multiple_repos.
>
> log message:
>
> [[[
>
> * subversion/tests/cmdline/commit_tests.py
> (commit_multiple_wc_nested): Rename commit_multiple_wc as
> commit_multiple_wc_nested, and mark it as XFail. It will be changed to Pass
> after we have fix issue #2381.
> (commit_multiple_wc_not_nested): new test, This test is the same as
> commit_multiple_wc_nested except that two wcs in this test are not nested.
> ]]]
>
> I hope this time it is ok，thank you.
It is mainly OK, except a few minor details. I have committed a patch
based on your patch in r38030, and also added another test.
Please carefully look at the patch I committed to make sure you
agree with it. If you see any problem please tell me!
Also please look at the log message carefully. For example,
your log message didn't mention the changes you made to test_list.
A good rule is to remember is that you need to mention *every* hunk
of the patch in your log message (a "hunk" starts with an @@ ... @@ line),
describing the change made by the hunk.
Some comments on your patch:
> Index: commit_tests.py
> ===================================================================
> --- commit_tests.py (°æ±¾ 38025)
> +++ commit_tests.py (¹¤×÷¸±±¾)
If you can, please create patches from the working copy root,
so that full path subversion/tests/cmdline/commit_tests.py
appears in the patch. But that is not very important.
> @@ -1384,17 +1388,72 @@
> expected_status2.tweak('A/B/lambda', status='M ')
> svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
>
> - # Commit should fail, even though one target is a "child" of the other.
> - svntest.actions.run_and_verify_svn("Unexpectedly not locked",
> - None, svntest.verify.AnyOutput,
> + # Commit should fail, even though one target is a "child" of the other,
> + # since targets come from different wcs.
> + svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
> 'commit', '-m', 'log',
> wc_dir, wc2_dir)
In this hunk, you have changed the function to expect the commit
to succeed, but you didn't update the comment above the function!
Please make sure to always keep comments in sync with code you are
changing. Otherwise, someone might look at the code and the comment
later and get confused.
Also, when you pass "None" for stdin or stderr to a run_and_verify_*
function, the function will throw an exception saying "stderr cannot
be None". This will make your test see to XFail for a different reason
than you want it to fail for. If you don't expect output, pass an empty
list [], not None. See my commit for examples.
In general, always make sure you understand why a test is failing.
This is not easy with XFAIL tests.
For example, in the commit_multiple_wc test, we expect Subversion
to complain about the parent of the working copies not being
a working copy. Running the test, you will see that this is what
really happens:
[~/svn/svn-trunk/subversion/tests/cmdline] $ ./commit_tests.py 26
subversion/svn/commit-cmd.c:137: (apr_err=155007)
subversion/libsvn_client/commit.c:1522: (apr_err=155007)
subversion/libsvn_wc/lock.c:593: (apr_err=155007)
subversion/libsvn_wc/lock.c:484: (apr_err=155007)
svn: '/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svn-test-work/working_copies/commit_tests-26' is not a working copy
Here you see svn complaining about the right thing.
If you see any different error, the test failed, but not for the correct
reason!
Traceback (most recent call last):
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 1141, in run
rc = self.pred.run(sandbox)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/testcase.py", line 61, in run
return self._delegate.run(sandbox)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/testcase.py", line 121, in run
return self.func(sandbox)
File "./commit_tests.py", line 1439, in commit_multiple_wc
wc1_dir, wc2_dir)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/actions.py", line 199, in run_and_verify_svn
expected_exit, *varargs)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/actions.py", line 232, in run_and_verify_svn2
exit_code, out, err = main.run_svn(want_err, *varargs)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 579, in run_svn
*(_with_auth(_with_config_dir(varargs))))
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 357, in run_command
None, *varargs)
File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 512, in run_command_stdin
raise Failure
Failure
XFAIL: commit_tests.py 26: commit from two working copies
So just looking at the last line does not give any information
when we are dealing with XFAIL tests. That makes it a bit hard
to work with them. It's much easier when tests are expected to PASS.
You can get even more detailed test output by passing the --verbose
option to the test script. This often helps to understand better
why a test is failing. Try running:
commit_tests.py --verbose 25
> + # Commit should fail, since targets come from different wcs.
> + svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
> + 'commit', '-m', 'log',
> + wc1_dir, wc2_dir)
Here, you also forgot to change the comment.
> @@ -2696,7 +2755,8 @@
> commit_from_long_dir,
> commit_with_lock,
> commit_current_dir,
> - commit_multiple_wc,
> + XFail(commit_multiple_wc_nested),
> + XFail(commit_multiple_wc_not_nested),
> commit_nonrecursive,
> failed_commit,
> commit_out_of_date_deletions,
The above change was not mentioned in your log message (I already
said this above).
Overall, the patch was good!
And I hope the tests will help you writing code that actually
solves issue #2381.
Thanks :)
Stefan
Received on 2009-06-14 14:10:17 CEST