Bug 102443 – Generate a list of builders/test suites from the buildbot json

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Generate a list of builders/test suites from the buildbot json

[Ojan Vafai]

Created attachment 174546 [details]
Patch

[Ojan Vafai]

Created attachment 174549 [details]
Patch

[Dirk Pranke]

Comment on attachment 174549 [details]
Patch

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

This basically looks okay but I'm a bit unsure about the overall direction; possibly this is because I don't know what all you have in mind. Take a look at my comments and let me know what you think?

> Tools/ChangeLog:14
> +        have the test results server generate the data on the fly.

I'm not wild about checking in the generated files, for a few reasons ...

1) It presumably means we have to check in new versions every time the bot configs change, right? I suppose most changes today also induce this, but even so, this seems bad.

2) I don't like checking in all of the configs for the chromium-specific stuff into WebKit; could we push a version of this into chromium and fetch it from src.chromium.org or something?

3) I don't like checking in both builders-pretty.jsonp and builders.jsonp, especially if builders-pretty.jsonp is basically just for making diffs easier to review. I probably wouldn't bother with builders-pretty.jsonp. Also, presumably we can configure the app engine app to compress the file and serve it that way? It looks like builders.json compresses from 22522 -> 2392 and builders-pretty from 50453 to 2769, so it seems like a good win and would suggest that just using builders-pretty.jsonp would be fine.

All of these things can probably be addressed in subsequent patches, assuming we can get on the same page about how this should all work ...

Another idea is to add an app engine entry point to generate and return the list of builders itself (and cache the results), so that we could still fetch the list quickly but we wouldn't need to check in anything.

> Tools/TestResultServer/generate_builders_json.py:68
> +            last_cached_build = cached_builds.pop()

Is "last" oldest or newest/latest/most recent? We should probably rename to one of these to be a little clearer.

> Tools/TestResultServer/generate_builders_json.py:80
> +                # FIXME: Eventually, we should have the other test suites at webkit.org (e.g. the python unittests) upload their results to the test results server as well.

might make sense for the unit tests; the python tests don't fail enough (or, really, fail flakily enough) to be worth tracking.

> Tools/TestResultServer/generate_builders_json.py:119
> +'''

Having two multi-line strings back to back like this is a bit hard to read. Can you replace these with auto-concatenated strings like

json_file_prefix = ('// This file is auto-generated...\n'
                             '// It uses jsonp ...\n'
                             'LOAD_BUILDBOT_DATA(')
json_file_suffix = ');\n'

instead? Or at least put a blank line in between line 117 and 118?

> Tools/TestResultServer/static-dashboards/builders.js:96
> +WEBKIT_BUILDER_MASTER = 'webkit.org';

If these constants are now just strings and only used in the switch inside LoadBuildersList(), I'm not sure if they're worth keeping around, especially if you're not using the constants in LEGACY_BUILDER_MASTERS_TO_GROUPS, below.

> Tools/TestResultServer/static-dashboards/builders.js:250
>  function loadBuildersList(groupName, testType) {

I don't fully understand what all this function is doing, but does it make sense to push much (if not all) of this into the generated JSON as well, so that the generated JSON contains the group names ?

[Ojan Vafai]

Comment on attachment 174549 [details]
Patch

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

>> Tools/ChangeLog:14
>> +        have the test results server generate the data on the fly.
> 
> I'm not wild about checking in the generated files, for a few reasons ...
> 
> 1) It presumably means we have to check in new versions every time the bot configs change, right? I suppose most changes today also induce this, but even so, this seems bad.
> 
> 2) I don't like checking in all of the configs for the chromium-specific stuff into WebKit; could we push a version of this into chromium and fetch it from src.chromium.org or something?
> 
> 3) I don't like checking in both builders-pretty.jsonp and builders.jsonp, especially if builders-pretty.jsonp is basically just for making diffs easier to review. I probably wouldn't bother with builders-pretty.jsonp. Also, presumably we can configure the app engine app to compress the file and serve it that way? It looks like builders.json compresses from 22522 -> 2392 and builders-pretty from 50453 to 2769, so it seems like a good win and would suggest that just using builders-pretty.jsonp would be fine.
> 
> All of these things can probably be addressed in subsequent patches, assuming we can get on the same page about how this should all work ...
> 
> Another idea is to add an app engine entry point to generate and return the list of builders itself (and cache the results), so that we could still fetch the list quickly but we wouldn't need to check in anything.

1) Yes.
2) That seems like too much work given the eventual end result (see below).
3) That's a good point. I didn't look at the gzipped sizes. I'll get rid of builders-pretty.jsonp and just check in the pretty-printed version at builders.jsonp.

The eventual goal is to to your last idea (appengine entry point to serve generate and serve up this file). That's what the sentence above was trying to say. This just seemed like a good incremental step in that direction since the appengine entry point would use this python code to generate the file it serves up.

Sound good?

>> Tools/TestResultServer/generate_builders_json.py:68
>> +            last_cached_build = cached_builds.pop()
> 
> Is "last" oldest or newest/latest/most recent? We should probably rename to one of these to be a little clearer.

Will rename to latest_cached_build.

>> Tools/TestResultServer/generate_builders_json.py:80
>> +                # FIXME: Eventually, we should have the other test suites at webkit.org (e.g. the python unittests) upload their results to the test results server as well.
> 
> might make sense for the unit tests; the python tests don't fail enough (or, really, fail flakily enough) to be worth tracking.

I'll just get rid of the FIXME.

>> Tools/TestResultServer/generate_builders_json.py:119
>> +'''
> 
> Having two multi-line strings back to back like this is a bit hard to read. Can you replace these with auto-concatenated strings like
> 
> json_file_prefix = ('// This file is auto-generated...\n'
>                              '// It uses jsonp ...\n'
>                              'LOAD_BUILDBOT_DATA(')
> json_file_suffix = ');\n'
> 
> instead? Or at least put a blank line in between line 117 and 118?

Will do.

>> Tools/TestResultServer/static-dashboards/builders.js:96
>> +WEBKIT_BUILDER_MASTER = 'webkit.org';
> 
> If these constants are now just strings and only used in the switch inside LoadBuildersList(), I'm not sure if they're worth keeping around, especially if you're not using the constants in LEGACY_BUILDER_MASTERS_TO_GROUPS, below.

WEBKIT_BUILDER_MASTER is used in a bunch of places. Those should probably be changed to having an isWebKitBuilderMaster method on BuilderMaster or something like that, but I was trying to keep this change as small as possible.

The other thing is that some of these are repeated many times in loadBuildersList and it felt a bit dirty to copy-paste the same string over and over.

>> Tools/TestResultServer/static-dashboards/builders.js:250

> 
> I don't fully understand what all this function is doing, but does it make sense to push much (if not all) of this into the generated JSON as well, so that the generated JSON contains the group names ?

Hmmm. That might make sense. I'm not sure. It would make this patch a lot bigger though. Mind if a make that a FIXME?

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 174549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174549&action=review
> 
> >> Tools/ChangeLog:14
> >> +        have the test results server generate the data on the fly.
> > 
> > I'm not wild about checking in the generated files, for a few reasons ...
> > 
> > 1) It presumably means we have to check in new versions every time the bot configs change, right? I suppose most changes today also induce this, but even so, this seems bad.
> > 
> > 2) I don't like checking in all of the configs for the chromium-specific stuff into WebKit; could we push a version of this into chromium and fetch it from src.chromium.org or something?
> > 
> > 3) I don't like checking in both builders-pretty.jsonp and builders.jsonp, especially if builders-pretty.jsonp is basically just for making diffs easier to review. I probably wouldn't bother with builders-pretty.jsonp. Also, presumably we can configure the app engine app to compress the file and serve it that way? It looks like builders.json compresses from 22522 -> 2392 and builders-pretty from 50453 to 2769, so it seems like a good win and would suggest that just using builders-pretty.jsonp would be fine.
> > 
> > All of these things can probably be addressed in subsequent patches, assuming we can get on the same page about how this should all work ...
> > 
> > Another idea is to add an app engine entry point to generate and return the list of builders itself (and cache the results), so that we could still fetch the list quickly but we wouldn't need to check in anything.
> 
> 1) Yes.
> 2) That seems like too much work given the eventual end result (see below).
> 3) That's a good point. I didn't look at the gzipped sizes. I'll get rid of builders-pretty.jsonp and just check in the pretty-printed version at builders.jsonp.
> 
> The eventual goal is to to your last idea (appengine entry point to serve generate and serve up this file). That's what the sentence above was trying to say. This just seemed like a good incremental step in that direction since the appengine entry point would use this python code to generate the file it serves up.
> 
> Sound good?
> 

Yup, sounds good.

> 
> WEBKIT_BUILDER_MASTER is used in a bunch of places. Those should probably be changed to having an isWebKitBuilderMaster method on BuilderMaster or something like that, but I was trying to keep this change as small as possible.
> 
> The other thing is that some of these are repeated many times in loadBuildersList and it felt a bit dirty to copy-paste the same string over and over.
>

Okay. It didn't seem like they were being used that much but I probably missed things.
 
> > I don't fully understand what all this function is doing, but does it make sense to push much (if not all) of this into the generated JSON as well, so that the generated JSON contains the group names ?
> 
> Hmmm. That might make sense. I'm not sure. It would make this patch a lot bigger though. Mind if a make that a FIXME?

Sure, I wasn't expecting you to do that as part of this patch, just more trying to understand the direction/design.

[Dirk Pranke]

Comment on attachment 174549 [details]
Patch

r+ assuming you make the changes you said you would :).

[Ojan Vafai]

Committed r135304: <http://trac.webkit.org/changeset/135304>