Bug 86242 – garden-o-matic should specify which types of baselines to rebaseline

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

[Dirk Pranke]

garden-o-matic should specify which types of baselines to rebaseline

[Dirk Pranke]

Created attachment 141474 [details]
Patch

[Dirk Pranke]

Okay, I think this roughly does what we want, but I don't really know this code and my javascript coding ability is, shall we say, weak. So, feedback is encouraged.

[Dirk Pranke]

Comment on attachment 141474 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:106
> +            // FIXME: Do we need to handle lists with multiple failure types?

This part in particular I'm not sure about?

[Adam Barth]

Comment on attachment 141474 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:83
> +            'suffixes': suffixes,

Do we need to suffixes.join(',') here too?

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:106
>> +            // FIXME: Do we need to handle lists with multiple failure types?
> 
> This part in particular I'm not sure about?

Yeah, we need to handle multiple failure types.

By the way, this file should have some associated unit tests in checkout_unittests.js.  You can run them your browser by opening run_unittests.html.  It should be easy to test this change.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:107
> +            var suffixes = results.failureTypeToExtensionList(failureInfo.failureTypeList[0]);

I think what you want to do is something like the following:

base.uniquifyArray(base.flattenArray(failureInfo.failureTypeList.map(results.failureTypeToExtensionList)))

[Adam Barth]

Generally looks good.  I think the main thing is you just need to update the unittests to test the new behavior.

[Ojan Vafai]

Comment on attachment 141474 [details]
Patch

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

LGTM as well with more tests.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:182
> +        self._baseline_suffix_list = options.suffixes.split(',')

Dead code.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:211
> +        self._baseline_suffix_list = options.suffixes.split(',')

Looks like if you avoid the join in the calling code, you could avoid needing to call split here as well, a la line 183 above.

[Dirk Pranke]

Created attachment 141515 [details]
make js tests work, update w/ review feedback

[Adam Barth]

Comment on attachment 141515 [details]
make js tests work, update w/ review feedback

Looks great.  Thanks!

[Dirk Pranke]

Committed r116988: <http://trac.webkit.org/changeset/116988>