On Fri, Jun 17, 2011 at 12:34 PM, Dirk Thomas <websvn_at_dirk-thomas.net> wrote:
>> That does appear to be a bug. I don't see a similar issue in the
>> tracker; would you mind filing a new issue? No worries if you cannot,
>> I can do it at some point today.
>
> i have created issue #3931.
Hi Dirk,
Thanks for adding the issue.
>> I'll commit your patch once we have an issue.
>
> Prepend:
> @Issue(3931)
> before "def log_unrelated(sbox):"
>
>
>> Note that this is not a regression from 1.6.17 so it
>> probably may not be fixed for 1.7.0 (I'm not sure if this is hindering
>> your work on issue #3830).
>
> But may be it is possible that someone takes a closer look before in order
> to decide if it easily fixable or not.
Agreed, I just wanted to mention it was not a regression. We are
pretty focused on 1.7.0 right now, so most of the focus is on blockers
for that.
>>> + target = os.path.join(sbox.repo_url, 'A', 'D', 'G', 'rho') + "@2"
>>
>> Don't construct URLs with os.path.join, it uses os.sep to join the
>> paths, which on windows is '\', so you won't end up with a valid URL.
>
> This is done using os.path.join in the whole file - then all other test
> functions should be updated accordingly?
You are likely looking at the creation of *working copy* targets, your
patch is creating a *URL* target. We use os.path.join when creating
WC paths in the test to maintain cross-platform compatibility (Windows
with its crazy '\' path separators :-). If you replaced
"sbox.repo_url" with "sbox.wc_dir" your patch would be fine. Or my
suggestion would work too. Doesn't matter much either way.
>> A few comments explaining where/why the test fails is always helpful.
>> Sure, we'll have an associated issue and/or dev threads to check, but
>> a quick bit like this never hurts:
>>
>>> + # Currently this test fails because instead of returning the
>>> expected
>>> + # 'Unable to find repository location for ^/A/D/G/rho in revision 9'
>>> + # error, the log for ^/A/D/G/rho_at_8 is returned, but that is an
>>> unrelated
>> > + # line of history.
>>
>> This is especially helpful in more complex tests which start failing
>> for other reasons. When someone comes along months (or even years)
>> later to work on an issue it's really nice to know if you have new
>> problems to tackle.
>
> Of course, are you going to add the comments before submitting the patch or
> do you want be to provide an updated diff?
I'll add them, no need for a new patch.
>>> + file_not_found = ".*File not found.*"
>>> +
>>> + exit_code, output, err = svntest.actions.run_and_verify_svn(
>>> + None, None, file_not_found,
>>> + 'log', '-r', '6:7', target)
>>> + exit_code, output, err = svntest.actions.run_and_verify_svn(
>>> + None, None, file_not_found,
>>> + 'log', '-r', '7:6', target)
>>
>> This is personal preference, but since this last block works maybe
>> move it ahead of the failing block?
>
>
> Ok, actually this last block testing for "File not found" is not really
> needed.
> It mainly exists to confirm that patch for 3830 has no regressions.
> May be it would be better to move this test to a separate test-function (as
> it is unrelated to the name of this test: "log_unrelated")
No need for a separate test I think. We try to walk a fine line
between "too many small tests" and "too few big tests that do too
much". I think we are fine here.
Paul
Received on 2011-06-17 19:05:28 CEST