Bug 106420 – Fixing AuthenticationError when running test-webkitpy as a non-committer.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim 'mithro' Ansell]

Fixing AuthenticationError when running test-webkitpy as a non-committer.

[Tim 'mithro' Ansell]

Created attachment 181855 [details]
Patch

[WebKit Review Bot]

Attachment 181855 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/svn.py, line         # ignore false positives for methods implemented in the mixee class. pylint: disable-msg=E1101
)
  DeprecationWarning)
Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:304:  [SCMTest._shared_test_changed_files] Instance of 'SCMTest' has no 'scm' member  [pylint/E1101] [5]
Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:308:  [SCMTest._shared_test_changed_files] Instance of 'SCMTest' has no 'scm' member  [pylint/E1101] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 181855 [details]
Patch

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

> Tools/ChangeLog:7
> +

Could you explain a bit more here about what sort of changes you're making here?  It's not immediately clear if this is all search-replace or one-off-fixes or what?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:128
> +class SVNTestRepository(object):

Welcome to our cave.  Do you like our charcoal buffalo?

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:345
> +        self.assertEqual(sorted(changed_files), sorted(["test_dir/test_file3", "test_file"]))

I wonder if fancy new unitests has a sort-for-you method.  Not that this is hard.  We've talked of moving to unittests2 since we can't quite drop 2.6 yet.  (We're held back by the last version of Mac OS X which Apple WebKit supports, as we try not to have contributors have to install any more than absolutely necessary).

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1055
> +        self.scm = detect_scm_system(self.git_checkout_path)

I believe there is a newer SCMDetector object which is smarter than this free-function.  But that's another change for another time. :)

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1555
> +        cwd = tempfile.mkdtemp(suffix="git_test_with_mock")

Why do the mock tests need a real tempfile?  Don't they work from a MockFileSystem()?

[Eric Seidel (no email)]

Mostly I just need more context for the cleanup.  Or it's possible there is a search-replace part of this which would be trivial to land first? (even if it didn't change behavior/fix tests)

[Tim 'mithro' Ansell]

Comment on attachment 181855 [details]
Patch

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

>> Tools/ChangeLog:7
>> +
> 
> Could you explain a bit more here about what sort of changes you're making here?  It's not immediately clear if this is all search-replace or one-off-fixes or what?

There is a couple of things which I've changed;
 * Sort the list inputs into a deterministic order (see the added sorted())
 * Change all tests to use self.scm rather then a local scm
 * Make the self.scm object override the svn_auth_realm

>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:345
>> +        self.assertEqual(sorted(changed_files), sorted(["test_dir/test_file3", "test_file"]))
> 
> I wonder if fancy new unitests has a sort-for-you method.  Not that this is hard.  We've talked of moving to unittests2 since we can't quite drop 2.6 yet.  (We're held back by the last version of Mac OS X which Apple WebKit supports, as we try not to have contributors have to install any more than absolutely necessary).

Yes, they have an "assertXXXEqual" which doesn't compare order.

>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1055
>> +        self.scm = detect_scm_system(self.git_checkout_path)
> 
> I believe there is a newer SCMDetector object which is smarter than this free-function.  But that's another change for another time. :)

Agreed

>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1555
>> +        cwd = tempfile.mkdtemp(suffix="git_test_with_mock")
> 
> Why do the mock tests need a real tempfile?  Don't they work from a MockFileSystem()?

cwd can't be none. Some of these tests are actually still broken.

[Eric Seidel (no email)]

Comment on attachment 181855 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:1555
>>> +        cwd = tempfile.mkdtemp(suffix="git_test_with_mock")
>> 
>> Why do the mock tests need a real tempfile?  Don't they work from a MockFileSystem()?
> 
> cwd can't be none. Some of these tests are actually still broken.

I see, Git is still a real object and uses a real FileSystem (because it doesn't even take a FileSystem parameter!) so it needs a real path?

[Tim 'mithro' Ansell]

Created attachment 182098 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 182098 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 182098 [details]
Patch

Rejecting attachment 182098 [details] from commit-queue.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', u'--status-host=queues.webkit.org', ..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13309 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
53>At revision 13309.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15808013

[Adam Barth]

    The following ChangeLog files contain OOPS:

        trunk/Tools/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.

[Adam Barth]

Comment on attachment 182098 [details]
Patch

Your patch has two ChangeLog entries in one ChangeLog.  Please use one ChangeLog entry per patch.

[Tim 'mithro' Ansell]

Created attachment 182241 [details]
Patch

[Tim 'mithro' Ansell]

Before:
 Ran 1733 tests in 40.638s                                                                             
 FAILED (failures=0, errors=22)

After:
 Ran 1733 tests in 43.084s                                                                                     
 FAILED (failures=1, errors=8)

[Eric Seidel (no email)]

Comment on attachment 182241 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 182241 [details]
Patch

Clearing flags on attachment: 182241

Committed r139399: <http://trac.webkit.org/changeset/139399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.