On Fri, Jun 17, 2011 at 12:34 PM, Dirk Thomas <web...@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@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
