Bug 54374 – Add method to make BuildBot return test outputs

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

[James Kozianski]

Add method to make BuildBot return test outputs

[James Kozianski]

Created attachment 82285 [details]
Patch

[Ojan Vafai]

Comment on attachment 82285 [details]
Patch

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

r- for lack of unittests.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:97
> +    def latest_build(self):

Nit: while I can see why this might be a useful method, it's not actually used in this patch or described in the ChangeLog description. A one-liner in the ChangeLog description of what this is to be used for would be good.

[Eric Seidel (no email)]

Comment on attachment 82285 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:97
>> +    def latest_build(self):
> 
> Nit: while I can see why this might be a useful method, it's not actually used in this patch or described in the ChangeLog description. A one-liner in the ChangeLog description of what this is to be used for would be good.

The danger with this method is you don't know if it's cached or not. :)

[Ojan Vafai]

Comment on attachment 82285 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:97
>>> +    def latest_build(self):
>> 
>> Nit: while I can see why this might be a useful method, it's not actually used in this patch or described in the ChangeLog description. A one-liner in the ChangeLog description of what this is to be used for would be good.
> 
> The danger with this method is you don't know if it's cached or not. :)

I don't understand. You're saying it might be slow?

[Dirk Pranke]

No, that the data might stale and hence you'd think you had the latest build available but you'd be wrong.

[Dirk Pranke]

(In reply to comment #5)
> No, that the data might stale and hence you'd think you had the latest build available but you'd be wrong.

er, "the data might be stale"

[Ojan Vafai]

Comment on attachment 82285 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:97
>>>> +    def latest_build(self):
>>> 
>>> Nit: while I can see why this might be a useful method, it's not actually used in this patch or described in the ChangeLog description. A one-liner in the ChangeLog description of what this is to be used for would be good.
>> 
>> The danger with this method is you don't know if it's cached or not. :)
> 
> I don't understand. You're saying it might be slow?

So, what's the right way to grab the latest build then? Should this not use revision_build_pairs_with_results()? Should this method just be called latest_cached_build? Should builder have an update_cached_builds method?

Seems to me like just calling it latest_cached_build is probably sufficient for now.

[Eric Seidel (no email)]

if you look around the pyhon, you will find other places where we talk about how it might be nice to have a latest_build method.  I'm not sure what we do in all those cases.

[James Kozianski]

Created attachment 83603 [details]
Patch

[James Kozianski]

Created attachment 83604 [details]
Patch

[Ojan Vafai]

Comment on attachment 83604 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:237
> +    def results_zip_url(self):
> +        return "%s.zip" % self.results_url()
> +
> +    def results(self):
> +        return TestOutputSet(self._builder.name(), None, ZipFileSet(self.results_zip_url()), include_expected=False)
> +

I know this is a bit inane, but can you write tests for these too. They don't need to actually verify the output. It just ensures that they run without errors.

results_zip_url can just verify that it gets the right string back and TestOutputSet can just verify that it gets a non-None value back.

[Eric Seidel (no email)]

Comment on attachment 83604 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:98
> +        revision_build_pairs = self.revision_build_pairs_with_results()

I don't remember.  Isn't this very expensive to compute?

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:232
> +    def results_zip_url(self):

Please test this.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:418
> +        class MockBuilder(Builder):

No need to make this local to the test.  These are very often shared between more than one test.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:420
> +                Builder.__init__(self, 'mock builder', BuildBot())

Mocks shouldn't inherit from real objets.  If you want to test hte real object, just mock out the methods.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:422
> +            def _fetch_build(self, build_number):

I would have probably just replaced the methods on a real object.

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:425
> +                else:

No else after return in WEbKIt.

[James Kozianski]

(In reply to comment #11)
> (From update of attachment 83604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83604&action=review
> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:237
> > +    def results_zip_url(self):
> > +        return "%s.zip" % self.results_url()
> > +
> > +    def results(self):
> > +        return TestOutputSet(self._builder.name(), None, ZipFileSet(self.results_zip_url()), include_expected=False)
> > +
> 
> I know this is a bit inane, but can you write tests for these too. They don't need to actually verify the output. It just ensures that they run without errors.
> 
> results_zip_url can just verify that it gets the right string back and TestOutputSet can just verify that it gets a non-None value back.

Done.

[James Kozianski]

(In reply to comment #12)
> (From update of attachment 83604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83604&action=review
> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:98
> > +        revision_build_pairs = self.revision_build_pairs_with_results()
> 
> I don't remember.  Isn't this very expensive to compute?

_fetch_revision_to_build_map() is expensive, but its result is cached, and that's what revision_build_pairs_with_results() uses.

> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:232
> > +    def results_zip_url(self):
> 
> Please test this.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:418
> > +        class MockBuilder(Builder):
> 
> No need to make this local to the test.  These are very often shared between more than one test.

I've removed this class.

> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:420
> > +                Builder.__init__(self, 'mock builder', BuildBot())
> 
> Mocks shouldn't inherit from real objets.  If you want to test hte real object, just mock out the methods.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:422
> > +            def _fetch_build(self, build_number):
> 
> I would have probably just replaced the methods on a real object.

Done.

> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot_unittest.py:425
> > +                else:
> 
> No else after return in WEbKIt.

Done.

[James Kozianski]

Created attachment 84913 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 84913 [details]
Patch

Clearing flags on attachment: 84913

Committed r81106: <http://trac.webkit.org/changeset/81106>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.