Bug 64246

Created attachment 100240 [details] Work in progress

Comment on attachment 100240 [details] Work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=100240&action=review > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:72 > + r"Webkit Win": "chromium-win-xp", > + r"Webkit Vista": "chromium-win-vista", > + r"Webkit Win7": "chromium-win-win7", > + r"Webkit Win (dbg)(1)": "chromium-win-win7", # FIXME: Is this correct? > + r"Webkit Win (dbg)(2)": "chromium-win-win7", # FIXME: Is this correct? > + r"Webkit Linux": "chromium-linux-x86_64", > + r"Webkit Linux 32": "chromium-linux-x86", > + r"Webkit Linux (dbg)(1)": "chromium-linux-x86_64", > + r"Webkit Linux (dbg)(2)": "chromium-linux-x86_64", > + r"Webkit Mac10\.5": "chromium-mac-leopard", > + r"Webkit Mac10\.5 (dbg)(1)": "chromium-mac-leopard", > + r"Webkit Mac10\.5 (dbg)(2)": "chromium-mac-leopard", > + r"Webkit Mac10\.6": "chromium-mac-snowleopard", > + r"Webkit Mac10\.6 (dbg)": "chromium-mac-snowleopard", You want builders.py for all this information. builders.py didn't exist when rebaseline.py was written.

Created attachment 100242 [details] Work in progress

Created attachment 100247 [details] Patch

Comment on attachment 100247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100247&action=review I can look again after some sleep. > Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py:39 > + # Instead of using url-encoded buider names, build.chromium.org uses this > + # underscore variation in some places. > + def _directory_encoded_name(self): > + return builder_path_from_name(self._name) This may not be worth being a single-line function. But OK. > Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py:49 > + # In addition to per-build results, the build.chromium.org builders also > + # keep a directory that accumulates test results over many runs. > + def accumulated_results_url(self): > + return self.results_url() + "/results/layout-test-results" Seems you should add this to the baseclass too. And it should return None? > Tools/Scripts/webkitpy/common/net/web.py:36 > +class Web(object): > + def get_binary(self, url): > + return NetworkTransaction().run(lambda: urllib2.urlopen(url).read()) Cute. I'm not sure thsi is going to end up being our best design (I can't think of a parallel object in any nice framework I've written to before), but seems OK. this is some sort of HTTPRequest object it seems. > Tools/Scripts/webkitpy/layout_tests/port/builders_unittest.py:41 > + self.assertEquals(expected, builders.builder_path_from_name(name)) Such a shame that we use this as a module instead of as a BuilderList object (which would be much more mockable). > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:48 > # For now it's here, until we have a second need for it. > class BuilderToPort(object): > _builder_name_to_port_name = { I agree you should stop shaving yaks. But you should fix the FIXME to say this should move to builders.py. > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:106 > + def _rebaseline_test(self, builder_name, test_name, suffix): > + results_url = self._results_url(builder_name) > + baseline_directory = self._baseline_directory(builder_name) This function just about exceeds my mental length limit. At least at 3am. I would have found a way to split it I think. > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:114 > + data = self._tool.web.get_binary(source_baseline) Yeah, I'm nto sure the Web.py really adds much here.

(In reply to comment #5) > (From update of attachment 100247 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100247&action=review > > I can look again after some sleep. > > > Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py:39 > > + # Instead of using url-encoded buider names, build.chromium.org uses this > > + # underscore variation in some places. > > + def _directory_encoded_name(self): > > + return builder_path_from_name(self._name) > > This may not be worth being a single-line function. But OK. Fixed. > > Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py:49 > > + # In addition to per-build results, the build.chromium.org builders also > > + # keep a directory that accumulates test results over many runs. > > + def accumulated_results_url(self): > > + return self.results_url() + "/results/layout-test-results" > > Seems you should add this to the baseclass too. And it should return None? Ok. > > Tools/Scripts/webkitpy/common/net/web.py:36 > > +class Web(object): > > + def get_binary(self, url): > > + return NetworkTransaction().run(lambda: urllib2.urlopen(url).read()) > > Cute. I'm not sure thsi is going to end up being our best design (I can't think of a parallel object in any nice framework I've written to before), but seems OK. this is some sort of HTTPRequest object it seems. This object exists so we can mock out the network, just like the filesystem. > > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:48 > > # For now it's here, until we have a second need for it. > > class BuilderToPort(object): > > _builder_name_to_port_name = { > > I agree you should stop shaving yaks. But you should fix the FIXME to say this should move to builders.py. Done. > > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:106 > > + def _rebaseline_test(self, builder_name, test_name, suffix): > > + results_url = self._results_url(builder_name) > > + baseline_directory = self._baseline_directory(builder_name) > > This function just about exceeds my mental length limit. At least at 3am. I would have found a way to split it I think. Ok. I tried to make it smaller. > > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:114 > > + data = self._tool.web.get_binary(source_baseline) > > Yeah, I'm nto sure the Web.py really adds much here. It's needed so that the unit tests don't actually try to fetch things from the network!

Created attachment 100332 [details] Patch

Comment on attachment 100332 [details] Patch Thank you for splitting up rebaseline-test. I'm still not convinced Web is going to be a long-term-useful API, but I will wait to judge. :)

Comment on attachment 100332 [details] Patch Clearing flags on attachment: 100332 Committed r90770: <http://trac.webkit.org/changeset/90770>

All reviewed patches have been landed. Closing bug.