Bug 114386 – [webkitpy] SVNTest fails four tests when using subversion client 1.7 or later

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Glenn Adams]

If the local repository is git based rather than svn based, then the following 4 tests fail when running test-webkitpy --all as follows. In each case, an attempt is being made to invoke 'svn' directly.

% test-webkitpy --all
Suppressing most webkitpy logging while running unit tests.

[128/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_add_recursively erred:
  Traceback (most recent call last):
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 825, in test_add_recursively
      self._shared_test_add_recursively()
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 466, in _shared_test_add_recursively
      self.scm.add("added_dir/added_file")
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add
      self.add_list([path], return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list
      return self._run_svn(["add"] + paths, return_exit_code=return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn
      return self.run([self.executable_name] + args, **kwargs)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run
      decode_output=decode_output)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler
      raise error
  ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1

[143/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_delete_recursively erred:
  Traceback (most recent call last):
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 839, in test_delete_recursively
      self._shared_test_delete_recursively()
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 472, in _shared_test_delete_recursively
      self.scm.add("added_dir/added_file")
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add
      self.add_list([path], return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list
      return self._run_svn(["add"] + paths, return_exit_code=return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn
      return self.run([self.executable_name] + args, **kwargs)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run
      decode_output=decode_output)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler
      raise error
  ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1

[144/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_delete_recursively_or_not erred:
  Traceback (most recent call last):
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 842, in test_delete_recursively_or_not
      self._shared_test_delete_recursively_or_not()
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 481, in _shared_test_delete_recursively_or_not
      self.scm.add("added_dir/added_file")
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add
      self.add_list([path], return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list
      return self._run_svn(["add"] + paths, return_exit_code=return_exit_code)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn
      return self.run([self.executable_name] + args, **kwargs)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run
      decode_output=decode_output)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler
      raise error
  ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1

[495/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_svn_lock failed:
  Traceback (most recent call last):
    File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 897, in test_svn_lock
      self.assertRaises(ScriptError, run_command, ['svn', 'update'])
  AssertionError: ScriptError not raised

Ran 1584 tests in 39.856s
FAILED (failures=1, errors=3)

[Glenn Adams]

Further investigation shows this isn't an issue with git-svn vs svn.

[Glenn Adams]

Turns out these fails are caused by changes introduced by svn client version 1.7:

* .svn/lock no longer used
* svn add needs --parents option to add intermediate directories, and returns exit code 1 if a dir/file has already been added

[Glenn Adams]

Created attachment 197465 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 197465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894
> +        if self.scm.svn_version() > "1.6":

Why don't we convert the svn version to float otherwise, we'll be doing a string comparison.
Also, it's probably better to do >= 1.7 instead.

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:895
> +            # .svn/lock isn't used with svn client 1.7 and later

I would say "subversion client" or "svn", not "svn client".

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:187
> +        if self.svn_version() < "1.7":

Ditto about casting it to float first.

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:198
> +        if return_exit_code:
> +            return exit_code
> +        if exit_code and exit_code != 1:
> +            raise ScriptError(script_args=svn_add_args, exit_code=exit_code)

We might need to return 0 when the exit code is 1 and return_exit_code is set True for the compatibility.

[Glenn Adams]

(In reply to comment #4)
> (From update of attachment 197465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894
> > +        if self.scm.svn_version() > "1.6":
> 
> Why don't we convert the svn version to float otherwise, we'll be doing a string comparison.
> Also, it's probably better to do >= 1.7 instead.

The problem is that the version string is normally three parts, e.g., "1.7.8" is returned from my current version. Also there was existing code that did a string comparison: see SVN._status_regexp().

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:895
> > +            # .svn/lock isn't used with svn client 1.7 and later
> 
> I would say "subversion client" or "svn", not "svn client".

OK, i can change that comment before committing.

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:187
> > +        if self.svn_version() < "1.7":
> 
> Ditto about casting it to float first.

See above.

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:198
> > +        if return_exit_code:
> > +            return exit_code
> > +        if exit_code and exit_code != 1:
> > +            raise ScriptError(script_args=svn_add_args, exit_code=exit_code)
> 
> We might need to return 0 when the exit code is 1 and return_exit_code is set True for the compatibility.

* SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
* MockSCM.add() does the same thing
* BaselineOptimizer._move_baselines ignores the return value from add_list
* AbstractParallelRebaselineCommand._rebaseline also ignores

So no caller of add_list is actually using the return value, thus I see no compat issue.

[Glenn Adams]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 197465 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review
> > 
> > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894
> > > +        if self.scm.svn_version() > "1.6":
> > 
> > Why don't we convert the svn version to float otherwise, we'll be doing a string comparison.
> > Also, it's probably better to do >= 1.7 instead.
> 
> The problem is that the version string is normally three parts, e.g., "1.7.8" is returned from my current version. Also there was existing code that did a string comparison: see SVN._status_regexp().

As for using >= "1.7",  you are right; i will also change that prior to commit; i.e., "1.6.1" is lexically greater than "1.6" but I don't want the test skipped in that case.

[Glenn Adams]

Created attachment 197498 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
>
> * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
> * MockSCM.add() does the same thing
> * BaselineOptimizer._move_baselines ignores the return value from add_list
> * AbstractParallelRebaselineCommand._rebaseline also ignores
> 
> So no caller of add_list is actually using the return value, thus I see no compat issue.

Why don't we get rid of return_exit_code in that case?  Sounds like this argument is completely useless.

[Glenn Adams]

(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #4)
> >
> > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
> > * MockSCM.add() does the same thing
> > * BaselineOptimizer._move_baselines ignores the return value from add_list
> > * AbstractParallelRebaselineCommand._rebaseline also ignores
> > 
> > So no caller of add_list is actually using the return value, thus I see no compat issue.
> 
> Why don't we get rid of return_exit_code in that case?  Sounds like this argument is completely useless.

I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix.

[Glenn Adams]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > >
> > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
> > > * MockSCM.add() does the same thing
> > > * BaselineOptimizer._move_baselines ignores the return value from add_list
> > > * AbstractParallelRebaselineCommand._rebaseline also ignores
> > > 
> > > So no caller of add_list is actually using the return value, thus I see no compat issue.
> > 
> > Why don't we get rid of return_exit_code in that case?  Sounds like this argument is completely useless.
> 
> I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix.

Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py.

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > >
> > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
> > > > * MockSCM.add() does the same thing
> > > > * BaselineOptimizer._move_baselines ignores the return value from add_list
> > > > * AbstractParallelRebaselineCommand._rebaseline also ignores
> > > > 
> > > > So no caller of add_list is actually using the return value, thus I see no compat issue.
> > > 
> > > Why don't we get rid of return_exit_code in that case?  Sounds like this argument is completely useless.
> > 
> > I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix.
> 
> Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py.

But we're adding a code that nobody uses in practice. That seems like a bad idea.

[Ryosuke Niwa]

Comment on attachment 197498 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197498&action=review

On my second thought, we shouldn't be adding code that nobody really uses. r-.

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:196
> +        if return_exit_code:
> +            return exit_code

Please mention, in the change log, it's okay to return 1 in some cases.

[Glenn Adams]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > >
> > > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed.
> > > > > * MockSCM.add() does the same thing
> > > > > * BaselineOptimizer._move_baselines ignores the return value from add_list
> > > > > * AbstractParallelRebaselineCommand._rebaseline also ignores
> > > > > 
> > > > > So no caller of add_list is actually using the return value, thus I see no compat issue.
> > > > 
> > > > Why don't we get rid of return_exit_code in that case?  Sounds like this argument is completely useless.
> > > 
> > > I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix.
> > 
> > Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py.
> 
> But we're adding a code that nobody uses in practice. That seems like a bad idea.

I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not.

[Ryosuke Niwa]

(In reply to comment #13)
> I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not.

In WebKit, we don't normally fix code that's never used. If we find out that some feature or API is never used, we try to delete that part of the API as soon as possible.

[Glenn Adams]

(In reply to comment #12)
> (From update of attachment 197498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197498&action=review
> 
> On my second thought, we shouldn't be adding code that nobody really uses. r-.

I don't understand this comment. We aren't adding code that nobody really uses. We are maintaining an existing API's semantics without changing it. Someone else put the argument there in the first place. I can't say if it was used in the past and that usage was removed or if they anticipated its use in the future.

Simplifying or changing the API is out of scope of the fix, and should not be merged into this patch IMO. If you want me to do a follow on patch that changes this API, I can do that, but I don't want to merge it with this patch since it is unrelated.

> 
> > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:196
> > +        if return_exit_code:
> > +            return exit_code
> 
> Please mention, in the change log, it's okay to return 1 in some cases.

OK.

[Ryosuke Niwa]

(In reply to comment #15)
>
> Simplifying or changing the API is out of scope of the fix, and should not be merged into this patch IMO. If you want me to do a follow on patch that changes this API, I can do that, but I don't want to merge it with this patch since it is unrelated.

Then please remove the argument first.

[Glenn Adams]

(In reply to comment #14)
> (In reply to comment #13)
> > I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not.
> 
> In WebKit, we don't normally fix code that's never used. If we find out that some feature or API is never used, we try to delete that part of the API as soon as possible.

I don't understand this comment. I'm not fixing code that's never used. I'm fixing three failed tests that fail because this code doesn't take into account subversion 1.7 semantics.

As I said in my prior comment, I'd be happy to do another, follow-on patch that changes the API, but I don't want to merge it with this patch/bug since it serves an entirely different purpose.

[Glenn Adams]

Created attachment 197509 [details]
Patch

[Benjamin Poulain]

Comment on attachment 197509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197509&action=review

> Tools/ChangeLog:7
> +

I think you should have a word about why the test were failing on 1.7 and later.

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:194
> +            if exit_code and exit_code != 1:

Maybe just my coding style but to avoid magic numbers I would do:
    added_tracked_file_exit_code = 1
    if exit_code and exit_code != added_tracked_file_exit_code:

[Glenn Adams]

Created attachment 197512 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 197512 [details]
Patch for landing

Clearing flags on attachment: 197512

Committed r148177: <http://trac.webkit.org/changeset/148177>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.