webkit-patch: system keyring is not used to read my password

• Component: Tools / Tests
• Status: RESOLVED FIXED

[arno.]

Hi,
I'm using ubuntu.
I haven't set username and password in git options, neither in environment.
when I upload a patch to bugzilla (with webkit-patch upload), I'm asked if I want to store my username and password in the system keyring. I say yes, and the credentials are stored.
But those credentials are not used next time I use webkit-patch upload.

[arno.]

That's because in Credentials.read_credentials, we try to read password from keyring before prompting for username.
Instead, if username was not read from environment, git, or keychain, we could prompt for username, and try to read password from keyring after.

[arno.]

Created attachment 155020 [details]
patch proposal

[Dirk Pranke]

Comment on attachment 155020 [details]
patch proposal

Seems reasonable to me. Is it possible to write a test for this?

[arno.]

(In reply to comment #3)
> (From update of attachment 155020 [details])
> Seems reasonable to me. Is it possible to write a test for this?

I don't really known how to write a test for that:
if we want to create unittest, at some point, the User.prompt() method will block them.
Is there a way to override User.prompt() with a mock object ?

[Adam Barth]

Yes.  If you use a MockHost object, the User object will be mocked out for you by default.

[Adam Barth]

You can grep for "MockUser" to see how it gets used.  Many existing tests use it.

[arno.]

Created attachment 155337 [details]
updated patch with test

[Dirk Pranke]

Comment on attachment 155337 [details]
updated patch with test

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

> Tools/Scripts/webkitpy/common/net/credentials.py:136
> +    def read_credentials(self, User=User):

the host has a user field, so you should be able to just say self.host.user.prompt(), and then down in your test you can create a credentials object, pass it a mock host, and override the prompt method. That should cut down on the scaffolding a fair amount.

[arno.]

(In reply to comment #8)
> (From update of attachment 155337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155337&action=review
> 
> > Tools/Scripts/webkitpy/common/net/credentials.py:136
> > +    def read_credentials(self, User=User):
> 
> the host has a user field, so you should be able to just say self.host.user.prompt(), and then down in your test you can create a credentials object, pass it a mock host, and override the prompt method. That should cut down on the scaffolding a fair amount.

In Credentials, host is just a string

[Dirk Pranke]

Comment on attachment 155337 [details]
updated patch with test

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

>>> Tools/Scripts/webkitpy/common/net/credentials.py:136
>>> +    def read_credentials(self, User=User):
>> 
>> the host has a user field, so you should be able to just say self.host.user.prompt(), and then down in your test you can create a credentials object, pass it a mock host, and override the prompt method. That should cut down on the scaffolding a fair amount.
> 
> In Credentials, host is just a string

ugh, you're right. Okay, your approach is fine for now until we can clean that up. this object shouldn't be creating executives and users directly.

one nit: parameters should be lowercased, so user=User.

[arno.]

Created attachment 155383 [details]
same patch; typo fixed

[Dirk Pranke]

Comment on attachment 155383 [details]
same patch; typo fixed

thanks!

[WebKit Review Bot]

Comment on attachment 155383 [details]
same patch; typo fixed

Clearing flags on attachment: 155383

Committed r124140: <http://trac.webkit.org/changeset/124140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.