Bug 46764 – Record bot ID when updating queue status

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

[Mihai Parparita]

Record bot ID when updating queue status

[Mihai Parparita]

Created attachment 69129 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 69129 [details]
Patch

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

Working with you is such a pleasure, Mihai.  Thank you for the patch!

r- because I think we'd end up with Browser() shared between all StatusServer instances (not that there is normally more than one).  Otherwise looks great.

> WebKitTools/QueueStatusServer/model/queuestatus.py:34
> +    bot_id = db.StringProperty()

Will AppEngine automatically migrate the datastore correctly?

> WebKitTools/Scripts/webkitpy/common/net/statusserver.py:45
> +    def __init__(self, host=default_host, browser=Browser(), bot_id=socket.gethostname()):

This is subtly wrong in 2 ways.

1.  foo=bar should be avoided in __init__ because bar will be shared across all instances of the object (at least that's my understanding).  The recommended pattern is to use foo=None and then in teh body if foo is None set it to the default value.
2.  I think we want to avoid giving the entire dns name?  Or maybe I'm understanding gethostname() incorrectly?  Will that just return amazon-ews-12345 or will it return the full DNS address?

I think we might want to add a global command-line option to set the bot name?  There is already a status-server-host= option which is global and passed to the StatusServer at creation time.

> WebKitTools/Scripts/webkitpy/common/net/statusserver_unittest.py:36
> +class StatusServerTest(unittest.TestCase):

I love that you added StatusServerTest.  We've needed this for a long time, but I think been too lazy to mock Browser.

We have a bunch of our mocks dumped in mocktool.py, I can't remember if there i a MockBrowser there.

[Eric Seidel (no email)]

On my laptop:
>>> import socket
>>> socket.gethostname()
'eseidel-macbookpro.local'

I think we probably want these to be explicit names passed on the command line, but I'm not sure.

[Mihai Parparita]

Created attachment 69149 [details]
Patch

[Mihai Parparita]

(In reply to comment #2)
> > WebKitTools/QueueStatusServer/model/queuestatus.py:34
> > +    bot_id = db.StringProperty()
> 
> Will AppEngine automatically migrate the datastore correctly?

Yes, old entities should just have a None for the bot_id (the UI will have to handle that though).
 
> > WebKitTools/Scripts/webkitpy/common/net/statusserver.py:45
> > +    def __init__(self, host=default_host, browser=Browser(), bot_id=socket.gethostname()):
> 
> This is subtly wrong in 2 ways.
> 
> 1.  foo=bar should be avoided in __init__ because bar will be shared across all instances of the object (at least that's my understanding).  The recommended pattern is to use foo=None and then in teh body if foo is None set it to the default value.

Oops, fixed to use None as the default value.

> 2.  I think we want to avoid giving the entire dns name?  Or maybe I'm understanding gethostname() incorrectly?  Will that just return amazon-ews-12345 or will it return the full DNS address?
> 
> I think we might want to add a global command-line option to set the bot name?  There is already a status-server-host= option which is global and passed to the StatusServer at creation time.

Made into a comand-line option.

> I love that you added StatusServerTest.  We've needed this for a long time, but I think been too lazy to mock Browser.
> 
> We have a bunch of our mocks dumped in mocktool.py, I can't remember if there i a MockBrowser there.

There isn't. There was a MockBrowser in bugzilla_unittest.py that I based mine on. I moved it to mocktool.py and used it in both places.

[Adam Barth]

Comment on attachment 69149 [details]
Patch

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

The one think I would change is that we should forward the bot_id to the child process in run_webkit_patch (or wherever we forward the status_host).

> WebKitTools/Scripts/webkitpy/common/net/statusserver_unittest.py:42
> +    def setUp(self):
> +        self.output_capture = OutputCapture()
> +        self.output_capture.capture_output()
> +
> +    def tearDown(self):
> +        self.output_capture.restore_output()

Should we put this code in a base class?  Maybe in the outputcapture package?

> WebKitTools/Scripts/webkitpy/tool/main.py:63
> +        make_option("--bot-id", action="store", dest="bot_id", type="string", help="Identifier for this bot (if multiple bots are running for a queue)"),

Should this be a global option or an option on AbstractQueue?  I guess it makes sense as a global option in case we need to pass it to commands along with status_host.

[Eric Seidel (no email)]

Comment on attachment 69149 [details]
Patch

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

>> WebKitTools/Scripts/webkitpy/common/net/statusserver_unittest.py:42
>> +    def setUp(self):
>> +        self.output_capture = OutputCapture()
>> +        self.output_capture.capture_output()
>> +
>> +    def tearDown(self):
>> +        self.output_capture.restore_output()
> 
> Should we put this code in a base class?  Maybe in the outputcapture package?

Why discard the output instead of using output_capture.assert_output()?  It's much stronger to check the output when possible.

[Mihai Parparita]

Created attachment 69235 [details]
Patch

[Adam Barth]

Comment on attachment 69235 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:66
> +        tool.status_server.bot_id = "gort"

:)

[Mihai Parparita]

(In reply to comment #6)
> Should we put this code in a base class?  Maybe in the outputcapture package?

Sure, added OutputCaptureTestCaseBase.

> Should this be a global option or an option on AbstractQueue?  I guess it makes sense as a global option in case we need to pass it to commands along with status_host.

Yeah, we should pass it around. Added that.

(In reply to comment #7)
> Why discard the output instead of using output_capture.assert_output()?  It's much stronger to check the output when possible.

I don't think we should make the unit tests too rigid, otherwise they'll break even for changes that shouldn't affect them. The logging in StausServer.update_status is not the core functionality (the uploading to the server is), therefore checking for it in an unit test doesn't seem necessary.

[WebKit Commit Bot]

Comment on attachment 69235 [details]
Patch

Clearing flags on attachment: 69235

Committed r68673: <http://trac.webkit.org/changeset/68673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.