Add method to make BuildBot return test outputs
Created attachment 82285 [details] Patch
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.
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. :)
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?
No, that the data might stale and hence you'd think you had the latest build available but you'd be wrong.
(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"
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.
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.
Created attachment 83603 [details] Patch
Created attachment 83604 [details] Patch
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.
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.
(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.
(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.
Created attachment 84913 [details] Patch
Comment on attachment 84913 [details] Patch Clearing flags on attachment: 84913 Committed r81106: <http://trac.webkit.org/changeset/81106>
All reviewed patches have been landed. Closing bug.