Bug 90504 – Improve webkit-patch rebaseline to work for more cases

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

[Ojan Vafai]

Improve webkit-patch rebaseline to work for more cases

[Ojan Vafai]

Created attachment 150684 [details]
Patch

[Dirk Pranke]

Comment on attachment 150684 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:378
> +            optparse.make_option("--test", help="Test to rebaseline"),

should we support multiple tests and/or builders here?

It seems like if you don't specify either --builder or --test, then we will prompt the user for each builder and then prompt for the tests on the builder, and that could be pretty annoying, so I'm not sure that this is a great design, but I'm happy to try it out and see.

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 150684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150684&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:378
> > +            optparse.make_option("--test", help="Test to rebaseline"),
> 
> should we support multiple tests and/or builders here?
> 
> It seems like if you don't specify either --builder or --test, then we will prompt the user for each builder and then prompt for the tests on the builder, and that could be pretty annoying, so I'm not sure that this is a great design, but I'm happy to try it out and see.

I'll add a FIXME to do this. We'll see how people use it in practice. Personally, I hate having to type or copy-paste builder or test names, so the lists might be useful.

Mostly, I just want --test so I can rebaseline a test across all ports.

[Ojan Vafai]

Committed r121821: <http://trac.webkit.org/changeset/121821>

[Adam Barth]

Comment on attachment 150684 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:242
> +        # FIXME: We should probably have a better way of determining chromium builders than checking if the name starts with 'Webkit'.

Rather than greping the builder name, why not just add some state to the builder object to track this information?

[Adam Barth]

Comment on attachment 150684 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:243
> +        results_directory = "results/layout-test-results" if self._builder.name().startswith('Webkit') else "r%s (%s)" % (self.revision(), self._number)

The more I think about this line, the more it bothers me.  The fact that the Chromium builders have names that misspell WebKit is something I'd like to fix some day.  Relying upon that error here is really crappy.

This line also contains the string "results/layout-test-results", which repeats a constant in accumulated_results_url().  Rather than repeating ourselves, we should call accumulated_results_url().  In fact, that's key to avoiding introspecting the builder's name.  Rather than looking for the "Webkit" typo, we should check whether accumulated_results_url() is truthy.

Actually, now that I think about this further, this change is just plain wrong.  results_url() no longer returns the results URL for this Build on Chromium buildbots.  Instead, it returns the accumulated_results_url, which contains results other than the ones for this build.  Rather than make this function wrong, we need to make the callers of this function smarter.

[Adam Barth]

Comment on attachment 150684 [details]
Patch

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

> Tools/ChangeLog:29
> +        so these can return different values. In either case, I'm pretty sure these
> +        are not remotely hot codepaths.

Did you check this in any way?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:384
> +        builder_names = [self._chromium_prefix + name if name.startswith('Webkit') else name for name in builders.all_builder_names()]

Another instance of relying on the Webkit typo.  That's really a crappy way to write this code.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:389
> +        if name.startswith(self._chromium_prefix):

More string comparison.  Note: This code will be wrong if any of the build.webkit.orb buildbots happen to be named with the self._chromium_prefix prefix (e.g., the chromium ones).

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:394
> +        if port.name().startswith('chromium-'):

I didn't like this idiom the first time it was introduced.  Rather than copying / pasting it around the codebase, we should refactor the code to either put the hack in a single place, or (better) avoid the need to do this comparison.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:416
> +                # FIXME: Allow for choosing the suffixes.
> +                test_list[test][builder.name()] = ['txt']

Does this mean this command only works for text results?

[Adam Barth]

I'm sad that I didn't have a chance to review this patch in the 75 minutes that it was posted for review.  Given that it's now the July 4th weekend, I'm going to roll out this change so that we don't forget to fix the wrongness this patch introduced.

[WebKit Review Bot]

Re-opened since this is blocked by 90551

[Ojan Vafai]

(In reply to comment #8)
> I'm sad that I didn't have a chance to review this patch in the 75 minutes that it was posted for review.  Given that it's now the July 4th weekend, I'm going to roll out this change so that we don't forget to fix the wrongness this patch introduced.

Rolling out is not a big deal. I'll fix it up after the break. I think maybe you're overreacting a bit because it got committed. IMO, the wrongness in this patch is mostly a result of technical debt already incurred by other things (bot naming conventions, chromiumbuildbot not supporting a good chunk of the buildbot API, etc). You're right though that I could do a better job of minimizing and isolating the wrongness.

I'll post a patch after the break that isolates and avoids the string comparison as much as possible.

(In reply to comment #7)
> (From update of attachment 150684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150684&action=review
> 
> > Tools/ChangeLog:29
> > +        so these can return different values. In either case, I'm pretty sure these
> > +        are not remotely hot codepaths.
> 
> Did you check this in any way?

Not sure what you're asking. Having these be memoized was not useful and was causing the tests to be flaky. You see a problem with removing the memoization here? Is there a purpose to the memoization other than the performance benefit?

> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:416
> > +                # FIXME: Allow for choosing the suffixes.
> > +                test_list[test][builder.name()] = ['txt']
> 
> Does this mean this command only works for text results?

For now. Which is also what the help text says. I'm improving this command incrementally. The follow-on patch was going to make it also work for pngs. Before this patch, this command only worked for text results that weren't new files.

(In reply to comment #6)
> Actually, now that I think about this further, this change is just plain wrong.  results_url() no longer returns the results URL for this Build on Chromium buildbots.  Instead, it returns the accumulated_results_url, which contains results other than the ones for this build.  Rather than make this function wrong, we need to make the callers of this function smarter.

That's a good point. I think probably the best way to fix this is to add a new method latest_full_results_json_url that points to the latest full_results.json for that bot.

[Adam Barth]

The main reason I didn't like the patch was the dependency on the Webkit misspelling, which (to me) is emblematic of a bunch of Chromium practices that I dislike, in particular ignorance of WebKit (i.e., not even being able to spell the name of the project correctly) and unwillingness to fix bugs for fear of breaking things (i.e., this misspelling has persisted for years for this reason even though I've asked the admins to fix it).

[Ojan Vafai]

Created attachment 151295 [details]
Patch

[Ojan Vafai]

I think this should address all of Adam's concerns and also makes --builders/--tests take a CSV as per Dirk's request.

> > > Tools/ChangeLog:29
> > > +        so these can return different values. In either case, I'm pretty sure these
> > > +        are not remotely hot codepaths.
> > 
> > Did you check this in any way?
> 
> Not sure what you're asking. Having these be memoized was not useful and was causing the tests to be flaky. You see a problem with removing the memoization here? Is there a purpose to the memoization other than the performance benefit?

Ah, I misparsed this. I thought you were asking why I committed the change. I checked this manually by looking at all the callers. I didn't run any tests, but it's pretty clear to me that none of the callers are in hot code paths, except possibly one of them, but that one already memoizes the calling function. Added a comment to the ChangeLog to that effect.

[Dirk Pranke]

Comment on attachment 151295 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:377
> +            optparse.make_option("--tests", default=None, help="Comma-separated-list of tests to rebaseline"),

Nit: for multiple values I usually use action=append and so you do --builder=foo --builder=bar.

Also, it occurred to me that the common case is to do one or more tests, so we should probably just make those args instead of options, and not allow "all" tests (or make that an explicit opt-in with --all-tests or something).

[Ojan Vafai]

Created attachment 151377 [details]
Patch

[Ojan Vafai]

(In reply to comment #14)
> (From update of attachment 151295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151295&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:377
> > +            optparse.make_option("--tests", default=None, help="Comma-separated-list of tests to rebaseline"),
> 
> Nit: for multiple values I usually use action=append and so you do --builder=foo --builder=bar.

Sure. I made it append, but still accepting a comma-separated-list. So people can do whatever they prefer.

> Also, it occurred to me that the common case is to do one or more tests, so we should probably just make those args instead of options, and not allow "all" tests (or make that an explicit opt-in with --all-tests or something).

I think you're probably right. I changed it.

[Adam Barth]

Comment on attachment 151377 [details]
Patch

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

Thanks for iterating on this patch.  Sorry for being a complainy pants before...

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

Why not just @memoize?

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:88
> +        # FIXME: Use self._tool.buildbot_for_builder_name(builder_name).
>          port = self._tool.port_factory.get_from_builder_name(builder_name)
> -        # FIXME: Come up with a better way than string manipulation to see if the port is a chromium port.
> -        if port.name().startswith('chromium-'):
> +        if port.is_chromium():
>              return self._tool.chromium_buildbot().builder_with_name(builder_name).accumulated_results_url()
>          return self._tool.buildbot.builder_with_name(builder_name).latest_cached_build().results_url()

Why not refactor latest_layout_test_results slightly to separate the URL finding from the results fetching?  That will let you get rid of the is_chromium branch.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:414
> +            if len(args):

You can just say "if args:" or, even better:

tests = args or self._tests_to_update(builder)

[Ojan Vafai]

(In reply to comment #17)
> (From update of attachment 151377 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151377&action=review
> 
> Thanks for iterating on this patch.  Sorry for being a complainy pants before...
> 
> > Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:69
> > +    def latest_layout_test_results(self):
> 
> Why not just @memoize?

I was mimicing the code in Build. Changed both.

> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:88
> > +        # FIXME: Use self._tool.buildbot_for_builder_name(builder_name).
> >          port = self._tool.port_factory.get_from_builder_name(builder_name)
> > -        # FIXME: Come up with a better way than string manipulation to see if the port is a chromium port.
> > -        if port.name().startswith('chromium-'):
> > +        if port.is_chromium():
> >              return self._tool.chromium_buildbot().builder_with_name(builder_name).accumulated_results_url()
> >          return self._tool.buildbot.builder_with_name(builder_name).latest_cached_build().results_url()
> 
> Why not refactor latest_layout_test_results slightly to separate the URL finding from the results fetching?  That will let you get rid of the is_chromium branch.

I considered this and decided against it since this could potentially cause use to fetch the file twice (once in Builder and once in Build). On second thought, the code cleanliness is probably worth it. I went ahead and changed it.

[Ojan Vafai]

Committed r122234: <http://trac.webkit.org/changeset/122234>