You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Noorul Islam K M <no...@collab.net> on 2011/02/10 04:34:11 UTC
[PATCH] cat tests fails for http:// proto
It looks like for file:// and http:// protocols cat prints different
warning messages for non-existing target.
For http://
svn: warning: W160013:
'/svn-test-work/repositories/cat_tests-9/!svn/bc/1/non-existing' path
not found
For file://
svn: warning: W160013: File not found: revision 1, path '/non-existing'
Log
[[[
For file:// and http:// protocols cat prints different warning messages
for non-existing target.
* subversion/tests/cmdline/cat_tests.py
(cat_non_existing_remote_file): Modify regular expression to handle
both http:// and file:// targets.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: [PATCH] cat tests fails for http:// proto
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Thu, 2011-02-10, Noorul Islam K M wrote:
>
>> It looks like for file:// and http:// protocols cat prints different
>> warning messages for non-existing target.
>>
>> For http://
>>
>> svn: warning: W160013:
>> '/svn-test-work/repositories/cat_tests-9/!svn/bc/1/non-existing' path
>> not found
>>
>> For file://
>>
>> svn: warning: W160013: File not found: revision 1, path '/non-existing'
>
> OK. Ideally, I think, we should aim to enhance Subversion's error
> reporting so that it reports the same high-level error message for any
> protocol, as a wrapper around the protocol-specific lower-level
> messages. But first we can just work with what we have.
>
> I see that your patch is modifying a test's expectations to match both
> error messages. Is the test currently failing, or wrongly passing, due
> to this difference? In what way does the patch change the test results?
>
This is actually committed now in r1069330. The test was failing for
http:// and svn://. The patch looks for the error code alone.
> You didn't mention the svn:// protocol. Do you need to adjust for that
> too?
>
No need to adjust for that.
>
>> Log
>>
>> [[[
>> For file:// and http:// protocols cat prints different warning messages
>> for non-existing target.
>
> That statement supplies some relevant information, but a better log
> message would begin with a high level summary of the *change* being
> made. For this patch, the simplest top-level summary could be:
>
> Fix an error in a test.
>
> The ideal level of detail for the first sentence is just enough for any
> other developer to decide whether they need to read further and learn
> the details of this change. Something like this in style:
>
> Correct the expectations of an XFail test that has been failing for
> the wrong reason (when using http:// protocol) since rXXXXXXX.
>
> ... although I don't think that is a correct description of this
> particular patch.
>
I will keep all these points in mind. Thank you for taking your time
reviewing this.
Thanks and Regards
Noorul
>
>
>> * subversion/tests/cmdline/cat_tests.py
>> (cat_non_existing_remote_file): Modify regular expression to handle
>> both http:// and file:// targets.
>> ]]]
>
>> Index: subversion/tests/cmdline/cat_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/cat_tests.py (revision 1069209)
>> +++ subversion/tests/cmdline/cat_tests.py (working copy)
>> @@ -238,8 +238,7 @@
>> sbox.build(create_wc = False)
>> non_existing_path = sbox.repo_url + '/non-existing'
>>
>> - expected_err = "svn: warning: W160013: File not found.*" + \
>> - non_existing_path.split('/')[1]
>> + expected_err = "svn: warning: W160013: .*not found.*"
>>
>> # cat operation on non-existing remote path should return 1
>> svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
Re: [PATCH] cat tests fails for http:// proto
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2011-02-10, Noorul Islam K M wrote:
> It looks like for file:// and http:// protocols cat prints different
> warning messages for non-existing target.
>
> For http://
>
> svn: warning: W160013:
> '/svn-test-work/repositories/cat_tests-9/!svn/bc/1/non-existing' path
> not found
>
> For file://
>
> svn: warning: W160013: File not found: revision 1, path '/non-existing'
OK. Ideally, I think, we should aim to enhance Subversion's error
reporting so that it reports the same high-level error message for any
protocol, as a wrapper around the protocol-specific lower-level
messages. But first we can just work with what we have.
I see that your patch is modifying a test's expectations to match both
error messages. Is the test currently failing, or wrongly passing, due
to this difference? In what way does the patch change the test results?
You didn't mention the svn:// protocol. Do you need to adjust for that
too?
> Log
>
> [[[
> For file:// and http:// protocols cat prints different warning messages
> for non-existing target.
That statement supplies some relevant information, but a better log
message would begin with a high level summary of the *change* being
made. For this patch, the simplest top-level summary could be:
Fix an error in a test.
The ideal level of detail for the first sentence is just enough for any
other developer to decide whether they need to read further and learn
the details of this change. Something like this in style:
Correct the expectations of an XFail test that has been failing for
the wrong reason (when using http:// protocol) since rXXXXXXX.
... although I don't think that is a correct description of this
particular patch.
- Julian
> * subversion/tests/cmdline/cat_tests.py
> (cat_non_existing_remote_file): Modify regular expression to handle
> both http:// and file:// targets.
> ]]]
> Index: subversion/tests/cmdline/cat_tests.py
> ===================================================================
> --- subversion/tests/cmdline/cat_tests.py (revision 1069209)
> +++ subversion/tests/cmdline/cat_tests.py (working copy)
> @@ -238,8 +238,7 @@
> sbox.build(create_wc = False)
> non_existing_path = sbox.repo_url + '/non-existing'
>
> - expected_err = "svn: warning: W160013: File not found.*" + \
> - non_existing_path.split('/')[1]
> + expected_err = "svn: warning: W160013: .*not found.*"
>
> # cat operation on non-existing remote path should return 1
> svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,