Bug 84642 – Make garden-o-matic work for the Apple Mac port

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

As discussed at the WebKit Developers Conference,  garden-o-matic needs to work for the Apple Mac port.

[Dirk Pranke]

cc'ing abarth since I believe he is most in the know about what's remaining to support this. I'll be happy to work on it once I can sync up w/ him.

[Ojan Vafai]

I outlined the steps I think are needed in bug 82719. Copy-pasting here for easier reference.

If I were doing this, I'd do the following set of patches:
1. Adding a drop-down in the top-right corner of garden-o-matic to choose your platform and then add a builders.js file for Apple (we can add other ports later when all the kinks are worked out).
2. Fix all the garden-o-matic JS code that uses build.chromium.org to use build.webkit.org for non-chromium platforms. Off the top of my head, the only things that need to change are the paths to the expected/actual results and the "master" query parameter passed to test-results.appspot.com (ChromiumWebkit --> webkit.org).
3. Fix webkit-patch rebaseline-test to do the right thing for non-Chromium bots. I think this is just making it grab actual results off build.webkit.org instead of build.chromium.org.

[Simon Fraser (smfr)]

Going to take a stab at this.

[Ojan Vafai]

As a followup to the steps I outlined, I think we might also want an "all platforms" view for garden-o-matic for the use-case of wanting to rebaseline a test you just committed across all platforms. But I think that's a natural followup change. The primary use-case is wanting to get your platform's builders green and for that we want a dropdown where you pick your platform.

[Dirk Pranke]

it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).

[Ojan Vafai]

(In reply to comment #5)
> it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).

I'm not set on the specifics of the UI. We can always tweak it if we find it's less than ideal. Simon, feel free to approach this bit however you want. My point was more that the UI currently assumes one platform (chromium) and making the UI mutli-platform aware is probably not as good as letting you pick the platform given the primary use-cases.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > it seems to me that it might make sense to specify the default platform on the command line (using the usual -platform/--chromium/--gtk/etc flags) rather than forcing the user to always select from the drop down. I'm not even sure we need/want the dropdown at all (although I do think an "all platforms" is a nice long-term dream).
> 
> I'm not set on the specifics of the UI. We can always tweak it if we find it's less than ideal. Simon, feel free to approach this bit however you want. My point was more that the UI currently assumes one platform (chromium) and making the UI mutli-platform aware is probably not as good as letting you pick the platform given the primary use-cases.

Oh, yeah, I agree that single-port is a better default than all ports.

[Simon Fraser (smfr)]

I was going to add a platform flag, which sets the popup to the platform. But I agree that switching platforms on the fly isn't that important (other than possibly making development easier).

[Adam Barth]

smfr: Please feel free to grab me on #webkit if you have any questions.  I would probably start by hacking builders.js and looking to see what breaks.

[Simon Fraser (smfr)]

I think we're going to need some server-side changes on b.w.o here. Chromium uses a consistent directory layout, and keeps json results around after zipping up test results:

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_7/140722/

build.webkit.org, however, has results directories like:
http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20Tests/r120136%20%284%29/
then at some point moves things to old-results:
http://build.webkit.org/old-results/Lion%20Intel%20Release%20%28Tests%29/
but doesn't keep the json file separate from the zip file.

[Simon Fraser (smfr)]

Also note the %20 vs underscores in the directory names.

[Simon Fraser (smfr)]

Created attachment 147206 [details]
Work in progress

[Simon Fraser (smfr)]

Comment on attachment 147206 [details]
Work in progress

Comments on the patch welcome, but not ready for review.

[WebKit Review Bot]

Attachment 147206 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/BuildSlaveSupport/build.webkit.org-c..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/builders.py:56:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

(In reply to comment #10)
> I think we're going to need some server-side changes on b.w.o here. Chromium uses a consistent directory layout, and keeps json results around after zipping up test results:
> 
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_7/140722/
> 
> build.webkit.org, however, has results directories like:
> http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20Tests/r120136%20%284%29/
> then at some point moves things to old-results:
> http://build.webkit.org/old-results/Lion%20Intel%20Release%20%28Tests%29/
> but doesn't keep the json file separate from the zip file.

Looks like you're on the right track with this patch. Do we know when things get moved to old-results? I would imagine if it's not for 24 hours or more after the build completes, we can probably ignore its existence.

[Adam Barth]

Comment on attachment 147206 [details]
Work in progress

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

Yep.  Definitely looks like you're on the right track.

Having a directory on the buildbot that accumulates results for each builder is pretty useful.  If we can add that to build.webkit.org, that would be helpful both for the UI and for the backend of the rebaseline functionality.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/config.js:101
> +    config.kCurrentPlatform = platform;

Just a style nit: The "k" in kCurrentPlatform means "constant".  You might want to call this variable config.currentPlatform

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
> +        // FIXME: can jQuery do this?

I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:110
> +        Object.keys(config.kPlatforms).forEach(function(platformName) {

You might want to sort this list before iterating over it.  Object.keys can give things back in a wacky order.

[Adam Barth]

In case you haven't found it yet, you can run the unit test by opening http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/run-unittests.html in a browser.

[Simon Fraser (smfr)]

build.chromium.org has directories which I presume are symlinks to the most recent results, like:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/

b.w.o has the revision and build number in the url:
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r120154%20%2816%29/
and no 'latest' symlink.

[Adam Barth]

They're not actually symlinks.  The results of each build are just spammed into the folder, so it accumulates the most recent failure for each test.  Having a symlink to the latest build would probably work fine for garden o matic.  Accumulating the results is most useful for the flakiness dashboard to show you how each test failed during the most recent flake.

[Simon Fraser (smfr)]

I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(

[Adam Barth]

Hey now.  Be nice.  :)

[Ojan Vafai]

(In reply to comment #19)
> They're not actually symlinks.  The results of each build are just spammed into the folder, so it accumulates the most recent failure for each test.  Having a symlink to the latest build would probably work fine for garden o matic.  Accumulating the results is most useful for the flakiness dashboard to show you how each test failed during the most recent flake.

TL;DR: Do whatever is easiest for you here.

Historically, the spamming was done because it was easiest to do it that way with some complexity of chromium's buildbot machinery. If you just symlink, all the tools should work fine. The only thing you'd lose is the ability to see actual results for flaky tests (e.g. if a test fails one run, but passes in the most recent run, you wouldn't be able to easily see the output of the failure run in the flakiness dashboard). It's actually no worse than the current state of the flakiness dashboard for the b.w.o. bots.

[Ojan Vafai]

Comment on attachment 147206 [details]
Work in progress

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

I don't have any significant feedback either. This patch is doing it all as I'd expect/want.

> Tools/ChangeLog:14
> +        (.):

Side-nit: we really should change prepare-ChangeLog to understand anonymous top-level JS functions better so we don't get this spam in the output. Obviously, not related to the patch in question. :)

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
>> +        // FIXME: can jQuery do this?
> 
> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.

I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.

Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:98
> +                (RegExp(name + '=' + '(.+?)(&|$)').exec(location.search)||[,null])[1]

I like this! It's a bit hard to read, but it's a nice one-line way of doing this.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:107
> +        var platformSelect = document.getElementById('platform-picker');

Looks like you haven't hooked up an onchange handler to this yet. When do you, I think the easiest thing might be to change the platform parameter and just window.location.reload(). That way you don't have to worry about cleaning up global state or other single-platform assumptions garden-o-matic makes.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:108
> +        $(platformSelect).empty();

Nit: I don't think this does anything here since it's already empty.

[Ojan Vafai]

(In reply to comment #20)
> I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(

An alternative to changing the server would be having garden-o-matic learn to grab the actual results files using URLs that include the revision number of the latest run. We actually have access to this from the full_results.json file that we already load in order to generate the list of failing tests. The json has a "revision" key that should match the revision number in the b.w.o. urls.

This is what the flakiness dashboard does to handle the b.w.o. bots currently and it works fine. It just makes the JS code a little more complicated. I think changing the server might be simpler in the long-run, but I'm fine with doing this either way.

[Adam Barth]

Comment on attachment 147206 [details]
Work in progress

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

>>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
>>> +        // FIXME: can jQuery do this?
>> 
>> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.
> 
> I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.
> 
> Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.

I think he meant parse query parameters out of URLs.  :)

[Ojan Vafai]

(In reply to comment #25)
> (From update of attachment 147206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147206&action=review
> 
> >>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:95
> >>> +        // FIXME: can jQuery do this?
> >> 
> >> I think so.  If not, we should probably add this sort of function to base.js so we can re-use it elsewhere.
> > 
> > I don't see anything with a quick glance at http://docs.jquery.com/index.php?title=UI. Also, all we really want here is a select element. I'd rather we not add a complex jquery widget here.
> > 
> > Having a generic function that takes a dictionary-style data-structure and spits out a select element (or populates an existing select element?) in base seems reasonable though.
> 
> I think he meant parse query parameters out of URLs.  :)

Lol. That makes more sense.

[Simon Fraser (smfr)]

(In reply to comment #24)
> (In reply to comment #20)
> > I'm starting to think that garden-o-matic requires quite a bit of server-side support. I wish Chromium hadn't forked the tools so long ago :(
> 
> An alternative to changing the server would be having garden-o-matic learn to grab the actual results files using URLs that include the revision number of the latest run. We actually have access to this from the full_results.json file that we already load in order to generate the list of failing tests. The json has a "revision" key that should match the revision number in the b.w.o. urls.
> 
> This is what the flakiness dashboard does to handle the b.w.o. bots currently and it works fine. It just makes the JS code a little more complicated. I think changing the server might be simpler in the long-run, but I'm fine with doing this either way.

Yeah, this was going to be my next approach. We're also getting revs and build numbers from each builder, but I haven't figured out the loading dependencies yet.

[Simon Fraser (smfr)]

Got as far as requesting full_results.json from build.webkit.org, but it doesn't set the Access-Control-Allow-Origin: * header, so I can't load them.

[Simon Fraser (smfr)]

Filed bug 89164 on b.w.o for CORS stuff.

[Simon Fraser (smfr)]

Created attachment 147729 [details]
More work in progress.

[Adam Barth]

(In reply to comment #28)
> Got as far as requesting full_results.json from build.webkit.org, but it doesn't set the Access-Control-Allow-Origin: * header, so I can't load them.

If you want to continue to make progress, you can hack SecurityOrigin::canRequest to always return true, or you can set Settings::disableWebSecurity to true.

[Simon Fraser (smfr)]

Created attachment 147951 [details]
Patch

[Simon Fraser (smfr)]

Patch is good for review. A couple of unit tests fail (checkout: rebaseline, ui.notifications: BuildersFailing). Not sure if that's my fault or not.

The perf panel doesn't work with the Apple builders. Haven't figured out why not yet.

[Adam Barth]

Comment on attachment 147951 [details]
Patch

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

This looks great!  A bunch of minor comments below.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/LayoutTestResultsLoader.js:67
> +        window.console.log('fetching NRWT results ', this._builder.resultsDirectoryURL(buildName) + 'full_results.json')

Should we put these console message behind a debug flag?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:81
> +// FIXME: export, called by results.
> +function indexOfMostRecentCompletedBuild(individualBuilderStatus)

Usually the way we export things is with this idiom:

builders.indexOfMostRecentCompletedBuild = function(indexOfMostRecentCompletedBuild)
...

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:140
> +    net.get(builderInfoURL, function(builderInfo) {
> +        callback(builderInfo);
> +    });

You can just pass callback directly to net.get.  There's no need for the extra thunk.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:152
> +            selectURL += (first ? '?' : '&') + 'select=' + buildNumber;

A slightly better idiom for doing this is to build up a dictionary and then use $.param to convert the dictionary into a query string.  That will handle the escaping correctly in case anything funky is going on.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/builders.js:158
> +        net.get(selectURL, function(cachedBuildsInfo) {
> +            callback(cachedBuildsInfo);
> +        });

Same comment about the extra thunk.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/model.js:174
> +        // if (testName != 'editing/spelling/grammar-edit-word.html')
> +        //     return;

We should probably remove this before landing.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/model.js:184
> -
> +            

This change seem spurious.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/net.js:61
> +            // window.console.log('failed to load', url, textStatus, errorThrown);

We should either remove this line or uncomment it and put it behind a debug flag.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:393
> +                continueWalk();

I always worry that this function is going to overflow its stack, but I think we can wait for that problem to occur before worrying about it too much.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:621
> +        net.jsonp(buildURLs[currentIndex], resultsCallback);

There's probably a way of avoiding repeating this line, but it's probably not worthwhile.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:629
> +        var requestsInFlight = builderNameList.length;

Sorry to have created a merge conflict for you in this file.  You should feel free to revert my change to this function.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/results.js:643
> +                    ++requestsInFlight;

Interesting.  I'd probably change this around to use RequestTracker, but you or I can do that in a later patch.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:99
> +        function getURLParameter(name) {
> +            return decodeURI(
> +                (RegExp(name + '=' + '(.+?)(&|$)').exec(location.search)||[,null])[1]
> +            );
> +        }

Can you move this function to base.js?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:106
> +        var platformSelect = document.getElementById('platform-picker');

Typically we'd say $(this, "#platform-picker") to scope this selector to this object.  It's not super important here, though, because this UI element really is a singleton.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:108
> +        Object.keys(config.kPlatforms).forEach(function(platformName) {

You should always call sort() after calling Object.keys so that you iterate over the keys in a predictable order.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui.js:122
> +            var url = window.location.href;
> +            var queryIndex = url.indexOf('?');
> +            if (queryIndex != -1)
> +                url = url.substring(0, queryIndex);
> +            window.location.href = url + '?platform=' + platformSelect.selectedOptions[0]._platform;

Why not just assign to window.location.search?

location.search = '?platform=' + platformSelect.selectedOptions[0]._platform;

That also has the advantage of preserving the fragment, which we use for remembering which tab is currently open.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles/notifications.css:93
> +        opacity: 0.2;
> +        -webkit-transition: opacity 0.5s;

Nice idea.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/styles/onebar.css:47
> +  float: right;
> +  margin: 8px;
> +  font-size: larger;

We've been using four space indent for CSS.

[Adam Barth]

> Patch is good for review. A couple of unit tests fail (checkout: rebaseline, ui.notifications: BuildersFailing). Not sure if that's my fault or not.

Don't worry about them.  I'll take a look at these after you land your patch.

> The perf panel doesn't work with the Apple builders. Haven't figured out why not yet.

The perf panel uses http://build.chromium.org/f/chromium/perf/dashboard/overview.html.  We should probably just disable the tab on non-Chromium ports for now.  In the future, we could integrate with or link to http://webkit-perf.appspot.com/

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/120546

[Chris Dumez]

The following webkitpy tests are failing after this patch for both GTK and EFL ports:
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_complex_shadowing
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_mac_future
webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_no_add_mac_future

[Csaba Osztrogonác]

(In reply to comment #37)
> The following webkitpy tests are failing after this patch for both GTK and EFL ports:
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_complex_shadowing
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_mac_future
> webkitpy.common.checkout.baselineoptimizer_unittest.BaselineOptimizerTest.test_no_add_mac_future

New bug to track these failures: https://bugs.webkit.org/show_bug.cgi?id=89332

[Eric Seidel (no email)]

Super exciting!