Closed Bug 774942 Opened 12 years ago Closed 10 years ago

Data.js should separate out regular expressions into a JSON file

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: k0scist, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

From http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/raw-file/tip/js/Data.js: 

      // see Config.testNames and Config.buildNames
      var type =
        /talos.*xperf$/i.test(name) ? "Talos xperf" :
        /talos.*tsspider$/i.test(name) ? "Talos sspider" :
        /talos.*twinopen$/i.test(name) ? "Talos winopen" :
        /talos.*tpan$/i.test(name) ? "Talos pan" :
        /talos.*tdhtml$/i.test(name) ? "Talos dhtml" :

This walks through a bunch of regexes and tries to match each of them in turn.  It would be more consumable if instead it used a JSON structure in a separate file:

{"Talos xperf": "talos.*xperf",
 "Talos sspider": "talos.*tsspider",
... etc
}
Attached patch patch (obsolete) — Splinter Review
This change looks sane enough to me, but I'm not sure if I'm fetching the file from the right place or even how to test the whole thing.  Advice would be appreciated.

I chose sync XHR so as to not massively rewrite all of this.
Attachment #660068 - Flags: feedback?(jhammel)
Attached patch patch (obsolete) — Splinter Review
Attachment #660068 - Attachment is obsolete: true
Attachment #660068 - Flags: feedback?(jhammel)
Attachment #660090 - Flags: feedback?(jhammel)
Attachment #660090 - Flags: feedback+
Comment on attachment 660090 [details] [diff] [review]
patch

This looks good to me and allows me to get the information I need to get, but I'm not a TBPL peer.
Attachment #660090 - Flags: feedback?(jhammel) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #1)
> or even how to test the whole thing

Testing client-side changes is just a case of changing a Config.js pref and loading from the local filesystem :-)

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/tip/README#l21
Attachment #660090 - Flags: feedback?(mstange)
Comment on attachment 660090 [details] [diff] [review]
patch

Looks good, except for the async: false part. If there's an simple way to load the file asynchronously while making sure that it's loaded before any calls to getMachine are made, I'd prefer that.
Attachment #660090 - Flags: feedback?(mstange) → feedback+
Attached patch patchSplinter Review
Here's a patch that moves the load up into Controller.init and delays the code there until after the load completes.  It's not obvious to me that this is any better, since this delays setting up the whole UI--and doesn't display the UI if the file can't be found!--but I am not familiar with the code to find a happy medium.

I didn't use loadTracker because I wasn't sure how calling bits in loadTracker would interact with Controller's initialization not being done yet.
Attachment #660090 - Attachment is obsolete: true
Attachment #660455 - Flags: feedback?(mstange)
Thinking about this - isn't it just going to break the ability for TBPL to run locally? (using the method in comment 4)

We could of course use |url: baseURL + "TestRegexpsMap.json"|, however that will prevent me from easily testing regex changes in the future.

As such, we need to get this working from the local filesystem before it can land, if that's ok?
(In reply to Ed Morley (Away 18th-20th) [:edmorley UTC+1] from comment #7)
> As such, we need to get this working from the local filesystem before it can
> land, if that's ok?

Sure!  No sense in fixing bugs if you just break other useful things.
I don't think there's a good way to arrange things so the local filesystem and the server cases both work.

How about this hack instead: We format the JSON specially so it can be extracted by interested parties, something like:

// Explanatory comment about the peculiar formatting.
var TestRegexpsMap =
// JSON BEGIN MARKER
{
   ...whatever information needed...
   ...note that we can't have comments in here...
}
// JSON END MARKER
;

Then any interested parties can retrieve the .js file where this is stored (from hg or from tbpl itself) and locate the relevant JSON straightforwardly.

It's pragmatic, it's not pretty.  Opinions?
FWIW, the current format is readable in some horrible manner: http://k0s.org/mozilla/hg/talosnames/file/277b167060e5/talosnames/api.py#l82

Its horrible, unreliable, and horrible.  I'd rather move towards something canonical and sensible.  I'd rather not do this in a hacky way.
Actually, why don't we make the list part of Config.js? Then we get synchronous loading for free because <script>s always wait for each other.
(In reply to Markus Stange from comment #11)
> Actually, why don't we make the list part of Config.js? Then we get
> synchronous loading for free because <script>s always wait for each other.

I don't see how that addresses the issue of trying to make the regexes available via JSON.
It wouldn't; is that a goal? Comment 0 talks about making it consumable, so maybe it is... what would be the consumers? I guess I'm not clear on the purpose of this bug. If we just want to make things more readable and easier to find when changing the code, we don't need to make the list externally available.
The goal is to have something consumable to other pieces of software so that interoperability with TBPL may be ensured.  Let's say you're writing a piece of software that wants to see the TBPL name (and letter, which is keyed off the name separately) from the buildbot suite name (the thing the regex matches against).  See comment 10 for how I do this now.
Comment on attachment 660455 [details] [diff] [review]
patch

Oh, I missed that. OK, so the Config.js / <script> approach won't fly.
I think the current patch is good, except for the objection about running from the local file system. I don't have an opinion on that so I'm passing the baton to edmorley.
Attachment #660455 - Flags: feedback?(mstange) → feedback?(bmo)
Comment on attachment 660455 [details] [diff] [review]
patch

We really can't break testing from the local filesystem; it will be an immense pain for me to have to fire up a VM every time I want to work on TBPL's frontend - and it increases the level of effort required from new contributors wanting to help out (I'm looking to get TBPL added to http://whatcanidoformozilla.org soon, so this will be an even bigger issue then).
Attachment #660455 - Flags: feedback?(bmo) → feedback-
(In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> and it increases the level of effort required from new
> contributors wanting to help out 

eg the new contributor in bug 683833.
Product: Webtools → Tree Management
Wontfix since TBPL is being replaced by Treeherder.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: