Bug 64246 – Add a webkit-patch command for rebaselining an individual test

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

[Adam Barth]

Add a webkit-patch command for rebaselining an individual test

[Adam Barth]

Created attachment 100240 [details]
Work in progress

[Eric Seidel (no email)]

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.

[Adam Barth]

Created attachment 100242 [details]
Work in progress

[Adam Barth]

Created attachment 100247 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

(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!

[Adam Barth]

Created attachment 100332 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Adam Barth]

Comment on attachment 100332 [details]
Patch

Clearing flags on attachment: 100332

Committed r90770: <http://trac.webkit.org/changeset/90770>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.