> 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.
> 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.
>> + 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?
> 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?
>> + 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")
Dirk
Received on 2011-06-17 18:34:57 CEST