Bug 32896 – [bzt] Bugzilla-tool should not try to get the username and password from Git when we're using SVN

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

[Daniel Bates]

Bugzilla-tool tries to read the username and password from Git to use on bugs.webkit.org. However, if neither Git is installed nor we are using Git as our SCM for WebKit then bugzilla-tool will die (with a trace). For instance, we may be using SVN.

We should make bugzilla-tool handle the case when the external command (i.e. git) cannot be found.

[Daniel Bates]

Created attachment 45425 [details]
Trace

Python trace that occurred when I tried to land-patches 32799.

[Daniel Bates]

Created attachment 45426 [details]
Patch (Work in progress)

Work in progress.

[Adam Barth]

A couple comments:

1) We already have code that detects whether we're in a Git working copy: Git.in_working_directory in SCM.py.

2) We'll need a unittest.  You can look in credentials_unittest to see some examples.

[Daniel Bates]

Created attachment 45483 [details]
Patch (Work in progress)

Revised patch to use Git.in_working_directory.

Added argument cwd (current working directory) to Credentials constructor. Added unit test that uses SVNTestRepository to create a temporary SVN repo. before calling credentials.read_credentials.

[Daniel Bates]

Created attachment 45484 [details]
Patch (Work in progress)

[Daniel Bates]

Created attachment 45538 [details]
Patch with unit tests

Notice, calling Git.in_working_directory can throw an OSError errrno 2 exception if the git command is not found because we call Executive.run_command which does not catch the exception. Hence, this patch modifies Executive.run_command to catch these exceptions and passes them to the specified error handler/returns the error code of the exception/returns the error message of the exception.

For the credentials unit test, I assume SVN is installed as per <http://webkit.org/building/tools.html> at the time of this writing, since SVNTestRepository (imported from modules.scm_unittest) assumes it.

Also, I added a new unit test file, executive_unittest.py, to test the change made to Executive.run_command.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45538 [details] without any errors.

[Adam Barth]

Comment on attachment 45538 [details]
Patch with unit tests

This looks good.  Thanks Dan!

[Daniel Bates]

Committed in <http://trac.webkit.org/changeset/52587>.

[Eric Seidel (no email)]

This broke a couple scm.py unit tests:
ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py", line 150, in test_error_handlers
    self.assertRaises(OSError, run_command, command_does_not_exist)
  File "/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/unittest.py", line 320, in failUnlessRaises
    callableObj(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 70, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 142, in run_command
    (error_handler or self.default_error_handler)(script_error)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 113, in default_error_handler
    raise error
ScriptError: Failed to run "['does_not_exist', 'invalid_option']" exit_code: 2

I'm not sure catching OSError here is a good idea.  It turns missing scripts into "exit code 2" which happens to be the same as QueueEngine.handled_error_code = 2

We obviously could change handled_error_code....

[Daniel Bates]

Will look into this issue.

(In reply to comment #10)
> This broke a couple scm.py unit tests:
> ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
> "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py",
> line 150, in test_error_handlers
>     self.assertRaises(OSError, run_command, command_does_not_exist)
>   File
> [...]
> I'm not sure catching OSError here is a good idea.  It turns missing scripts
> into "exit code 2" which happens to be the same as
> QueueEngine.handled_error_code = 2
> 
> We obviously could change handled_error_code....